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

Provide a selectable list of reasons when requesting deletion of a file #1824

Merged
merged 5 commits into from
Nov 20, 2018

Conversation

tanvidadu
Copy link
Collaborator

@tanvidadu tanvidadu commented Aug 13, 2018

Provide a selectable list of reasons when requesting deletion of a file

Fixes #1768

Description (required)

Added reasons in drop down menu list

Need to work on UI

Tests performed (required)

Tested on ProdDebug.

Screenshots showing what changed (optional)

{Only for user interface changes, otherwise remove this section. See how to take a screenshot}

Note: Please ensure that you have read CONTRIBUTING.md if this is your first pull request.

@codecov-io
Copy link

codecov-io commented Aug 13, 2018

Codecov Report

Merging #1824 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1824      +/-   ##
=========================================
- Coverage    4.14%   4.12%   -0.02%     
=========================================
  Files         223     224       +1     
  Lines       11197   11241      +44     
  Branches     1021    1023       +2     
=========================================
  Hits          464     464              
- Misses      10698   10742      +44     
  Partials       35      35
Impacted Files Coverage Δ
...fr/free/nrw/commons/media/MediaDetailFragment.java 0% <0%> (ø) ⬆️
...java/fr/free/nrw/commons/delete/ReasonBuilder.java 0% <0%> (ø)

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 e9bfc4b...2bcc8c1. Read the comment docs.

"I did not know it would be publicly visible",
"I realized it is bad for my privacy",
"Sorry this picture is not interesting for an encyclopedia",
"I changed my mind, I don't want it to be publicly visible anymore"};
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late notice, but how about an "Other (please detail on wiki)" reason too, for people who have a legitimate reason we have not thought about? :-)

Copy link
Member

Choose a reason for hiding this comment

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

Also, I guess these strings should be extracted to strings.xml

Copy link
Member

@misaochan misaochan Aug 14, 2018

Choose a reason for hiding this comment

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

Definitely should be in strings.xml :)

I am not sure if there is much difference between "I did not know it would be publicly visible", , "I realized it is bad for my privacy", and "I changed my mind, I don't want it to be publicly visible anymore"? Would it be better to have just one option instead of those 3, that just says "Privacy concerns"? @nicolas-raoul

Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could finalize the list of reasons to be shown. The rest of the PR looks good to me and the PR can be merged once the strings have been finalized.

@misaochan @nicolas-raoul

Copy link
Member

@nicolas-raoul nicolas-raoul Oct 29, 2018

Choose a reason for hiding this comment

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

Personally I would keep all three, because:

  • "I did not know it would be publicly visible" can not be restricted to privacy only. It also encompasses people who thought it was a kind of game and uploaded a test picture of their wall. A bit like a first-time Wikipedia editor who blanks a page just to see if that works and then freaks out when they realize it actually worked.
  • "I changed my mind etc" is not only about privacy, it might be that the uploader did not read the license first. I suggest rewording it to just "I changed my mind".
  • In the future, these choices might give us better insight about users behavior. For instance if we have 80% of "I did not know it would be publicly visible" then we might decide to add some explanation within the app.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will make the required changes !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After rebasing the pr with current master, I am getting Failed to find byte code for com/mapzen/android/lost/api/LostApiClient$ConnectionCallbacks error. Any idea on how to resolve it without changing mapbox dependencies ?

Copy link
Member

Choose a reason for hiding this comment

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

@tanvidadu Does it happen even on a clean build?

Also, can you update the PR after rebasing so that we can try reproducing the issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes @maskaravivek ,This error persists even on a clean build
I have updated the Pull request !

Copy link
Member

Choose a reason for hiding this comment

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

I pulled your branch and tried building and running prodDebug and betaDebug. The above error didn't occur for me. Can you share the full stack trace?

Eventually, if this error keeps happening for you then we can help in testing and get it merged. :)

Upload your first media by touching the camera or gallery icon above.</string>
<string name="no_uploads">Welcome to Commons!\n Upload your first media by touching the camera or gallery icon above.</string>

<string name="deletion_reason_1">I uploaded it by mistake</string>
Copy link
Member

Choose a reason for hiding this comment

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

rename string name. You can probably use

deletion_reason_uploaded_by_mistake
deletion_reason_publicly_visible
deletion_reason_bad_for_my_privacy
deletion_reason_no_longer_want_public
deletion_reason_not_interesting

}

private void appendArticlesUsed(FeedbackResponse object){
reason += ". Uploaded by myself on" + prettyUploadedDate(media);
Copy link
Member

Choose a reason for hiding this comment

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

Move to strings.xml

@maskaravivek
Copy link
Member

@tanvidadu This might be helpful #1943 (comment)

@tanvidadu
Copy link
Collaborator Author

Thanks @maskaravivek !

@tanvidadu tanvidadu changed the title Provide a selectable list of reasons when requesting deletion of a file[WIP] Provide a selectable list of reasons when requesting deletion of a file Nov 4, 2018
@maskaravivek
Copy link
Member

The PR works perfectly for me. Happy to merge it once one more person is able to test it.

I nominated 2 files for deletion using this PR, selecting different reasons.

https://commons.wikimedia.org/wiki/File:Elephant_6.jpg
https://commons.wikimedia.org/wiki/File:Elephant_7.jpg

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.

5 participants