-
Notifications
You must be signed in to change notification settings - Fork 520
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
Fixes #540: Updates profile admin flow when admin does not have a PIN #541
Conversation
…-profile-chooser-fragment
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.
Generally LGTM, but I didn't review in detail. Deferring to Rajat for a detailed review.
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.
All the test cases in AdminPinActivityTest
are failing because of following reason.
After typing, the keyboard is still visible and submit
button is behind the keyboard because of which perform(click())
is not working correctly.
Possible solution:
- Scroll to submit button before clicking on it.
- Hide the keyboard before clicking on submit button.
Also, I am seeing that you haven't used fragments in these, but in the entire app we are using fragments for containing everything. This applies to your PRs too. So what is the reason behind this? @jamesxu0 @BenHenning
app/src/main/java/org/oppia/app/profile/ProfileActivityPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/app/profile/AdminPinActivityTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/app/profile/ProfileChooserFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/app/profile/ProfileChooserFragmentTest.kt
Outdated
Show resolved
Hide resolved
@BenHenning PTAL. I addressed all of Rajat's comments and fixed the test case. |
app/src/main/java/org/oppia/app/profile/ProfileChooserFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
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.
Thanks @jamesxu0! Just a few lingering comments to address.
Explanation
Fixes #540: Updates profile admin flow when admin does not have a PIN. See https://xd.adobe.com/view/6384f092-e16a-4214-5a1a-f10ee0ef4f04-15b5/screen/18e69208-8528-424a-a368-b2c5d530d235/PC-SP-Profile-Chooser- for new flow. Requires admin to create a new PIN when attempting to add a new profile. Note that the base branch is PR #539.
Checklist