Fix #895: Add error text below password EditText in LoginActivity #913

m-sameer · 2018-10-25T17:49:57Z

Fixes #895

Please make sure these boxes are checked before submitting your pull request - thanks!

Apply the AndroidStyle.xml style template to your code in Android Studio.
Run the unit tests with ./gradlew check to make sure you didn't break anything
If you have multiple commits please combine them into one commit by squashing them.

luckyman20 · 2018-10-25T18:20:48Z

@m-sameer Fix the Travis Build Error

m-sameer · 2018-10-25T19:27:33Z

@luckyman20 Done. 😄

miPlodder · 2018-10-26T07:36:17Z

app/build.gradle

@@ -77,6 +77,9 @@ dependencies {
    implementation "com.google.android.gms:play-services-oss-licenses:$rootProject.playServicesVersion"
    implementation "com.isseiaoki:simplecropview:$rootProject.cropviewVersion"

+    // Multidex


@m-sameer Remove it. PR #909 will be merged then you can pull that code and update your PR.

miPlodder · 2018-10-26T07:42:13Z

build.gradle

        google()
+        jcenter()


Remove this too.

miPlodder · 2018-10-26T07:43:18Z

app/src/main/java/org/mifos/mobilebanking/presenters/LoginPresenter.java

@@ -178,22 +178,25 @@ private boolean isCredentialsValid(final String username, final String password)

        final Resources resources = context.getResources();
        final String correctUsername = username.replaceFirst("\\s++$", "").trim();
+        boolean usernameValid = true;


@m-sameer Check your logic, no additional variable is required and no checks for passwords are added.

@miPlodder If I don't use an additional variable for checking the username validity and return false directly the password EditText will not be checked. 😕
These are the minimal changes I can think of, can you suggest any?

@m-sameer Then, don't return at that stage. Try to improve your solution and use a TextWatcher then you wouldn't be needing any additional variables.

@miPlodder If I use a TextWatcher the credentials will be checked on every tap on the keyboard but they should be checked only when the login button is pressed. Also adding a listener will be more complicated and will add more lines than simply using a boolean variable.

@m-sameer It will give a better UX. Try to sign up/ sign in to any software, which one will you be convenient, the one which you get an instant feedback.

I know it will be more work. You don't worry about the deadline, I will extend it and don't focus on doing a high number of tasks, but focus on doing Quality tasks.

When free read this.

@m-sameer @miPlodder I have another approach which might satisfy need of both of you.

You can add a onFocus of Password EditText and then check if username is null or not.

@luckyman20 That is what I'm talking. For example, if a user is entering his username, we don't want the validation/textwatcher to work. When he changes his focus to another field, then the textwatcher should be active. Now, when user edits his username, how the textwatcher will be active and showing all the relevant errors.

See Paypal mobile interface while signing up.

@miPlodder I think we won't be needing a TextWatcher for this, we have to split the isCredentialsValid into two separate methods namely isUsernameValid and isPasswordValid and we will call isUsernameValid in onFocus of Password EditText and then call isPasswordValid in onSubmit.

@miPlodder I've added an OnFocusChangeListener which checks the credentials on focus change. Earlier I was checking the credentials on button click because the issue said Error text beneath EditText should be shown for all the EditText on clicking the Login Button.
I agree that we should strive to improve the UX and the link to the design website was helpful. :)
As you said, I'm focused on doing high-quality tasks rather than higher quantity.

@miPlodder @luckyman20 Thanks for your help.

m-sameer · 2018-10-28T09:12:45Z

@miPlodder Please review.

luckyman20 · 2018-10-28T10:06:58Z

@m-sameer Fix the travis build

m-sameer · 2018-10-28T15:30:52Z

app/src/main/java/org/mifos/mobilebanking/presenters/LoginPresenter.java

