Skip to content
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

NTV-1435 - Migrate Shipping V1 to GQL #1691

Merged

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented May 16, 2022

πŸ“² What

Fix the missing shipping badges. Slack Thread.

"Shipping Worldwide"
"Limited Shipping"
"Only {country}"

These badges have been missing since we migrated our project model to gql. Also is connected to the data/UX for updating local receipt information.

πŸ€” Why

This investigation was needed to understand the logic between the shipping data in the reward card and the badge related to shipping. ui/ux

Also all the places where shipping address selector, shipping label got its data. Will hopefully help with QA all possible scenarios when feature complete.

πŸ›  How

gql logic for returning shipping summary information.

If the reward isn't local, above logic states shipping summary can be used to display the badge data for shipping labels. This replaces the v1 shipping_type previously used.

Left the shipping_type in the existing v1 model because it causes crashes when v1's discover endpoint is called (app launch). Didn't do a deep dive but there are still use cases for Reward model shipping type field.

Anyway, the logic above also clarifies that if the reward has localReceiptLocation then the badge for shipping data should be hidden, and instead shipping summary is used for the Reward Location field.

πŸ‘€ See

Before πŸ›

Simulator Screen Recording - iPhone 8 - 2022-05-16 at 18 43 33

After πŸ¦‹

Simulator Screen Recording - iPhone 8 - 2022-05-16 at 18 46 02

βœ… Acceptance criteria

  • Shipping badges "Anywhere in the world", "Only ships to certain countries" and "Only ships to { country }" get displayed in app.

⏰ TODO

  • Update tests.

@msadoon msadoon added the WIP label May 16, 2022
@msadoon msadoon added this to the release-5.3.0 milestone May 16, 2022
@msadoon msadoon self-assigned this May 16, 2022
@nilaratna nilaratna requested a review from a team May 16, 2022 23:09

return nil
}
if project.state == .live,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean this info is hidden when the project is not live?

Copy link
Contributor Author

@msadoon msadoon May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Bessie, yes, the project needs to be in live state to show the shipping labels.
For all the base reward cards, (add-ons are only shown if project is live) we show 2 strings when the project isn't live (backer count and has add-ons) and two more if the project is live (shipping and time left).
Confirmed its the same on Android for non-live projects.
Screen Shot 2022-05-17 at 1 44 04 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for clarifying!

@msadoon msadoon requested a review from iambchan May 17, 2022 20:36
@msadoon msadoon added needs review and removed WIP labels May 17, 2022
@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #1691 (65e1a58) into feature/local-pickup-rewards (f19cb08) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                      Coverage Diff                      @@
##           feature/local-pickup-rewards    #1691   +/-   ##
=============================================================
  Coverage                         85.45%   85.45%           
=============================================================
  Files                              1255     1255           
  Lines                            111499   111514   +15     
  Branches                          29665    29659    -6     
=============================================================
+ Hits                              95281    95295   +14     
- Misses                            15177    15179    +2     
+ Partials                           1041     1040    -1     
Impacted Files Coverage Ξ”
KsApi/ServiceType.swift 76.06% <ΓΈ> (ΓΈ)
...s/Controllers/ProjectPageViewControllerTests.swift 100.00% <100.00%> (ΓΈ)
...arter-iOS/Views/RewardCardContainerViewTests.swift 100.00% <100.00%> (ΓΈ)
...raphql/adapters/Backing+BackingFragmentTests.swift 98.57% <100.00%> (+<0.01%) ⬆️
...l/adapters/Project+FetchAddOnsQueryDataTests.swift 78.04% <100.00%> (ΓΈ)
...roject+FetchProjectRewardsByIdQueryDataTests.swift 81.63% <100.00%> (ΓΈ)
...odels/graphql/adapters/Reward+RewardFragment.swift 82.66% <100.00%> (ΓΈ)
.../graphql/adapters/Reward+RewardFragmentTests.swift 95.52% <100.00%> (+0.06%) ⬆️
...ons/templates/query/FetchAddOnsQueryTemplate.swift 98.92% <100.00%> (+<0.01%) ⬆️
...s/query/FetchProjectRewardsByIdQueryTemplate.swift 99.66% <100.00%> (+<0.01%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update f19cb08...65e1a58. Read the comment docs.

@msadoon msadoon merged commit 8d40eb9 into feature/local-pickup-rewards May 18, 2022
@msadoon msadoon deleted the ntv-1435/migrate-shipping-selector-v1-gql branch May 18, 2022 01:44
@msadoon
Copy link
Contributor Author

msadoon commented May 18, 2022

Hope its cool if I merged this before you finished the review @iambchan . If you have further questions I can reopen this pr.

Copy link
Contributor

@iambchan iambchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ₯³


return nil
}
if project.state == .live,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for clarifying!

msadoon added a commit that referenced this pull request Jun 20, 2022
* NTV-1435 - Migrate Shipping V1 to GQL (#1691)

* [NTV-1446] Reward and Addon Cards Display Shipping Location (#1693)

* [NTV-1522] Pledge Screen Local Reward Pickup UI (#1695)

* [NTV-1690] Reward Card Shipping Preference Check + Other Cleanup (#1696)

* fixed layout issue occuring on estimated delivery date and reward location stackview in manage pledge page.

* updated tests for previous tests on reward location view to include check for shipping preference.

* [PAY-1442] Available Add Ons And Shipping Costs When Local Reward Selected  (#1697)

* [PAY-1525] Local Pickup Optimizely Feature Flag (#1698)

* added local pickup optimizely flag and tests

* added filtering logic to reward selection view controller based on ff for local pickup.

* corrected existing tests

* added tests for allowable rewards on reward collection view controller and view model.

* corrected an issue where if reward was not allowed to be displayed the next reward would by default not be shown.

* quick test build correction in Library
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants