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

MBL-1893 && MBL-1877: Separate rewards from project query #2173

Merged
merged 14 commits into from
Nov 26, 2024

Conversation

Arkariang
Copy link
Contributor

@Arkariang Arkariang commented Nov 19, 2024

📲 What

  • Project query obtains now very minimum information about rewards,(ideally in the future will not query anything around rewards, but for now some details to pre-load quickly the rewards carousel is required).
  • New query that will fetch the rewards + simple shipping rules + reward Items has being created, this new query is called within RewardsSelectionViewModel.
  • Crash fixed related to changing the selected location for a pledge.

🤔 Why

Project query has modified to obtain all simple shipping rules, for all rewards, (Globally shipping rewards then will hold a collection of 246 shipping rules each). Project (as any other data model within the app) can be parzelized to be serialized/deserialized from Intents, but unfortunately we have hit again the transactionTooLarge exception due to Project being too big (we need to find other ways to persist information within the app, passing the hole object from screen to screen via intent to avoid extra network calls has clear limitations).

👀 See

  • Filtering rewards keeps working as usual, the restricted reward ships only to canada, if changing location to other countries will not be displayed on the carousel
filteringRewards.mp4
  • Selecting new location and new addons crash has been fixed
modifiedRewardShippingAndAddOn.mp4
  • Change reward selected to "No Reward", the blue pill is now showing for "No rewards" was not before
ChangerewardToNoRewardAndBluePill.mp4
  • Changing payment method keeps working as usual
changePaymentMethod.mp4
  • Change reward selection from Digital reward to reward with shipping
chooseAnotherRewardFromDigitalToShipping.mp4

| | |

📋 QA

Story 📖

MBL-1877
MBL-1893

- In RewardsSelectionViewModel we fetch all rewards again with simpleShippingRules this time, but this information is not moved via intent to any other screen.
…f github.com:kickstarter/android-oss into imartin/separate-rewards-from-project
…wardsSelectionVM

- Removed from project query the rewards, only added the reward no reward with minimum pledge for a project.
- When updating/managing a reward the information for reward/addons was collected from backing, therefore shippingRules was not available within baking that nested object.
- Use as default the fresly queried information, and update with the backed information rather that the other way around.
…mation to who "change payment method"/ "Fix Pledge" do require the rewards information for the project
…rd", but always for fixing changing payment method AND choosing another reward it will show "Manage pledge"
@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 68.88889% with 14 lines in your changes missing coverage. Please review.

Project coverage is 68.57%. Comparing base (952fef2) to head (ffdffbb).

Files with missing lines Patch % Lines
...iewmodels/projectpage/RewardsSelectionViewModel.kt 78.94% 1 Missing and 3 partials ⚠️
...kstarter/viewmodels/projectpage/AddOnsViewModel.kt 57.14% 0 Missing and 3 partials ⚠️
...ter/viewmodels/usecases/GetShippingRulesUseCase.kt 78.57% 0 Missing and 3 partials ⚠️
...arter/services/transformers/GraphQLTransformers.kt 0.00% 2 Missing ⚠️
...kickstarter/libs/utils/extensions/PledgeDataExt.kt 0.00% 0 Missing and 1 partial ⚠️
...ewmodels/projectpage/CrowdfundCheckoutViewModel.kt 50.00% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2173      +/-   ##
============================================
+ Coverage     68.55%   68.57%   +0.01%     
  Complexity     2164     2164              
============================================
  Files           347      347              
  Lines         23015    23018       +3     
  Branches       3357     3354       -3     
============================================
+ Hits          15778    15784       +6     
+ Misses         5436     5434       -2     
+ Partials       1801     1800       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -28,7 +28,7 @@ fun PledgeData.shippingCostIfShipping(): Double {
var addOnsShippingCost = 0.0
this.addOns()?.map {
if (RewardUtils.shipsWorldwide(it) || RewardUtils.shipsToRestrictedLocations(it)) {
addOnsShippingCost += (it.shippingRules()?.first()?.cost() ?: 0.0) * (it.quantity() ?: 0)
addOnsShippingCost += (it.shippingRules()?.firstOrNull()?.cost() ?: 0.0) * (it.quantity() ?: 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The crash was due to using the reward and addOns from the backing object, where shipping locations information is not available

@@ -220,7 +220,8 @@ fun RewardCarouselScreen(
id = R.string.Back_it_because_you_believe_in_it
),
onRewardSelectClicked = { onRewardSelected(reward) },
isCTAButtonVisible = project.isAllowedToPledge()
isCTAButtonVisible = project.isAllowedToPledge(),
yourSelectionIsVisible = project.backing()?.isBacked(reward) ?: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blue pill was not visible when the baked reward is a "Reward no reward"

val aux = holder[backedAddOn.id()]
if (aux != null) {
val updated = aux.toBuilder().quantity(backedAddOn.quantity()).build()
holder[backedAddOn.id()] = updated
Copy link
Contributor Author

@Arkariang Arkariang Nov 19, 2024

Choose a reason for hiding this comment

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

before we were using the information from the baked object, now we update the "quantity" field alone to not loose the "shipping rules" field

when (flowContext) {
PledgeFlowContext.NEW_PLEDGE,
PledgeFlowContext.CHANGE_REWARD -> getPledgeInfoFrom(pData)
PledgeFlowContext.MANAGE_REWARD,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Manage_Reward" is used for both updating selected reward and changing reward, it describes the fact of having to present to the user RewardsCarousel not granular enough.
Exchanged to use PledgeReason instead of PledgeFlowContext as it's uniquely identified.

@Arkariang Arkariang marked this pull request as ready for review November 19, 2024 19:47
@Arkariang Arkariang changed the title Imartin/separate rewards from project MBL-1893 && MBL-1877: Separate rewards from project query Nov 19, 2024
@Arkariang Arkariang self-assigned this Nov 19, 2024
// - API does not provide the Reward no reward, we need to add it first
val minPledge = data.project()?.minPledge()?.toDouble() ?: 1.0
val modifiedRewards = rwList.toMutableList()
modifiedRewards.add(0, RewardFactory.noReward().toBuilder().minimum(minPledge).build())
Copy link
Contributor Author

@Arkariang Arkariang Nov 19, 2024

Choose a reason for hiding this comment

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

Adding reward no reward to the list of rewards. It is not provided by the API, but the minimum value to be pledged is configured in a project level.

@@ -150,7 +150,6 @@ class AddOnsViewModel(val environment: Environment, bundle: Bundle? = null) : Vi
bonusAmount = b.amount()
} else {
backedAddOns = b.addOns() ?: emptyList()
currentUserReward = b.reward() ?: currentUserReward
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was causing the issue mentioned here

Copy link
Contributor

@leighdouglas leighdouglas left a comment

Choose a reason for hiding this comment

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

LGTM!!!!

@Arkariang Arkariang merged commit 9975252 into master Nov 26, 2024
3 checks passed
@Arkariang Arkariang deleted the imartin/separate-rewards-from-project branch November 26, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants