Precise error message password change #5544

shashankiitbhu · 2024-02-12T18:30:21Z

Description (required)

What changes did you make and why?
Made changes in UploadWorker making the user go to LoginActivity if the password is changed in their current session (which made their current session Invalid), and will display a Persistent Message to the user of what they need to do on Login Page.

Tests performed (required)

Tested 4.2.1-debug-main on Xiaomi 11 Lite NE with API level 33

Screenshots (for UI changes only)

app/src/main/java/fr/free/nrw/commons/upload/StashUploadResult.kt

app/src/main/java/fr/free/nrw/commons/upload/UploadClient.java

app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt

domdomegg

As per comments.

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt

sivaraam · 2024-02-13T14:13:24Z

Thanks for working on this @shashankiitbhu and thanks for the feedback @domdomegg and @whym 🙂 !

I could see what this PR is a achieving is a better state than what we have now. But is it not too late to throw this at the user after they've entered all their details?

Could we not do it as soon as they open the app? Doing it on each open might be a bit costly, may be we could cache the result and re-check once in a few hours?

Or if that sounds too complex, may be just check the validity right when the upload is initiated via the UI? May be asynchronously to rnsure we don't block user's flow? Any thoughts? 🤔

shashankiitbhu · 2024-02-13T14:35:40Z

@sivaraam

If we do it every time the user opens the app then it'll be too expensive
If we do it let's say every few hours then there would remain a possibility that the user encounters this same issue when they change their password during that period (Hence we need to handle that case anyway, as we are doing in this PR), we can implement this and also have the changes the I made in this PR currently so that there is no cases where user does not get a clear message after upload failure.

So my proposed solution is:
-> Make the check when a user opens the app and re-check once in a few hours
-> Also keep these changes to handle the above case (2)

This way we are making fewer network requests and ensuring that there are no cases where the user does not get a clear message after upload failure.

Please Let me know what you think about this solution

app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt

app/src/main/java/fr/free/nrw/commons/auth/csrf/CsrfTokenClient.kt

whym · 2024-02-26T08:47:56Z

@sivaraam:
I like your idea of checking it earlier than the last step of the uploading process. Do you want to create a new issue for that? If I understand it correctly, the two options are not exclusive.

When tokens are invalidated, it's done on the server side, and we don't know when it will happen. There is a chance that it will happen during the upload process (and the user can take time to choose categories and other options). So I don't think we have to choose between the two timings of checking - I think we want to do both.

EDIT: I copy pasted the wrong user name at the top earlier, sorry.

shashankiitbhu · 2024-03-02T18:11:44Z

@whym as we have already discussed and as you said we can go with approach 1 here - https://github.com/commons-app/apps-android-commons/issues/5522#issuecomment-1942891133, I am already working on my other 2 PRs for this issue ( or should I create separate issues for those PRs?)

# Conflicts: # app/src/main/java/fr/free/nrw/commons/upload/UploadClient.java

shashankiitbhu · 2024-03-02T18:50:26Z

@nicolas-raoul Can you please Review and Merge this PR so that I have a clear indication to similarly solve for the other 2 cases as discussed in #5522 (comment) and eventually close the issue #5522 , as it's been open for a while and it's quite crucial bug

whym · 2024-03-04T13:30:27Z

From my perspective, this looks already okay to merge. I think we can incorporate @sivaraam's idea above in future iterations.

shashankiitbhu · 2024-03-04T13:37:20Z

@whym Great, then I'll add the other two PRs for the other two scenarios soon and get this issue fixed

shashankiitbhu · 2024-03-05T15:35:33Z

@nicolas-raoul @RitikaPahwa4444 Please Review & Merge this PR so that I can make the other two PRs

shashankiitbhu · 2024-03-13T09:45:09Z

@nicolas-raoul @RitikaPahwa4444 @sivaraam can you please review and merge it, thanks

sivaraam

Sorry for the delay here. I just had one comment about curbing the redundant code. Kindly check the same 🙂

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt

sivaraam · 2024-03-14T17:49:18Z

@sivaraam: I like your idea of checking it earlier than the last step of the uploading process. Do you want to create a new issue for that? If I understand it correctly, the two options are not exclusive.

When tokens are invalidated, it's done on the server side, and we don't know when it will happen. There is a chance that it will happen during the upload process (and the user can take time to choose categories and other options). So I don't think we have to choose between the two timings of checking - I think we want to do both.

I agree with you on this. We would indeed need both of those options. On a side note, it would be worth exploring how the Wikipedia app handles this. They do have a logout method similar to ours. It would be interesting to figure out when and where they call the same.

shashankiitbhu · 2024-03-14T18:28:50Z

@sivaraam Made Required Changes , Please Review and Merge, thanks

sivaraam

Kindly take care of the below comments.

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt

shashankiitbhu · 2024-03-14T19:10:01Z

@sivaraam Made the changes

sivaraam

Thanks for the quick turnaround! Here are few comments to help improve the changes. Do take a look 🙂

app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java

app/src/main/java/fr/free/nrw/commons/navtab/MoreBottomSheetFragment.java

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java

shashankiitbhu · 2024-03-15T08:54:49Z

@sivaraam Made the required changes, Please Review and Merge, thanks

sivaraam

I left some comments regarding the JavaDoc. Kindly check them.

Also, I seem to get the following crash from time to time when the app logs me out due to token invalidation:

android.database.sqlite.SQLiteException: no such table: workspec (code 1 SQLITE_ERROR): , while compiling: SELECT output FROM workspec WHERE id IN
             (SELECT prerequisite_id FROM dependency WHERE work_spec_id=?)
        at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method)
        at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:1053)
        at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:660)
        at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:590)
        at android.database.sqlite.SQLiteProgram.<init>(SQLiteProgram.java:62)
        at android.database.sqlite.SQLiteQuery.<init>(SQLiteQuery.java:37)
        at android.database.sqlite.SQLiteDirectCursorDriver.query(SQLiteDirectCursorDriver.java:46)
        at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1546)
        at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1521)
        at androidx.sqlite.db.framework.FrameworkSQLiteDatabase.query(FrameworkSQLiteDatabase.kt:156)
        at androidx.room.RoomDatabase.query(RoomDatabase.kt:481)
        at androidx.room.util.DBUtil.query(DBUtil.kt:75)
        at androidx.work.impl.model.WorkSpecDao_Impl.getInputsFromPrerequisites(WorkSpecDao_Impl.java:1552)
        at androidx.work.impl.WorkerWrapper.runWorker(WorkerWrapper.java:223)
        at androidx.work.impl.WorkerWrapper.run(WorkerWrapper.java:145)
        at androidx.work.impl.utils.SerialExecutorImpl$Task.run(SerialExecutorImpl.java:96)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
        at java.lang.Thread.run(Thread.java:1012)

It seems similar to this issue reported here. Am I the only one facing this? Or is it generally reproducible? If it is common, any idea on what's causing this issue?

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java

shashankiitbhu · 2024-03-18T20:32:25Z

Also, I seem to get the following crash from time to time when the app logs me out due to token invalidation:

I have not faced this Issue yet so I am not sure if that is related to this PR

shashankiitbhu · 2024-03-18T20:46:09Z

@sivaraam made the changes suggested, let me know if something else is required, Please Review and Merge, thanks

sivaraam

The JavaDocs could still use some improvements. Kindly check the comments.

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java

sivaraam · 2024-03-19T18:41:59Z

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java

+        /**
+         * Constructor for BaseLogoutListener
+         *
+         * @param ctx           Application context


The purpose of the ctx is not specified clearly. Kindly clarify the same.

This seems unresolved yet.

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java

shashankiitbhu · 2024-03-20T22:51:52Z

@sivaraam Made Some changes , Please Review , thanks

sivaraam

Just left 2 comments. Kindly check. Rest of the changes look fine 👍

sivaraam · 2024-03-21T03:27:58Z

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java

+        /**
+         * Constructor for BaseLogoutListener
+         *
+         * @param ctx           Application context


This seems unresolved yet.

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java

shashankiitbhu · 2024-03-21T16:04:09Z

@sivaraam Made the changes, Please Review

sivaraam · 2024-03-21T17:52:21Z

Thank you for your contribution @shashankiitbhu 🙂

OpenGreenStreet · 2024-07-01T05:07:33Z

The problem persists even with version 5.0.1~da0b2c28e. No meaningful message is displayed after a previous password change via the Wikipedia web interface:

https://commons.wikimedia.org/wiki/File:20240701_xl_0615-no_error_messages_after_change_password.png
https://commons.wikimedia.org/wiki/File:20240701_xl_0615-no_error_messages_after_change_password_2.png
https://commons.wikimedia.org/wiki/File:20240701_xl_0615-no_error_messages_after_change_password_3.png
https://commons.wikimedia.org/wiki/File:20240701_xl_0615-no_error_messages_after_change_password_4.png

shashankiitbhu added 2 commits February 11, 2024 00:23

Precise Error Message and Action When Password is Changed

876ef0c

Make message persistent

45c82b1

domdomegg reviewed Feb 12, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/upload/StashUploadResult.kt Outdated Show resolved Hide resolved

domdomegg reviewed Feb 12, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/upload/UploadClient.java Outdated Show resolved Hide resolved

domdomegg reviewed Feb 12, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java Outdated Show resolved Hide resolved

domdomegg reviewed Feb 12, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java Outdated Show resolved Hide resolved

domdomegg reviewed Feb 12, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt Outdated Show resolved Hide resolved

domdomegg requested changes Feb 12, 2024

View reviewed changes

whym reviewed Feb 13, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt Outdated Show resolved Hide resolved

code cleanup and fixes

0dc3335

domdomegg reviewed Feb 13, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java Outdated Show resolved Hide resolved

removed unnecessary string resource

0af3dd6

domdomegg approved these changes Feb 13, 2024

View reviewed changes

whym reviewed Feb 14, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt Outdated Show resolved Hide resolved

removed unnecessary string resource

49703c3

whym reviewed Feb 14, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/auth/csrf/CsrfTokenClient.kt Outdated Show resolved Hide resolved

shashankiitbhu added 2 commits February 14, 2024 14:20

fix

b9afa1c

fix

74727bf

shashankiitbhu added 2 commits March 2, 2024 23:42

Merge branch 'main' into Precise_error_message_password_change

10ff6ca

# Conflicts: # app/src/main/java/fr/free/nrw/commons/upload/UploadClient.java

Upload Client to kotlin

68c816a

whym approved these changes Mar 4, 2024

View reviewed changes

whym requested a review from sivaraam March 14, 2024 08:29

sivaraam reviewed Mar 14, 2024

View reviewed changes

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt Outdated Show resolved Hide resolved

Remove Redundant Code

26c2932

sivaraam reviewed Mar 14, 2024

View reviewed changes

Remove Redundant Code

c6f09c1

sivaraam reviewed Mar 15, 2024

View reviewed changes

code cleanup and Improvements

8003950

sivaraam reviewed Mar 18, 2024

View reviewed changes

Improved Java doc

a065ab9

sivaraam reviewed Mar 19, 2024

View reviewed changes

Improved Java doc

68a4aea

sivaraam reviewed Mar 21, 2024

View reviewed changes

Improved Javadoc

24befbd

sivaraam merged commit 2a6ab66 into commons-app:main Mar 21, 2024
1 check passed

shashankiitbhu mentioned this pull request Mar 23, 2024

[Bug]: No Precise Error Message After Error Due to Password Change #5642

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Precise error message password change #5544

Precise error message password change #5544

shashankiitbhu commented Feb 12, 2024

domdomegg left a comment •

edited

Loading

sivaraam commented Feb 13, 2024

shashankiitbhu commented Feb 13, 2024 •

edited

Loading

whym commented Feb 26, 2024 •

edited

Loading

shashankiitbhu commented Mar 2, 2024

shashankiitbhu commented Mar 2, 2024

whym commented Mar 4, 2024

shashankiitbhu commented Mar 4, 2024

shashankiitbhu commented Mar 5, 2024

shashankiitbhu commented Mar 13, 2024

sivaraam left a comment

sivaraam commented Mar 14, 2024

shashankiitbhu commented Mar 14, 2024

sivaraam left a comment

shashankiitbhu commented Mar 14, 2024

sivaraam left a comment

shashankiitbhu commented Mar 15, 2024

sivaraam left a comment

shashankiitbhu commented Mar 18, 2024

shashankiitbhu commented Mar 18, 2024

sivaraam left a comment

sivaraam Mar 19, 2024

sivaraam Mar 21, 2024

shashankiitbhu commented Mar 20, 2024

sivaraam left a comment •

edited

Loading

sivaraam Mar 21, 2024

shashankiitbhu commented Mar 21, 2024

sivaraam commented Mar 21, 2024 •

edited

Loading

OpenGreenStreet commented Jul 1, 2024

Precise error message password change #5544

Precise error message password change #5544

Conversation

shashankiitbhu commented Feb 12, 2024

domdomegg left a comment • edited Loading

Choose a reason for hiding this comment

sivaraam commented Feb 13, 2024

shashankiitbhu commented Feb 13, 2024 • edited Loading

whym commented Feb 26, 2024 • edited Loading

shashankiitbhu commented Mar 2, 2024

shashankiitbhu commented Mar 2, 2024

whym commented Mar 4, 2024

shashankiitbhu commented Mar 4, 2024

shashankiitbhu commented Mar 5, 2024

shashankiitbhu commented Mar 13, 2024

sivaraam left a comment

Choose a reason for hiding this comment

sivaraam commented Mar 14, 2024

shashankiitbhu commented Mar 14, 2024

sivaraam left a comment

Choose a reason for hiding this comment

shashankiitbhu commented Mar 14, 2024

sivaraam left a comment

Choose a reason for hiding this comment

shashankiitbhu commented Mar 15, 2024

sivaraam left a comment

Choose a reason for hiding this comment

shashankiitbhu commented Mar 18, 2024

shashankiitbhu commented Mar 18, 2024

sivaraam left a comment

Choose a reason for hiding this comment

sivaraam Mar 19, 2024

Choose a reason for hiding this comment

sivaraam Mar 21, 2024

Choose a reason for hiding this comment

shashankiitbhu commented Mar 20, 2024

sivaraam left a comment • edited Loading

Choose a reason for hiding this comment

sivaraam Mar 21, 2024

Choose a reason for hiding this comment

shashankiitbhu commented Mar 21, 2024

sivaraam commented Mar 21, 2024 • edited Loading

OpenGreenStreet commented Jul 1, 2024

domdomegg left a comment •

edited

Loading

shashankiitbhu commented Feb 13, 2024 •

edited

Loading

whym commented Feb 26, 2024 •

edited

Loading

sivaraam left a comment •

edited

Loading

sivaraam commented Mar 21, 2024 •

edited

Loading