-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Remove unused code for setPassword()
action
#8680
Conversation
return; | ||
} | ||
setPassword(password, validateCode, accountID); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just removing this because I don't think it's clear to have a single method responsible for setting a password or changing one and this can be left up to the view to decide.
const login = lodashGet(response, 'data.email', null); | ||
Onyx.merge(ONYXKEYS.ACCOUNT, {accountExists: true, validateCodeExpired: true, error: null}); | ||
|
||
// The login might not be set if the user hits a url in a new session. We set it here to ensure calls to resendValidationLink() will succeed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't really get what this comment is saying even though I asked for it to be added when a contributor added this 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it cannot run... and could not run when the code was added...
Original state of file when this was added...
cc @chiragsalian on this too just to check me. I read some older comments of yours in this thread #5150 (comment) and wanted to be sure that there isn't another way that |
Umm are you asking if there is some incoming response flow that would require a catch block? If so i feel like in the past an "incorrect validate code" or "email not found" would trigger the catch block. But i just tested this and it doesn't seem to be the case as its going to
Since the request is a failure it should go to the catch block. Not sure if this is something we would want to account for though. |
@chiragsalian Oh wow, thanks for testing 🙇 That is consistent with my expectations then. Yes, I think if you are offline it's possible for the request to eventually fail and throw (but we are not really handling this case anywhere yet AFAIK). |
</View> | ||
<View> | ||
<FormAlertWithSubmitButton | ||
buttonText={buttonText} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! It's a blocker for me so I'll fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @marcaaron in version: 1.1.57-0 🚀
|
🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀
|
cc @roryabraham since you reviewed #6587 and I'm trying to reference that PR to make sure this one is correct.
Details
The code running inside this
catch()
block seems like it will never run. This particular (maybe all) API command methods will only catch if:.then()
Since the only logic performed is a call to
createTemporaryLogin()
and the throw inside that method is handled by it's owncatch()
the logic here won't ever run.Code was added in this PR and I didn't catch it (no pun intended)
Fixed Issues
$ #8678
Tests
PR Review Checklist
PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
This is not really easy to QA on native since you need to manually break the validate code. But we can repeat the steps above for:
Web
mobile web
Verify that no errors appear in the JS console
Screenshots
Web
Mobile Web
Desktop
iOS
Android