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

[WEB-696] CTA Dialog on Facebook Login Error (FB Deprecation) #1763

Merged
merged 8 commits into from Dec 13, 2022
Merged

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Dec 7, 2022

📲 What

As a User who does not have a Kickstarter password and attempts to log in via Facebook, but gets an error either from Facebook, we should present a CTA dialog on the login screen with options to set a new password or use normal email password login

See the designs for details on what this should look like.

Since we're still looking into the issue we're seeing around snapshot testing on M1 macbooks, I've created a ticket to get that part of this PR in once, Mubarak is back

🤔 Why

In the past FB has turned off their FB Login API without warning keeping kickstarters from logging in.
In an effort to deprecate FB Login altogether, this covers that scenario. Both informing the kickstarter that we need them to set a password or log in through our normal login flow.

This will allow us to ensure that everyone is more aware of the move away from FB Login as well as give everyone a chance to set a password on their accounts if they don't already have one.

🛠 How

  • Created a new ResetYourFacebookPassword screen. Allows a kickstarter to get an email to set a password on their account. This is essentially the normal reset password flow.
  • Created a new CTA popup with 2 options. Either navigate to normal log in or to the new screen.
  • Using the showFacebookErrorAlert listener in LoginToutViewController, we can:
    1. Check to make sure that Facebook Login Deprecation flag is enabled
    2. Present this new CTA at the right time

👀 See

Trello, screenshots, external resources?

Simulator Screen Recording - iPhone 13 Pro - 2022-12-07 at 12 12 07

✅ Acceptance criteria

  • If an error occurs when attempting to login with FB, and the FF is on, this new CTA shows
  • The actions on the new CTA navigate to the correct screens
  • The submit button on ResetYourFacebookPassword screen sends an email to the email, if valid, allowing you to set a password on your account.

@scottkicks scottkicks added this to the release-5.7.0 milestone Dec 7, 2022
@scottkicks scottkicks self-assigned this Dec 7, 2022
@nativeksr
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #1763 (49e5ef6) into main (b936c69) will decrease coverage by 0.12%.
The diff coverage is 39.93%.

@@            Coverage Diff             @@
##             main    #1763      +/-   ##
==========================================
- Coverage   85.42%   85.29%   -0.13%     
==========================================
  Files        1273     1276       +3     
  Lines      116209   116541     +332     
  Branches    30637    30722      +85     
==========================================
+ Hits        99273    99408     +135     
- Misses      15869    16066     +197     
  Partials     1067     1067              
Impacted Files Coverage Δ
...LoginTout/Controller/LoginToutViewController.swift 53.50% <0.00%> (-2.67%) ⬇️
...ller/ResetYourFacebookPasswordViewController.swift 0.00% <0.00%> (ø)
Library/UIAlertController.swift 0.73% <0.00%> (-0.07%) ⬇️
...iewModels/ResetYourFacebookPasswordViewModel.swift 96.29% <96.29%> (ø)
...dels/ResetYourFacebookPasswordViewModelTests.swift 100.00% <100.00%> (ø)
Library/Navigation.swift 85.85% <0.00%> (+0.64%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@scottkicks scottkicks marked this pull request as ready for review December 13, 2022 15:34
@scottkicks scottkicks merged commit cd6fe2b into main Dec 13, 2022
@scottkicks scottkicks deleted the web-696 branch December 13, 2022 15:34
Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Hey Scott, good job here! Works well, code is pretty clean.
I did a deeper review of this and found some improvements to make.

I didn't write it as a comment but can you also remove all the recorded screenshots here. I've made a note to record these in the fix screenshots pr so they are platform agnostic.

There's something else too which I know can be a chore, but would improve the accessibility and usability of the app for larger font sizes. It would also be more consistent with SignUpViewController and other form fields.

The two things are:

  1. Ensure the scrollview is not hidden by the keyboard when its' up. Context for how to achieve this is SignupViewController animateTextViewConstraint function.
  2. Also the more complicated thing but does make sense with both ResetYourFacebookPasswordViewController and SetYourPasswordViewController is to allow each textfield to move to the next and then hook up the done button to the api call on the submit button. Examples of how to do this are in SignupViewController signals like emailTextFieldReturn, nameTextFieldReturn and signupButtonPressed.

Current behaviour:

Simulator.Screen.Recording.-.iPhone.8.-.2023-01-04.at.17.11.18.mp4

In-app conforming behaviour:

Simulator.Screen.Recording.-.iPhone.8.-.2023-01-04.at.17.26.10.mp4

Those last two are big asks, so if you want me to make the changes let me know.

Maybe you can write all this up in a separate "improvements" pr?

internal let setPasswordSuccess = TestObserver<String, Never>()
internal let setPasswordFailure = TestObserver<String, Never>()
internal let shouldShowActivityIndicator = TestObserver<Bool, Never>()
internal let textFieldAndSetPasswordButtonAreEnabled = TestObserver<Bool, Never>()
Copy link
Contributor

Choose a reason for hiding this comment

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

textFieldAndSetPasswordButtonAreEnabled is not being tested, please add a test for it.


func testResetFail_WithNon404Error() {
let error = ErrorEnvelope(
errorMessages: ["Zoinks!"],
Copy link
Contributor

Choose a reason for hiding this comment

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

haha nice

@msadoon msadoon modified the milestones: release-5.7.0, release-5.6.1 Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants