-
Notifications
You must be signed in to change notification settings - Fork 120
[Woo POS] Coupons: Filter out expired coupons from appearing in POS #15527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
This PR might explain it a bit #6686, but either way it looks like it _gmt is either not working properly or wasn't working properly. 🤔 @iamgabrielma could you check if there's a bug reported on WooCommerce? Worth creating it if it's not yet there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the POS coupon mapping logic to filter out expired coupons so that the POS only shows valid coupons while leaving the rest of the app behavior unchanged.
- Added filtering logic in the coupon-to-POS mapping function
- Introduced a dedicated unit test to verify that expired coupons are excluded from the POS list
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Yosemite/YosemiteTests/PointOfSale/PointOfSaleCouponServiceTests.swift | Added unit tests to check that expired coupons are filtered out |
| Yosemite/Yosemite/PointOfSale/PointOfSaleCouponService.swift | Updated mapping function to filter expired coupons before mapping |
staskus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks for the work.
I tried to look for issues in tests and implementation with timezones but I think it should be alright.
Are there are any concerns you have, e.g. pagination?
| let now = Date() | ||
| let nonExpiredCoupons = coupons.filter { coupon in | ||
| guard let expirationDate = coupon.dateExpires else { return true } | ||
| return expirationDate >= now | ||
| } | ||
|
|
||
| return nonExpiredCoupons.compactMap { coupon in | ||
| .coupon(POSCoupon(id: UUID(), | ||
| code: coupon.code, | ||
| summary: coupon.summary(currencySettings: currencySettings))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; you can chain .filter { } .compact { } functional programming way
| let now = Date() | |
| let nonExpiredCoupons = coupons.filter { coupon in | |
| guard let expirationDate = coupon.dateExpires else { return true } | |
| return expirationDate >= now | |
| } | |
| return nonExpiredCoupons.compactMap { coupon in | |
| .coupon(POSCoupon(id: UUID(), | |
| code: coupon.code, | |
| summary: coupon.summary(currencySettings: currencySettings))) | |
| return coupons | |
| .filter { | |
| guard let expirationDate = $0.dateExpires else { return true } | |
| return expirationDate >= Date() | |
| } | |
| .compactMap { | |
| .coupon( | |
| POSCoupon( | |
| id: UUID(), | |
| code: $0.code, | |
| summary: $0.summary(currencySettings: currencySettings) | |
| ) | |
| ) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL! I'll leave it with the existing implementation for now just because I feel it's more explicit, which makes it easier to read.
I think we could see edge cases where pagination is not triggered correctly if we have a merchant with a bunch of coupons expired on the first fetch (~10+), and nothing else saved locally, since the infiniteScrollTriggerDeterminer merely checks the scroll position. The same could happen for subsequent pages if they happen to have a full page or results where all coupons are expired. I don't think we should optimize for it right now, but perhaps makes a stronger case to go with "let's render expired coupons as well, rather than hide them". |
@iamgabrielma got it. I don't like these edge cases. I'll bring up the question to the team. |

Closes: WOOMOB-246
This PR intercepts mapping coupons to POSItems, and filters out those that are expired (if any), so are not shown in POS while still working normally for the rest of the app:
Caveat: The coupon properties seem to be a bit miss leading: By the API documentation I'd expected we'd have two properties:
However we only use
date_expiresin the app, which is received as UTC anyway from the site. ie: my testing site is in UTC+7, but the coupon expiration date assigned on creation is still in UTC:Testing information
expiredRELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: