Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Issue #10363 - Handle authentication of the credit card selection for a prompt request #10369

Merged
merged 2 commits into from
May 28, 2021
Merged

Issue #10363 - Handle authentication of the credit card selection for a prompt request #10369

merged 2 commits into from
May 28, 2021

Conversation

gabrielluong
Copy link
Member

Fixes #10363

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@codecov
Copy link

codecov bot commented May 28, 2021 •
edited
Loading

Codecov Report

Merging #10369 (3f2f043) into master (75c6af6) will decrease coverage by 2.51%.
The diff coverage is 73.68%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #10369      +/-   ##
============================================
- Coverage     74.24%   71.72%   -2.52%     
+ Complexity     6315     1045    -5270     
============================================
  Files           843      184     -659     
  Lines         32125     5446   -26679     
  Branches       5330      962    -4368     
============================================
- Hits          23850     3906   -19944     
+ Misses         5551     1111    -4440     
+ Partials       2724      429    -2295
Impacted Files Coverage Δ
...ozilla/components/feature/prompts/PromptFeature.kt 77.98% <72.00%> (-0.40%) ⬇️
...nts/feature/prompts/creditcard/CreditCardPicker.kt 71.87% <76.92%> (+5.20%) ⬆️
...nents/feature/downloads/ui/DownloaderAppAdapter.kt
...onents/support/migration/FennecSessionMigration.kt
...illa/components/feature/search/ext/SearchEngine.kt
...notification/AbstractPrivateNotificationService.kt
...la/components/service/pocket/api/PocketResponse.kt
...a/components/feature/top/sites/TopSitesUseCases.kt
...n/java/mozilla/components/concept/fetch/Headers.kt
...n/java/mozilla/components/lib/crash/db/CrashDao.kt
... and 651 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 75c6af6...3f2f043. Read the comment docs.

@Amejia481 Amejia481 self-requested a review May 28, 2021 12:57
@gabrielluong gabrielluong force-pushed the 10363 branch 3 times, most recently from 5a4c8a9 to 3875fa2 Compare May 28, 2021 18:39
@gabrielluong gabrielluong marked this pull request as ready for review May 28, 2021 18:41
@gabrielluong gabrielluong changed the title Issue #10363 - Confirm credit card selection on a successful authentication Issue #10363 - Handle authentication of the credit card selection for a prompt request May 28, 2021
Amejia481
Amejia481 reviewed May 28, 2021
View reviewed changes
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

It looks pretty good, awesome work 🏅
I added some comments, I will like to do some manual tests as sanity checks, I will report after.

components/feature/prompts/src/main/java/mozilla/components/feature/prompts/PromptFeature.kt Show resolved Hide resolved
.../prompts/src/test/java/mozilla/components/feature/prompts/creditcard/CreditCardPickerTest.kt Show resolved Hide resolved
.../prompts/src/test/java/mozilla/components/feature/prompts/creditcard/CreditCardPickerTest.kt Outdated
}

@Test
fun `GIVEN a selected credit card WHEN onAuthFailuree is called THEN the prompt request is dismissed`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: onAuthFailure

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also confirm the prompt request was consume and dismissed? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we already check with assertTrue(onDismissCalled) below

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 the clarification, as this value is shared for multiple tests, it could be reset it on setup() 👍🏽 .
Maybe making it default to null could be a good idea.

.../prompts/src/test/java/mozilla/components/feature/prompts/creditcard/CreditCardPickerTest.kt
@@ -105,6 +112,32 @@ class CreditCardPickerTest {
verify(creditCardSelectBar).showPrompt(promptRequest.creditCards)
}

@Test
fun `GIVEN a selected credit card WHEN onAuthSuccess is called THEN the confirmed credit card is received`() {
assertNull(creditCardPicker.selectedCreditCard)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also confirm the prompt request was consume and confirmed? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

We do that by assertEquals(creditCard, confirmedCreditCard) check below

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 the clarification, as this value is shared for multiple tests, could be reset it on setup() 👍🏽

@Amejia481
Copy link
Contributor

Additionally, it will be good to add this new cool feature to the changelog file.

@gabrielluong gabrielluong force-pushed the 10363 branch 2 times, most recently from 3dd4c3c to 88e4658 Compare May 28, 2021 19:54
jonalmeida
jonalmeida approved these changes May 28, 2021
View reviewed changes
components/feature/prompts/src/main/java/mozilla/components/feature/prompts/PromptFeature.kt
* @param isAuthenticated True if the user is authenticated successfully from the biometric
* authentication prompt or false otherwise.
*/
fun onBiometricResult(isAuthenticated: Boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit on naming: we're sort of lock ourselves into the idea that we always want biometrics for confirmation.

This isn't worth fixing at this point though.

@gabrielluong @Amejia481
Issue mozilla-mobile#10363 - Handle authentication of the credit card…
304e7dd 
… selection for a prompt request

Co-authored-by: Arturo Mejia <arturomejiamarmol@gmail.com>
@gabrielluong gabrielluong added the 🛬 needs landing PRs that are ready to land label May 28, 2021
@mergify mergify bot merged commit cf8f882 into mozilla-mobile:master May 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonalmeida jonalmeida jonalmeida approved these changes

@Amejia481 Amejia481 Amejia481 approved these changes

Assignees

@Amejia481 Amejia481

@jonalmeida jonalmeida

Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle authentication of the credit card selection for a prompt request
3 participants
@gabrielluong @Amejia481 @jonalmeida