View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002133 | OXID eShop (all versions) | 1.02. Price calculations (discounts, coupons, additional costs etc.) | public | 2010-09-25 13:33 | 2012-12-07 15:07 |
| Reporter | andreas_ziethen | Assigned To | |||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 4.4.2 revision 29492 | ||||
| Fixed in Version | 4.5.9 revision 43186 | ||||
| Summary | 0002133: voucher reserved check is done redundantly | ||||
| Description | There are two places where a voucher is checked for beeing already reserved: first one: oxvoucher::_isNotResereved() second one: query in oxvoucher::getVoucherByNr() This is not good cause of you want to modify the reserved logic by module you are not expecting that logic in the second place. It should be in a central place only. | ||||
| Tags | Discount | ||||
| Theme | |||||
| Browser | All | ||||
| PHP Version | any | ||||
| Database Version | any | ||||
|
|
I had a deeper look into this issue now and found some more things which could be done better: oxvoucher::getVoucherByNr() does not only fetch the voucher object by voucher number - which I would expect - but it does a lot more. It does check voucher availability depending on: - oxvoucher.oxorderid has to be NULL or empty - oxvoucher.oxreserved has to be less then now minus 3 hours Both checks in my opinion should be done in different functions - not in getVoucherByNr(). And the general check if a voucher is already used should be done on oxdateused field - not on oxorderid field. E. g. we built a module to manually devalue vouchers. The shop-owner just has to fill in a voucher no. and click button. This is quite often needed if vouchers can be used on other channels as the online shop as well. So I thought it would be enough to set the oxdateused field for such vouchers - but later I realized that I HAVE TO fill in an order id to really make in invalid. I think that should be changed. If oxdateused is != NULL, the voucher should be taken as invalid. And the checks for availability should be on a single place. |
|
|
@Developers: please check from the source code if this is the case as described and can we optimize this check to central place. |
|
|
Checks if voucher used not only by orderid but also by useddate. If any of those fields has value, voucher counts as used. Changed code in oxvoucher.php add line 73 in method getVoucherByNr $sQ .= " and ( {$sViewName}.oxdateused is NULL || {$sViewName}.oxdateused = 0 ) "; changed line 405 in method _isAvailableInOtherOrder $sSelect .= '((oxorderid is not NULL and oxorderid != "") or (oxdateused is not NULL and oxdateused != 0)) '; Changed code in oxvoucherseria.php changed line 150 in method countVouchers $sQuery = 'select count(*) as used from oxvouchers where oxvoucherserieid = ' . $oDB->quote( $this->getId() ) . ' and ((oxorderid is not NULL and oxorderid != "") or (oxdateused is not NULL and oxdateused != 0))'; |
