-
Notifications
You must be signed in to change notification settings - Fork 121
[IPPInAppFeedback] Banner business logic and merchant eligibility in Orders screen #8549
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
You can test the changes from this Pull Request by:
|
...lasses/ViewRelated/Dashboard/Settings/In-Person Payments/InPersonPaymentsMenuViewModel.swift
Outdated
Show resolved
Hide resolved
...lasses/ViewRelated/Dashboard/Settings/In-Person Payments/InPersonPaymentsMenuViewModel.swift
Outdated
Show resolved
Hide resolved
Previously we considered IPP orders just based on their `paymetMethodID` or `paymentMethodTitle`, but this would not differentiate between IPP orders processed through the website vs the app. In order to have a more reliable solution, we add a second check to the Order `meta_data`, as IPP Orders processed through the app should have a `receipt_url` field, unlike their website counterpart.
|
Thanks for the pairing session yesterday @joshheald , this PR is ready for review 🙇 As discussed, in fe967c7 I've added an extra check to be sure that the IPP transactions we're getting are coming from the app, and not the web, by checking if their metadata contains a I couldn't find an easy way to do so directly via the resultsController predicate, so I opted for filtering the fetched objects once we get back the results from Storage. |
|
@iamgabrielma Initial testing: all of my IPP stores say this – However... some of those stores are full of recent IPP transactions, and all have at least some. I pulled to refresh and it says the same thing. We can pair on why it's doing that if you like? |
|
Hmm. I had to launch it again to hit a breakpoint (because I wanted to hit the one in So that's correct, there are exactly 10 paid IPP transactions in the last 30 days. I think the fix is probably to refresh the results on viewWillAppear, or when we fetch orders, or something like that. |
joshheald
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.
Second time round it worked for me: I suspect there's something about the first time run for each site which means it's not quite working right. Worth investigating but can be done as follow on work if you want to merge this – it's feature flagged.
Consider adding tests for this logic too: it could help you work faster on it, even though this is a temporary addition to the app.
|
Thanks for the review!
That's interesting, from these 2 lines would seem that not only failed to fetch IPP orders but also to sync stored Orders settings: I've opened a new issue here to investigate this further, happy to pair at any time 🙇
|
Part of #8530
Description
This PR adds the logic to discern which in-app feedback banner we should show to the merchant, based on if the Store is eligible ( has COD enabled, and the country is IPP-supported ), and three conditions:
At the moment we're not displaying or populating any specific content when any of the 3 conditions occur, this will be dealt with on a different PR.
Changes
displayIPPFeedbackBannerIfEligible()inOrderListViewController's init, which will perform the logic to check if COD is enabled, and fetch IPP transactions from storage (if any) as well as those within 30 days.Testing instructions
Due to the nature of fetching orders from Storage and how we use this layer as a sort of caching mechanism rather than as a source of truth, in some cases you may encounter 0 IPP transactions despite that these exist, for example if you install the app and go directly to the payments view without visiting the Orders view first. This is "expected" at this time, as we're looking for a fast solution to get us 90% there. The solution is to go to Orders > wait until are fetched > go back to Payments.One potential way to smooth this problem is to add pull-to-refresh, this can be worked on a different PR or in i2 ( tracked here )Edit: As new requirements have changed the logic from Payments to Orders, this may no longer be a problem.