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

feat: enable quiz completion without reward #3421

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

UncleSamtoshi
Copy link
Contributor

@UncleSamtoshi UncleSamtoshi commented Oct 25, 2023

  • add quizRewardsEnabled to ConsumerAccount gql object
  • enable marking a quiz question complete without payment success
  • refactor PartialReturn type

@github-actions github-actions bot added the core label Oct 25, 2023
@UncleSamtoshi UncleSamtoshi force-pushed the enable-rewardless-earn branch from 2da4245 to 21f0dbd Compare October 25, 2023 19:38
Copy link
Contributor

@vindard vindard left a comment

Choose a reason for hiding this comment

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

One comment so far on the way we've implemented passing a result + error along

core/api/src/app/payments/add-earn.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@vindard vindard left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only comment left is, I wonder if it'll be easy to add a bats test for this

throw mapError(error)
}

if (!result?.slice) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this PR should only include changes related rewards, why is this an error?

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 just matches the existing behavior, but with different types. As you can see in line 193 if there was any error, even if it was a partial success, we still threw in line 194.

@@ -190,15 +203,10 @@ const ConsumerAccount = GT.Object<Account, GraphQLPublicContextAuth>({
paginationArgs,
})

if (error instanceof Error) {
if (!result || error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this use PartialResult then should use the new approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will switch to using type on the partial result.

@UncleSamtoshi UncleSamtoshi force-pushed the enable-rewardless-earn branch 2 times, most recently from 619eec0 to 3d1767f Compare October 26, 2023 13:38
@UncleSamtoshi
Copy link
Contributor Author

Looks good to me. Only comment left is, I wonder if it'll be easy to add a bats test for this

For testing we could add the unhappy path where the phone carrier is not supported. However I dont think we can get the local setup into a state where phone carrier and ip are valid.

@UncleSamtoshi UncleSamtoshi force-pushed the enable-rewardless-earn branch from 3d1767f to 69e4f0d Compare October 26, 2023 14:12
@UncleSamtoshi
Copy link
Contributor Author

One concern I have with this PR is we lose explicitly visibility into whether we actually paid out the reward. Previously the presence of a completed question implied that the reward was paid. However this introduces the possibility that a question is completed but a reward was not paid. Since this is a marketing feature it may not be critical to have this visibility. Additionally we could look through the ledger for a transaction correlating to that id.

In exchange for losing this visibility we now allow people without the ability to earn rewards the ability to still complete the earn section.

@nicolasburtey
Copy link
Member

nicolasburtey commented Oct 26, 2023

One concern I have with this PR is we lose explicitly visibility into whether we actually paid out the reward. Previously the presence of a completed question implied that the reward was paid. However this introduces the possibility that a question is completed but a reward was not paid. Since this is a marketing feature it may not be critical to have this visibility. Additionally we could look through the ledger for a transaction correlating to that id.

In exchange for losing this visibility we now allow people without the ability to earn rewards the ability to still complete the earn section.

we should make sure that we can, through tracing, plot the query where rewards are given or not.

@UncleSamtoshi
Copy link
Contributor Author

we should make sure that we can, through tracing, plot the query where rewards are given or not.

This should be visible.

@UncleSamtoshi UncleSamtoshi force-pushed the enable-rewardless-earn branch from 1cd755e to 0945db9 Compare October 27, 2023 01:34
@vindard
Copy link
Contributor

vindard commented Oct 27, 2023

However this introduces the possibility that a question is completed but a reward was not paid.

Should we consider adding rewardPaid to persistence as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants