Skip to content
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

Feat : Added a feature for users to add feadback on github #5578

Conversation

neeldoshii
Copy link
Contributor

Description

Approach

It gives the users an ability to place feedbacks (issues, new feature) directly on the github from the app.

Testing Instructions

  1. Open the More Menu from Bottom Navigation.
  2. Bottom Sheet will be opened.
  3. From the Bottom Sheet click on Send Feedback on Github
  4. Browser will open (github issue section)

Screenshots/ Working Video

feedback.on.github.mp4

Tests performed

Test performed on Google Pixel 4 (Emulator).

Feel free to place your opinion.

Copy link
Contributor

@shashankiitbhu shashankiitbhu left a comment

Choose a reason for hiding this comment

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

Review by student

Before:

WhatsApp.Video.2024-03-01.at.9.51.53.PM.mp4

After:

WhatsApp.Video.2024-03-01.at.9.52.10.PM.mp4

Mentioned Issue is Solved


setUserName();
return binding.getRoot();
}

private void onFeedbackGithubClicked() {
final String url = "https://github.com/commons-app/apps-android-commons/issues";
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make this a string constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. Thanks for the feedback.

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Thanks! :-)

@nicolas-raoul nicolas-raoul merged commit 54312fc into commons-app:main Apr 1, 2024
1 check failed
@ShashwatKedia
Copy link
Contributor

@nicolas-raoul Sorry to ping after the PR has been merged, but this PR has two test cases failing here:

fr.free.nrw.commons.description.DescriptionEditActivityUnitTest > testUpdateDescription FAILED java.lang.reflect.InvocationTargetException at DescriptionEditActivityUnitTest.kt:120 Caused by: fr.free.nrw.commons.auth.csrf.InvalidLoginTokenException at DescriptionEditActivityUnitTest.kt:120

fr.free.nrw.commons.description.DescriptionEditActivityUnitTest > testOnSubmitButtonClicked FAILED java.lang.reflect.InvocationTargetException at DescriptionEditActivityUnitTest.kt:132 Caused by: fr.free.nrw.commons.auth.csrf.InvalidLoginTokenException at DescriptionEditActivityUnitTest.kt:132

This is causing the tests to fail on all other PRs. Also, while I was testing the main branch, I was surprised to see that this PR was merged, when there was still some discussion going on here about the best possible UI for feedback on GitHub. And as mentioned there, even you didn't approve of the concept of having 2 separate menu items.

@nicolas-raoul
Copy link
Member

Thanks @ShashwatKedia for pointing this out!
Actually, even after I revert 54312fc these tests fail.
Maybe an earlier commit? git bisect would help here.

I agree two menu items is not ideal, but I felt that it was still better than nothing.

@nicolas-raoul
Copy link
Member

git bisect says 3d1efec is the first commit where unit tests start failing.

@ShashwatKedia
Copy link
Contributor

Oh, thanks and sorry, @nicolas-raoul, that you had to perform git bisect for this. I'll look into this and try to perform the actions required by PR without failing the tests

@ShashwatKedia
Copy link
Contributor

I agree two menu items is not ideal, but I felt that it was still better than nothing.

Um, what do you think about the solution proposed here ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commons app feedback with reference to GitHub
4 participants