wikipedia-ios icon indicating copy to clipboard operation
wikipedia-ios copied to clipboard

T351649 - add edit summaries when updating short description

Open philherbert opened this issue 1 year ago • 1 comments

Phabricator: https://phabricator.wikimedia.org/T351649

Notes

  • This is my first PR to this project, so apologies for any errors!
  • This PR uses the existing summary parameter in sectionUploader.uploadWikiText(...) to provide a default edit summary when updating an existing short description
  • Also copies the summary parameter to sectionUploader.prepend(...) for the same purpose, when adding a new short description

Test Steps

Add flow

  1. Find an article without a short description
  2. Use the "+ Add article description" button to provide a new description
  3. View the edit history and see that the edit has a summary "Added short description"

Update flow

  1. Find an article with a short description that could be improved
  2. Use the Pen button > "Edit article description" to update the description
  3. View the edit history and see that the edit has a summary "Updated short description"

Screenshots/Videos

philherbert avatar Feb 09 '24 17:02 philherbert

@philherbert Thanks for taking this on! This looks good to me so far. Whenever you're ready you can mark this PR as Ready for Review.

tonisevener avatar Feb 17 '24 00:02 tonisevener

@tonisevener Thanks! I'll mark it as Ready for Review shortly. A few things I'm unsure about:

  1. docs/localization.md says Scripts will automatically create proper entries in qqq.json and related files. - despite building the app and running tests etc, I haven't seen any modifications to qqq.json, and this PR doesn't include any. Are there additional steps I need to take to get those changes generated?

  2. The checks on this PR seem to require XCode Cloud to run - my XCode isn't letting me connect to XCode Cloud for some reason, so I may not be able to run those checks. Are there other ways of running them?

Thank you!

philherbert avatar Feb 19 '24 17:02 philherbert

@philherbert

docs/localization.md says Scripts will automatically create proper entries in qqq.json and related files. - despite building the app and running tests etc, I haven't seen any modifications to qqq.json, and this PR doesn't include any. Are there additional steps I need to take to get those changes generated?

Good catch! This is because our extract script is set up to only run on specific subdirectories, and ShortDescriptionController is one of a small set of files that resides in a subdirectory under Code. That is why historically our file structure is so flat. I made a separate Phabricator task to improve this - ideally the extract script shouldn't care about subdirectories. Feel free to tackle this as well if you want, but it's not a must for this PR.

One way around this is to move your WMFLocalizationStrings into CommonStrings.swift, then reference the static properties from ShortDescriptionController. Once you do that, upon build you should see some automatic strings file changes in git. Please commit those and push to this PR.

The checks on this PR seem to require XCode Cloud to run - my XCode isn't letting me connect to XCode Cloud for some reason, so I may not be able to run those checks. Are there other ways of running them?

Yeah, currently our Xcode Cloud setup isn't running on forks. You can run tests locally by selecting the Wikipedia scheme and typing Cmd + U. We'll confirm on our side as well before merging.

tonisevener avatar Feb 21 '24 14:02 tonisevener

Hi @philherbert - we have plans on adjusting this summary view soon. I can merge this PR as-is, then we will tackle feedback from https://github.com/wikimedia/wikipedia-ios/pull/4745#issuecomment-1956840513 as a part of our upcoming work. Let me know if you have objections, thanks!

tonisevener avatar Mar 20 '24 16:03 tonisevener

Hi @tonisevener - no objections here! Sorry that I forgot to update this, it very much fell through the cracks.

philherbert avatar Mar 20 '24 19:03 philherbert

Hi @tonisevener - no objections here! Sorry that I forgot to update this, it very much fell through the cracks.

No problem at all - thank you for working on it!

tonisevener avatar Mar 21 '24 20:03 tonisevener