@@ -75,7 +75,7 @@ public void detachView() {
     */
    public void login(final String username, final String password) {
        checkViewAttached();
-        if (isCredentialsValid(username, password)) {
+        if (isUsernameValid(username) & isPasswordValid(password)) {


The build is failing because I'm using & here instead of &&. I cannot use short circuit and operation because both sides of the condition need to be called to show error on the username as well as the password field. @miPlodder As you said I'm not using any extra variables but this is failing on the findbugs check with the warning NS_DANGEROUS_NON_SHORT_CIRCUIT: Potentially dangerous use of non-short-circuit logic.

miPlodder · 2018-10-28T16:21:12Z

app/src/main/java/org/mifos/mobilebanking/presenters/LoginPresenter.java

@@ -75,7 +75,7 @@ public void detachView() {
     */
    public void login(final String username, final String password) {
        checkViewAttached();
-        if (isCredentialsValid(username, password)) {
+        if ((isUsernameValid(username) | isPasswordValid(password)) == 0) {


Use boolean functions and why are you doing Bitwise OR.

As I have mentioned before, if I use boolean and use logical && isPasswordValid will not be called if isUsernameValid return false as a result the error for the password field will not be shown.

Also, If I'm using boolean and bitwise & the build is failing with the error NS_DANGEROUS_NON_SHORT_CIRCUIT: Potentially dangerous use of non-short-circuit logic as we should not use bitwise operators for boolean values.

Bitwise operator is used to call both the methods in any case to show the error for both the fields (username and password).

Yeah, good hack. But this is not a good way to do it. That is what Build is also trying to say. Try to see #907 and implement it. (not fully, but partially as that code has more features also)

Separating the methods for validation is not needed.

@miPlodder That will require a boolean variable as I was doing it before. 😕

m-sameer · 2018-10-29T13:12:30Z

@miPlodder I've updated the PR again using boolean variables and the approach is similar to #907. I've tried using many approaches, if anything is wrong with this one please let me know. The build is passing and there are no performance issues.

miPlodder · 2018-10-30T02:10:56Z

app/src/main/java/org/mifos/mobilebanking/presenters/LoginPresenter.java

-            return false;
-        } else if (password == null || password.isEmpty()) {
+            credentialValid = false;
+        }


@m-sameer Add else and hide errors there. See #907 for reference.

@m-sameer Remove this
and add else there remove the error.

miPlodder · 2018-10-30T02:11:40Z

@m-sameer And update the GIF after changing the solution.

m-sameer · 2018-10-30T06:05:24Z

The errors are cleared on the login button click.

tilUsername.setErrorEnabled(false);
tilPassword.setErrorEnabled(false);

The GIF is already up to date.

miPlodder · 2018-10-30T14:28:32Z

app/src/main/java/org/mifos/mobilebanking/presenters/LoginPresenter.java

-            return false;
-        } else if (password == null || password.isEmpty()) {
+            credentialValid = false;
+        }


@m-sameer Remove this
and add else there remove the error.

app/src/main/java/org/mifos/mobilebanking/presenters/LoginPresenter.java

m-sameer · 2018-10-30T16:50:58Z

@miPlodder The requested changes have been incorporated.

miPlodder · 2019-01-02T08:38:14Z

@m-sameer Good job

m-sameer · 2019-01-02T11:21:01Z

@miPlodder Thank you!

m-sameer force-pushed the issue#895 branch 2 times, most recently from de7aae1 to 96fcdf5 Compare October 25, 2018 19:08

miPlodder reviewed Oct 26, 2018

View reviewed changes

build.gradle Outdated

google()

jcenter()

Copy link

Collaborator

miPlodder Oct 26, 2018

Choose a reason for hiding this comment

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

Remove this too.

miPlodder reviewed Oct 26, 2018

View reviewed changes

m-sameer force-pushed the issue#895 branch 3 times, most recently from 9e535b8 to 3c4515e Compare October 28, 2018 09:05

m-sameer commented Oct 28, 2018

View reviewed changes

m-sameer force-pushed the issue#895 branch from 3c4515e to 384fb03 Compare October 28, 2018 15:48

miPlodder requested changes Oct 28, 2018

View reviewed changes

m-sameer force-pushed the issue#895 branch from 384fb03 to 947b6c7 Compare October 29, 2018 12:44

miPlodder requested changes Oct 30, 2018

View reviewed changes

Fix openMF#895: Add error text below password EditText in LoginActivity

6c9a848

m-sameer force-pushed the issue#895 branch from 947b6c7 to 6c9a848 Compare October 30, 2018 16:49

miPlodder merged commit f4ec0b0 into openMF:development Jan 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #895: Add error text below password EditText in LoginActivity #913

Fix #895: Add error text below password EditText in LoginActivity #913

m-sameer commented Oct 25, 2018 •

edited

Loading

luckyman20 commented Oct 25, 2018

m-sameer commented Oct 25, 2018

miPlodder Oct 26, 2018

miPlodder Oct 26, 2018

miPlodder Oct 26, 2018

m-sameer Oct 26, 2018

miPlodder Oct 26, 2018 •

edited

Loading

m-sameer Oct 26, 2018

miPlodder Oct 26, 2018

luckyman20 Oct 26, 2018 •

edited

Loading

miPlodder Oct 26, 2018

luckyman20 Oct 26, 2018

m-sameer Oct 26, 2018

m-sameer commented Oct 28, 2018

luckyman20 commented Oct 28, 2018

m-sameer Oct 28, 2018

miPlodder Oct 28, 2018

m-sameer Oct 28, 2018

miPlodder Oct 29, 2018 •

edited

Loading

m-sameer Oct 29, 2018

m-sameer commented Oct 29, 2018

miPlodder Oct 30, 2018

miPlodder Oct 30, 2018

miPlodder commented Oct 30, 2018

m-sameer commented Oct 30, 2018

miPlodder Oct 30, 2018

m-sameer commented Oct 30, 2018

miPlodder commented Jan 2, 2019

m-sameer commented Jan 2, 2019

Fix #895: Add error text below password EditText in LoginActivity #913

Fix #895: Add error text below password EditText in LoginActivity #913

Conversation

m-sameer commented Oct 25, 2018 • edited Loading

luckyman20 commented Oct 25, 2018

m-sameer commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miPlodder Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckyman20 Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-sameer commented Oct 28, 2018

luckyman20 commented Oct 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miPlodder Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-sameer commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miPlodder commented Oct 30, 2018

m-sameer commented Oct 30, 2018

Choose a reason for hiding this comment

m-sameer commented Oct 30, 2018

miPlodder commented Jan 2, 2019

m-sameer commented Jan 2, 2019

m-sameer commented Oct 25, 2018 •

edited

Loading

miPlodder Oct 26, 2018 •

edited

Loading

luckyman20 Oct 26, 2018 •

edited

Loading

miPlodder Oct 29, 2018 •

edited

Loading