-
Notifications
You must be signed in to change notification settings - Fork 119
[Woo POS][Surveys] Schedule notification trigger on opening app, rather than on opening POS #16261
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
[Woo POS][Surveys] Schedule notification trigger on opening app, rather than on opening POS #16261
Conversation
|
|
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.
I think we need to keep setHasPOSBeenOpenedAtLeastOnce in POSTabCoordinator but move scheduleLocalNotificationIfEligible to app coordinator.
| Task { @MainActor in | ||
| await POSNotificationScheduler(stores: stores).scheduleLocalNotificationIfEligible(for: .currentMerchant) | ||
|
|
||
| let action = AppSettingsAction.setHasPOSBeenOpenedAtLeastOnce { _ in } |
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.
Why do we set POSBeenOpenedAtLeastOnce flag when opening the app, and not when opening the POS?
With this change, we can get a notification by opening the app twice.
As I understand, the desired behavior is to: show the notification after opening the app if POS was opened at least once
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.
You're so right, I should've paid more attention 🙇. I've updated the logic on 3571be9
- The 1st time they open the app (or after reset) when we perform the check for
.currentMerchantthis won't be scheduled as won't pass the guard forhasOpenedPOSAtLeastOnce - The merchant opens POS, which dispatches
AppSettingsAction.setHasPOSBeenOpenedAtLeastOnce - The 2nd time they open the app, passes the guard for
hasOpenedPOSAtLeastOnceand schedules the local notification. - The 3rd time they open the app, it will not pass the guard for
isNotificationScheduled, so it won't be re-scheduled.
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.
Thank you for the fixes, @iamgabrielma! Now it is as expected 👍
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.
Works as described 👍
My one suggestion is to also trigger potential merchant notification when tapping Collect Payment, not only Create Order. I think those IPP users are even more potential than those who just create an order.
I agree, this was one of the first suggestions but we ended going with order creation (we still haven't got product feedback on the project). I'll raise it again as soon as we discuss next steps 👍 |

Closes WOOMOB-1528
Closes WOOMOB-1521
Description
This PR updates the trigger for the "current merchant" case, so it schedules it when the app is opened, rather than when POS is opened.
Changes
POSTabCoordinatortoAppCoordinatorPOSNotificationSchedulingintoEditableOrderViewModelTesting information
POSNotificationScheduler.timeIntervalInSecondsto something like 5 secondsAppCoordinator.schedulePOSSurveyNotificationIfNeededadd the following bit to clear any persisted stateprivate extension AppCoordinator { func schedulePOSSurveyNotificationIfNeeded() { + Task { @MainActor in + let action = AppSettingsAction.resetPOSSurveyNotificationScheduled { _ in } + stores.dispatch(action) + } Task { @MainActor in await POSNotificationScheduler(stores: stores).scheduleLocalNotificationIfEligible(for: .currentMerchant) let action = AppSettingsAction.setHasPOSBeenOpenedAtLeastOnce { _ in } stores.dispatch(action) } } }AppCoordinator.schedulePOSSurveyNotificationIfNeeded