Fixed #4906 : Peer review: "thank the contributor" should show snackb… #4914

arinmodi · 2022-03-21T10:33:42Z

Fixes #4906

What changes did you make and why?

Replaced notification with snackbar

Tests performed

Tested build variant on ASUS_X00TD with API level 28

Tested {build variant, e.g. ProdDebug} on {name of device or emulator} with API level {API level}.

…show snackbar instead of notification

codecov · 2022-03-21T10:52:56Z

Codecov Report

Merging #4914 (cd4a951) into master (0d25c24) will decrease coverage by 0.01%.
The diff coverage is 44.44%.

❗ Current head cd4a951 differs from pull request most recent head cf2190b. Consider uploading reports for the commit cf2190b to get more accurate results

@@             Coverage Diff              @@
##             master    #4914      +/-   ##
============================================
- Coverage     51.42%   51.40%   -0.02%     
+ Complexity     2327     2324       -3     
============================================
  Files           345      345              
  Lines         16194    16196       +2     
  Branches       1430     1430              
============================================
- Hits           8327     8326       -1     
- Misses         7236     7237       +1     
- Partials        631      633       +2

Impacted Files	Coverage Δ
...a/fr/free/nrw/commons/review/ReviewController.java	`89.77% <44.44%> (+10.70%)`	⬆️
.../nrw/commons/category/CategoryContentProvider.java	`12.50% <0.00%> (-14.29%)`	⬇️
...w/commons/upload/categories/BaseDelegateAdapter.kt	`35.29% <0.00%> (-11.77%)`	⬇️
...va/fr/free/nrw/commons/category/CategoriesModel.kt	`57.62% <0.00%> (-1.70%)`	⬇️
...ns/upload/categories/UploadCategoriesFragment.java	`86.95% <0.00%> (-1.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bb2cbc...cf2190b. Read the comment docs.

nicolas-raoul · 2022-03-21T13:36:06Z

I just tried, this is pretty annoying, I feel like I can't press buttons because the snackbar just appeared...

@neslihanturan any UI advice on showing to the user that the thank was sent, without obstructing the UI? screenshot

4D17Y4 · 2022-03-22T03:46:58Z

I just tried, this is pretty annoying, I feel like I can't press buttons because the snackbar just appeared...

@neslihanturan any UI advice on showing to the user that the thank was sent, without obstructing the UI? screenshot

I think length short would work just fine. It won't solve the issue of ui here but would not obstruct the ui for long.
We can go with custom time if that feels too long.

madhurgupta10 · 2022-03-24T14:36:24Z

IMO a toast would make better sense here than snackbar since it doesn't block the UI.

arinmodi · 2022-03-25T16:46:19Z

@nicolas-raoul, what i have to implement snackbar with length_short or toast

According to me, toast will be better

But @nicolas-raoul, please update
Thanks

misaochan · 2022-03-25T18:19:28Z

Another vote for toast here. :)

nicolas-raoul · 2022-03-26T00:27:59Z

OK, let's go with a short toast :-)

…mons into snackbar

arinmodi · 2022-03-26T08:37:00Z

@nicolas-raoul, changes the snackbar to short lenght toast

4D17Y4

Looks good to me otherwise.

4D17Y4 · 2022-03-27T17:26:52Z

app/src/main/java/fr/free/nrw/commons/review/ReviewController.java

 import android.app.NotificationManager;
 import android.content.Context;

+import android.view.View;


These Imports are not needed anymore.

4D17Y4 · 2022-03-27T18:47:19Z

app/src/main/java/fr/free/nrw/commons/review/ReviewController.java


-                    showNotification(title, message);
-
+                    ViewUtil.showShortToast(context,message);


This statement was not covered in tests.
Add a test for this line ?

@4D17Y4, how can i cover the code inside of the Observable.defer ?

I believe it should be possible, can u look at the present instances in the codebase if there are any ?
I think we have similar code but not sure if they are covered by tests. Let me know in any case.

@4D17Y4, Sorry but I tried to find on internet and in the codebase also but not able to figure it out

@madhurgupta10, can you please help in this ?

I don't have access to my computer for a few days, maybe you can look at some samples in the codebase or maybe @devarsh-mavani-19 or @Ayan-10 can help you if they faced similar issues before.

I don't have access to my computer for a few days, maybe you can look at some samples in the codebase or maybe @devarsh-mavani-19 or @Ayan-10 can help you if they faced similar issues before.

I told him solution over call a few hours ago. As we were from same college previously and we are in touch with each other.

The way i handled it is by putting code of defer method in a separate function and writing tests for that function. i have implemented it previously in 1 of my PRs will update if i find it. Thanks

…mons into snackbar

arinmodi · 2022-03-31T13:27:47Z

@4D17Y4 , changes are done

4D17Y4

Looks good otherwise, nice work.

4D17Y4 · 2022-03-31T18:37:28Z

app/src/main/java/fr/free/nrw/commons/review/ReviewController.java


-                    showNotification(title, message);
+    @SuppressLint("StringFormatInvalid")
+    private void displayToast(final Context context, final boolean result){


displayToast is too generic. I would suggest displayThanksToast as this function is specific to the use case.
Also add some documentation to this function.

…mons into snackbar

arinmodi · 2022-04-05T15:59:05Z

@4D17Y4, please review this and sorry for the delay i was working on another project.

arinmodi · 2022-04-05T16:41:50Z

@4D17Y4, I was trying to run a test but it gives me the invalid res directory name error.

4D17Y4 · 2022-04-06T11:28:51Z

This could be related to #1059

neslihanturan

Looks correct to me, thanks @arinmodi :) Just waiting tests to be done to merge

Fixed commons-app#4906 : Peer review: "thank the contributor" should …

4043453

…show snackbar instead of notification

arinmodi added 2 commits March 26, 2022 13:34

Merge branch 'master' of https://github.com/arinmodi/apps-android-com…

ca6c764

…mons into snackbar

changes the snackbar to toast

aa5fbaf

4D17Y4 suggested changes Mar 27, 2022

View reviewed changes

arinmodi added 2 commits March 31, 2022 18:54

Merge branch 'master' of https://github.com/arinmodi/apps-android-com…

98cd9c1

…mons into snackbar

added the tests

7153b09

4D17Y4 suggested changes Mar 31, 2022

View reviewed changes

arinmodi added 2 commits April 5, 2022 21:24

Merge branch 'master' of https://github.com/arinmodi/apps-android-com…

918e79e

…mons into snackbar

minor changes

b040106

minor changes - 2

cf2190b

neslihanturan approved these changes Apr 18, 2022

View reviewed changes

neslihanturan merged commit 1ae013d into commons-app:master Apr 25, 2022


		showNotification(title, message);

		ViewUtil.showShortToast(context,message);

Fixed #4906 : Peer review: "thank the contributor" should show snackb… #4914

Fixed #4906 : Peer review: "thank the contributor" should show snackb… #4914

Uh oh!

Conversation

arinmodi commented Mar 21, 2022

Uh oh!

codecov bot commented Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nicolas-raoul commented Mar 21, 2022

Uh oh!

4D17Y4 commented Mar 22, 2022

Uh oh!

madhurgupta10 commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arinmodi commented Mar 25, 2022

Uh oh!

misaochan commented Mar 25, 2022

Uh oh!

nicolas-raoul commented Mar 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arinmodi commented Mar 26, 2022

Uh oh!

4D17Y4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arinmodi commented Mar 31, 2022

Uh oh!

4D17Y4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arinmodi commented Apr 5, 2022

Uh oh!

arinmodi commented Apr 5, 2022

Uh oh!

4D17Y4 commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neslihanturan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Mar 21, 2022 •

edited

Loading

madhurgupta10 commented Mar 24, 2022 •

edited

Loading

nicolas-raoul commented Mar 26, 2022 •

edited

Loading

4D17Y4 commented Apr 6, 2022 •

edited

Loading