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.

Sign up for GitHub

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

Jump to bottom

Fix #3134: Admin Settings shows error message for null pin entries. #3181

Merged
merged 12 commits into from
Aug 6, 2021
Merged

Fix #3134: Admin Settings shows error message for null pin entries. #3181

merged 12 commits into from
Aug 6, 2021

Conversation

Arnold2381
Copy link
Contributor

@Arnold2381 Arnold2381 commented May 11, 2021 •
edited

Explanation

Fixes #3134

WhatsApp.Video.2021-05-11.at.11.59.59.mp4

Screenshot of all the tests passing on espresso
image

image

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@Arnold2381 Arnold2381 requested a review from rt4914 as a code owner May 11, 2021 06:41
@anandwana001
Copy link
Contributor

anandwana001 commented May 11, 2021 •
edited

Write test case too.

https://github.com/oppia/oppia-android/wiki/Oppia-Android-Testing

@Arnold2381
Copy link
Contributor Author

Arnold2381 commented May 11, 2021 •
edited

@anandwana001 I have a question, in the Administrator Controls, the submit button only gets activated when the user enters a pin of length 5 but in this PR in the video if you see when a user is going to Administrator controls even if the submit button isn't activated, he can use his keyboard submit button and if he does that with an incorrect pin of whatever length less than 5 an error is shown, so I did the same for null string but this cant be tested. So is it okay if I don't include tests for this?

@rt4914 rt4914 assigned anandwana001 and unassigned Arnold2381 May 12, 2021
@anandwana001
Copy link
Contributor

@Arnold2381 Can we disable the keyboard from submitting? this makes out the keyboard in sync with the submit button state.

@Arnold2381
Copy link
Contributor Author

@Arnold2381 Can we disable the keyboard from submitting? this makes out the keyboard in sync with the submit button state.

I did figure out a way to test it. But yes we can remove it, and if I do that, I will be removing the tests for that.

@Arnold2381 Arnold2381 removed their assignment May 14, 2021
@Arnold2381
Copy link
Contributor Author

@anandwana001 Also PTAL in this one.

@rt4914 rt4914 self-assigned this May 17, 2021
anandwana001
anandwana001 suggested changes May 19, 2021
View reviewed changes
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

Thanks @Arnold2381

left a few thoughts, will check test file in next iteration.

app/src/main/java/org/oppia/android/app/profile/AdminAuthActivityPresenter.kt Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profile/AdminAuthActivityTest.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned mschanteltc and unassigned rt4914 May 20, 2021
@Arnold2381
Copy link
Contributor Author

Arnold2381 commented May 26, 2021 •
edited

@anandwana001 Sorry for the late reply, I was busy with my college exams and everything.
I have made the keyboard button in sync with the submit button. And also user can use the keyboard submit button only when he enters a pin of length equal to the admin pin. And also because we are removing the functionality of not allowing the user to press the submit button before the admin pin's length, I don't think we need the tests for that so I removed the two tests for the same.
PTAL

@Arnold2381 Arnold2381 removed their assignment May 29, 2021
@anandwana001
Copy link
Contributor

anandwana001 commented Jun 29, 2021 •
edited

Hi @Arnold2381

Looks like we lost track of this PR, are you still working on this, and could you update on where are we on this PR. Also, whenever you need a review of any member, please comment the persons' GitHub handle and add PTAL, our Oppiabot will assign that person to the assignee section so that the PR doesn't go off the track.

@Arnold2381
Copy link
Contributor Author

Arnold2381 commented Jun 30, 2021 •
edited

Hello @anandwana001
Sorry! I thought replying to your reviews would notify you, next time I would do the same. .

Yea so our discussion was on the tests I removed. I suggest there is no need of the tests and I removed it because the cases its testing for will never arise. As you will see when will enter a pin less then the length of the adminPin the button doesn't gets activated so I don't think it will ever show an error. The tests were required before because the keyboard submit button was allowing the user to press the submit button before the length of the admin pin entered but now, the keyboard submit button will have no action until and unless the pin length is same as admin and once same it acts same as the normal submit button.
So i think whatever is done till now is working.

@Arnold2381 Arnold2381 removed their assignment Jun 30, 2021
@Arnold2381
Copy link
Contributor Author

Arnold2381 commented Jun 30, 2021 •
edited

@anandwana001 PTAL

@rt4914
Copy link
Contributor

rt4914 commented Jul 11, 2021

@anandwana001 PTAL at this PR.

@anandwana001
Copy link
Contributor

I will take a look at this tonight.

@anandwana001 anandwana001 added the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Jul 15, 2021
anandwana001
anandwana001 suggested changes Jul 15, 2021
View reviewed changes
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

Tested it, seems working for me.

Nit queries regarding test cases, else LGTM.

app/src/sharedTest/java/org/oppia/android/app/profile/AdminAuthActivityTest.kt Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profile/AdminAuthActivityTest.kt Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt Show resolved Hide resolved
@Arnold2381
Copy link
Contributor Author

@anandwana001 PTAL.

@Arnold2381 Arnold2381 removed their assignment Jul 25, 2021
rt4914
rt4914 approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

Nit changes suggested. Else looks good.

Merge with latest develop and wait for @anandwana001 approval.

app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 removed their assignment Aug 4, 2021
@Arnold2381
Copy link
Contributor Author

@rt4914 done!

rt4914
rt4914 approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

anandwana001
anandwana001 approved these changes Aug 6, 2021
View reviewed changes
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

LGTM

@anandwana001 anandwana001 merged commit 1bf6bbc into oppia:develop Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mschanteltc mschanteltc mschanteltc left review comments

@rt4914 rt4914 rt4914 approved these changes

@anandwana001 anandwana001 anandwana001 approved these changes

Assignees

@anandwana001 anandwana001

Labels
PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AdminSettingDialogFragment doesn’t show error message when the input is empty
4 participants
@Arnold2381 @anandwana001 @rt4914 @mschanteltc