-
Notifications
You must be signed in to change notification settings - Fork 116
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
Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999
Merged
Merged
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
19def60
update: `feeLines` property to return `order.fees` instead, while rem…
pmusolino 4026840
Add `shippingLabels` property to `Order` which was connected in Stora…
pmusolino ec15b19
Merge commit 'f12dbd8aeff195fe6da618d23550f9d8d6d24b7f' into feat/bat…
pmusolino 0311bd9
update: relese-notes
pmusolino 0ef427b
update: add PR link to release-notes
pmusolino 22d0a31
Merge branch 'trunk' into feat/batch-fetching-in-order-details
pmusolino e2d583f
Merge branch 'trunk' into feat/batch-fetching-in-order-details
pmusolino bfc3695
Merge branch 'trunk' into feat/batch-fetching-in-order-details
pmusolino 85c0115
Update Order.swift and RELEASE-NOTES.txt
pmusolino 8d6cf40
Merge branch 'trunk' into feat/batch-fetching-in-order-details
pmusolino e03de8d
Merge commit '7627219dad37276b64378daca7a1258fe46f90be' into feat/bat…
pmusolino 3577381
fix: correctly update shipping labels in an order if the following se…
pmusolino f84f26e
Merge commit 'd231feff7094c39c56123f4fd2a065bcf459ca6f' into feat/bat…
pmusolino bc264a9
Merge commit 'd231feff7094c39c56123f4fd2a065bcf459ca6f' into feat/bat…
pmusolino 85530e1
Merge branch 'feat/batch-fetching-in-order-details-take-2' into feat/…
pmusolino 58fdc9b
Merge branch 'trunk' into feat/batch-fetching-in-order-details
pmusolino e6278c3
update: release-notes
pmusolino 32dbb12
Fix issue triggering order syncing after dismissing modals
itsmeichigo 5bc56ac
Fix unit test build failures
itsmeichigo bbfb18c
Improve syncing of shipping label details
itsmeichigo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
👋 @pmusolino Do we receive this
shipping_labels
key in orders response? Based on my testing in a Woo store with shipping labels, I don't see it being sent inwc/v3/orders
.If we do receive the
shipping_labels
, should we update the mock JSON and add unit tests to ensure thatshipping_labels
are mapped correctly?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.
Good question, @selanthiraiyan. In
ShippingLabelStore
, the methodupsertShippingLabels
connects the fetched shipping labels to the specific order, so the order doesn't return specific shipping labels, making it unnecessary to decode the Shipping Labels in the Order model. However, declaring the property (without decoding) in the networking model seems necessary to accessshippingLabels
, unless you have another solution in mind.Yes, we could access the shipping labels from storage, but that's what we're trying to avoid. Do you think I can leave the
shippingLabels
property inOrder.swift
but remove the decoding? This way, by default, it would be an empty array.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, @pmusolino! I don't have a solution in mind. But I think we should remove the decoding, as it will be misleading.
I'm tagging @itsmeichigo to check whether she has any other solutions.
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 ping Sharma!
I think if the key for shipping labels is non-existent in the order response, keeping the property an empty array should be the way to go. There's no need to waste more resources to look for such a key to decode. Please also leave a comment in the code to explain why the value is empty by default, and when it's going to be updated.
However, I'm concerned about this part:
In the order details screen, after syncing shipping labels, we'd need to ensure that the
order
in the view model is updated with the fetched label(s), just so they can be displayed on the UI.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 both! I updated the decoding here 85c0115 the comment should be clear enough.
Yes, this already happens from my testing. If you create or update a shipping label, or open an order detail with an existing shipping label that needs to be fetched remotely, you will see the order detail UI correctly updated.