For #18265 - [Add card] Integrate the credit card storage and handle adding a new credit card

[gabrielluong]

Fixes #18265

Pull Request checklist

• Tests: This PR includes thorough tests or an explanation of why it does not
• Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

1. click on Show All Checks,
2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
3. click on the "Fenix - assemble" task, then click "Run Artifacts".
4. the APK links should be on the left side of the screen, named for each CPU architecture

[codecov-io]

Codecov Report

Merging #18719 (1e93b03) into master (c7cb0fd) will decrease coverage by 0.02%.
The diff coverage is 12.12%.

@@             Coverage Diff              @@
##             master   #18719      +/-   ##
============================================
- Coverage     33.43%   33.40%   -0.03%     
  Complexity     1431     1431              
============================================
  Files           499      501       +2     
  Lines         20526    20555      +29     
  Branches       3099     3102       +3     
============================================
+ Hits           6863     6867       +4     
- Misses        12816    12838      +22     
- Partials        847      850       +3     

Impacted Files	Coverage Δ	Complexity Δ
...x/settings/creditcards/CreditCardEditorFragment.kt	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
...editcards/controller/CreditCardEditorController.kt	33.33% <33.33%> (ø)	0.00 <0.00> (?)
...editcards/interactor/CreditCardEditorInteractor.kt	100.00% <100.00%> (ø)	0.00 <0.00> (?)

---

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 c7cb0fd...1e93b03. Read the comment docs.

[Mugurell]

This looks okay but something that caught my eye is the use of an Interactor & Controller that indicates we'd want to have this new feature follow our MVI architecture but here there's no MVI loop, no standalone View used by the Fragment so this two classes seem superfluous to me.

If not following the MVI, the Fragment could contain all the business logic and the methods needed to update the UI or maybe it could have just one other class that it could delegate much of the business logic to but at least the Interactor here seems not needed.

[gabrielluong]

This looks okay but something that caught my eye is the use of an Interactor & Controller that indicates we'd want to have this new feature follow our MVI architecture but here there's no MVI loop, no standalone View used by the Fragment so this two classes seem superfluous to me.

If not following the MVI, the Fragment could contain all the business logic and the methods needed to update the UI or maybe it could have just one other class that it could delegate much of the business logic to but at least the Interactor here seems not needed.

Thanks for feedback regarding the MVI architecture. On first look, it made sense to me to just follow our current practises. I removed the Interactor for the time being, and kept the Controller to delegate the business logic for time and to ensure it's easily testable. We can revisit the need for having the Interactor as the fragment becomes more complex and whether or not we should delegate more code into a View class.

[Mugurell]

Looks good!
Thank you!

[Mugurell]

Thank you @gabrielluong for making this changes.
I'd say this looks cleaner. Indeed Interactors were planned to being phased out since they already had very little real responsability - https://github.com/mozilla-mobile/fenix/blob/master/docs/architecture-overview.md#interactor .