-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Improve peer review feature quality #2698
Comments
Hey @neslihanturan I would like to work on this Issue |
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? |
@neslihanturan I have some PR's up for review until then I can work on this. |
Sure, then I assign "Write unit tests with kotlin" to you? Does it sound good? |
Yes I will. It would be good if you can tell me specifically about some of the tests that we need to check :) |
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. |
Currently I see three test cases:
@neslihanturan Can you add more to the list :) |
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
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 👍 |
Great:) Do you need more explanation with your 2 tasks? |
@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. |
Also, theres a lot of commented out code in various places. Am removing it. |
Thanks @maskaravivek ! I am assigning you for code style in general and moving to JSON, by editing the first comment. |
@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? |
@neslihanturan I have solved the above two issues in #2707 and #2709. Kindly review and share your feedbacks :) |
Hey @neslihanturan can you assign me more tasks from this list? |
Hey @neslihanturan I can pick these two task. |
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 :) |
Hey @neslihanturan I have solved the above Issue here. Kindly review :) |
@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:) |
Hello, @neslihanturan |
Sure @silkypriya , thanks:) |
@silkypriya When you're doing this please can you use the |
Please continue this suggestion from here @sp2710 :) |
Hello @domdomegg @neslihanturan , Made PR #2783. |
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. |
@neslihanturan I am picking #2763 |
Hello @neslihanturan
|
I am adding, thanks @silkypriya |
Hello @neslihanturan, |
@neslihanturan Why do we have such a complex logic for randomizer? Can't we randomly pick one revision from the list? |
@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? |
I marked many of the issues as fixed, and the rest have been made not so relevant anymore, due to related changes. |
Summary:
After #2602 is merged #2253 is completed. Now some quality issues from hackathon still needs to be fixed.
I am willing to work on some of them. But we can share, anyone wants to work on them please comment down.
The text was updated successfully, but these errors were encountered: