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-569 : Migrate Signin/Login related classes to kotlin #1651

Merged
merged 12 commits into from Jul 7, 2022

Conversation

hadia
Copy link
Contributor

@hadia hadia commented Jun 30, 2022

📲 What

Migrate classes from java to kotlin

  1. LoginReason
  2. LoginToutViewModel
  3. SignupViewModel
  4. TwoFactorViewModel
  5. LoginPopupMenu

🤔 Why

Migrate All classes to java.

📋 QA

test Message Sign/Sigup 2FA

Story 📖

https://kickstarter.atlassian.net/browse/NTV-569

@hadia hadia requested a review from Arkariang June 30, 2022 20:32
@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Merging #1651 (ead5219) into master (e09c5d2) will decrease coverage by 0.06%.
The diff coverage is 86.62%.

@@             Coverage Diff              @@
##             master    #1651      +/-   ##
============================================
- Coverage     78.40%   78.33%   -0.07%     
+ Complexity     1864     1863       -1     
============================================
  Files           353      353              
  Lines         16728    16739      +11     
  Branches       2085     2098      +13     
============================================
- Hits          13115    13113       -2     
  Misses         2408     2408              
- Partials       1205     1218      +13     
Impacted Files Coverage Δ
...a/com/kickstarter/viewmodels/LoginToutViewModel.kt 75.43% <75.43%> (ø)
...c/main/java/com/kickstarter/ui/data/LoginReason.kt 81.81% <81.81%> (ø)
...a/com/kickstarter/viewmodels/TwoFactorViewModel.kt 93.51% <93.51%> (ø)
...java/com/kickstarter/viewmodels/SignupViewModel.kt 93.82% <93.82%> (ø)
...pp/src/main/java/com/kickstarter/models/Message.kt 79.16% <0.00%> (-2.09%) ⬇️
...src/main/java/com/kickstarter/models/StoredCard.kt 94.91% <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 e09c5d2...ead5219. Read the comment docs.

@hadia hadia marked this pull request as ready for review June 30, 2022 22:17
@hadia hadia requested a review from leighdouglas June 30, 2022 22:18
@hadia hadia added the migration Migration Java to Kotlin label Jun 30, 2022

private fun clearFacebookSession(e: FacebookException) {
LoginManager.getInstance().logOut()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticed there are a bunch of codecov warnings here, should these be covered by tests? Is this a codecov error related to the other issues we've been having with it?

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 am not sure how to test Facebook callbacks @Arkariang any suggestions

Copy link
Contributor

Choose a reason for hiding this comment

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

For being able to properly test those callbacks we should've created a wrapper client around FacebookSDK (Dependency Inversion Principle, as we do for Apollo, Retrofit, or Braze -> DIP, that implementation it's out of the scope of this task for sure.
FYI I think there were some conversations around deprecating Facebook as login method, but nothing conclusive yet, so it's ok for now to not have coverage on those areas yet.

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.

Left a comment about code coverage on one particular view model, otherwise tested on staging and prod and everything is working as it should! 🚀

@hadia hadia merged commit 0a7855a into master Jul 7, 2022
@hadia hadia deleted the hadia/NTV-569_Migrate_Signin_Login branch July 7, 2022 17:10
Arkariang added a commit that referenced this pull request Jul 7, 2022
…into imartin/NTV-570

* 'imartin/NTV-570' of github.com:kickstarter/android-oss:
  NTV-569 :  Migrate Signin/Login related classes to kotlin (#1651)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration Migration Java to Kotlin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants