Improve peer review feature quality

[neslihanturan]

Summary:

After #2602 is merged #2253 is completed. Now some quality issues from hackathon still needs to be fixed.

• Code style issues @maskaravivek
• Move from XML to JSON @maskaravivek
• It scrolls, I would prefer static/ support different screen sizes
• Images moves on swipe, just questions should move instead @vanshikaarora
• A beta alert should be added somewhere
• Make image clickable - Open media details activity when clicked.
• Not showing the categories peer review screen for the images having zero categories.
• Or let the user categorize the image.
• Adding a question mark next to "skip this image" button and Peer Review title which will display an explanation about their usage. @silkypriya
• Refactor asynctasks to Rxjava implementation @vanshikaarora
• Write unit tests with kotlin @neslihanturan
• Optimize randomizer
• Extracting string resources for i18n

I am willing to work on some of them. But we can share, anyone wants to work on them please comment down.

[vanshikaarora]

Hey @neslihanturan I would like to work on this Issue

[neslihanturan]

Hi @vanshikaarora , thanks for your interest. Do you have other tasks on you? If so I would recommend finish them first. Otherwise, would you like to write unit tests for this task?

[vanshikaarora]

@neslihanturan I have some PR's up for review until then I can work on this.

[neslihanturan]

Sure, then I assign "Write unit tests with kotlin" to you? Does it sound good?

[vanshikaarora]

Yes I will. It would be good if you can tell me specifically about some of the tests that we need to check :)

[neslihanturan]

Following this conversation can be helpful -> #2253 . We have whole workflow there, can help you to understand what we expect from the peer review activity.

[vanshikaarora]

Currently I see three test cases:

1. Answering the questions(clicking on the bottom button) should take us to next view pager
2. On answering the last question view pager should come to first and load the next image
   3.On pressing next reload image and return to the first element of viewpager

@neslihanturan Can you add more to the list :)

[neslihanturan]

• All of the requests and their answers from server.
• Test for the randomizer query

I just recognized, actually tests can be complex for this feature. Even though with your previous contributions you have proven that you can handle it, still you can always pick another thing from the list, I can pick the tests too. What about

• Images moves on swipe, just questions should move instead
  and/or
• Refactor asynctasks to Rxjava implementation?

They are fairly complex too, but at leats easier to define the scope. I was planning to claim them but we can exchange (tests to me, other 2 to you), what do you say @vanshikaarora ?

[vanshikaarora]

They are fairly complex too, but at leats easier to define the scope. I was planning to claim them but we can exchange (tests to me, other 2 to you), what do you say @vanshikaarora ?

@neslihanturan since I think that's fair enough. Since for writing test cases it will take me time to be familiar with the changes in the code.

I'll start with the other two tasks 👍

[neslihanturan]

Great:) Do you need more explanation with your 2 tasks?

[maskaravivek]

@neslihanturan To add to the list, the recent changes API can be moved to JSON. As of now, we are fetching an XML response. I would like to pick this up.

[maskaravivek]

Also, theres a lot of commented out code in various places. Am removing it.

[neslihanturan]

Thanks @maskaravivek ! I am assigning you for code style in general and moving to JSON, by editing the first comment.

[vanshikaarora]

Do you need more explanation with your 2 tasks?

@neslihanturan As part of one of my work I am supposed to replace the classes SendThankTask.java and CheckCategoryTask.java to their corresponding RxJava implementation. Right?

[vanshikaarora]

Refactor asynctasks to Rxjava implementation
Images moves on swipe, just questions should move instead

@neslihanturan I have solved the above two issues in #2707 and #2709. Kindly review and share your feedbacks :)

[vanshikaarora]

Hey @neslihanturan can you assign me more tasks from this list?

[vanshikaarora]

Make image clickable - Open media details activity when clicked.
Not showing the categories peer review screen for the images having zero categories.

Hey @neslihanturan I can pick these two task.

[vanshikaarora]

Hey @domdomegg I was working on some other Issue's enlisted here. If those issues still need to be fixed can you please reopen this Issue :)

[vanshikaarora]

Not showing the categories peer review screen for the images having zero categories.

Hey @neslihanturan I have solved the above Issue here. Kindly review :)

[neslihanturan]

@vanshikaarora I have added some reviews to your PRs, for now it can be better to focus on your already open PRs instead of having new one. Thanks for several contributions:)

[silkypriya]

Adding a question mark next to "skip this image" button and Peer Review title which will display an explanation about their usage.

Hello, @neslihanturan
Can I take this issue ?
I was woking on this and had almost completed this task. Can I make PR for this ?
Thank you!

[neslihanturan]

Sure @silkypriya , thanks:)

[domdomegg]

Can I take this issue ?
I was woking on this and had almost completed this task. Can I make PR for this ?

@silkypriya When you're doing this please can you use the ic_info_outline_24dp drawable, with the android:tint attribute - See #2734. (edited to reflect merge status)

[neslihanturan]

Currently on clicking the image it does nothing. IMO we should show the user the media details on clicking the image same as we do for explore and contributions fragment.

Please continue this suggestion from here @sp2710 :)

[silkypriya]

Hello @domdomegg @neslihanturan ,
I have used ic_help_black_24dp with tint attribute. Shall I change that to info icon ?

Made PR #2783.
Kindly review
Thank you!

[domdomegg]

I have used ic_help_black_24dp with tint attribute. Shall I change that to info icon ?

I think it might be better to use the info icon given we use it in the rest of the app for the same purpose - we should be consistent.

[cypherop]

@neslihanturan I am picking #2763

[silkypriya]

Hello @neslihanturan
Peer Review activity layout does not support different screen sizes.
Can we add this in the above listed issues ?

Currently	Expected

[neslihanturan]

I am adding, thanks @silkypriya

[silkypriya]

Hello @neslihanturan,
Fixed the issue mentioned in last comment in #2811
Kindly Review.
Thank you!

[maskaravivek]

Optimize randomizer

@neslihanturan Why do we have such a complex logic for randomizer? Can't we randomly pick one revision from the list?

[neslihanturan]

@maskaravivek can you explain more what do you indicate by complex? At the end of the day our purpose is selecting from recently uploaded mobile uploads.

[maskaravivek]

@maskaravivek can you explain more what do you indicate by complex? At the end of the day our purpose is selecting from recently uploaded mobile uploads.

Oh sorry, I didn't realize that we are trying to select an image uploaded from mobile. But now I am confused that how are we actually filtering mobile uploads. Can you briefly explain the process?

[nicolas-raoul]

I marked many of the issues as fixed, and the rest have been made not so relevant anymore, due to related changes.
Thus I think this can be closed.
Thanks everyone for your hard work! :-)