Page MenuHomePhabricator

Implement a proper code-review process for MediaWiki JS/CSS pages on Wikimedia sites
Open, LowestPublic

Assigned To
None
Authored By
Legoktm
Aug 12 2014, 7:44 PM
Referenced Files
F14648: readAndEditWikipediaLangs.txt
Nov 22 2014, 3:41 AM
Tokens
"Love" token, awarded by SD0001."Love" token, awarded by TerraCodes."The World Burns" token, awarded by Liuxinyu970226."Like" token, awarded by MGChecker."Love" token, awarded by Huji."Yellow Medal" token, awarded by Ltrlg."Orange Medal" token, awarded by Krinkle."The World Burns" token, awarded by yuvipanda.

Description

MediaWiki: CSS/JS pages on any non-large wiki are usually a mess. Occasionally, we'll discover that wikis have been loading external resources for months and no one noticed. In addition, the local sysops maintaining those pages usually don't know JavaScript and are copy-pasting what someone else told them to do. Even when that's not the case, mistakes can result in code with obvious syntax errors being sent to readers for long periods of time.

Various proposals have floated around over the years. The wikitech-l threads have some good discussion on some of the reasons why this is difficult for smaller wikis.

See Also:

Details

Reference
bz69445

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Would it be more secure to possibly add in a few more custom user permissions such as splitting editinterface into editJS and editCSS with also adding in restrictions for template namespace and editLua for Lua? I know that edituserjs&css was split into edituserjs and editusercss. This way the mediawiki namespace would contain the interface messages; all javascript is restricted to .js and users with editJS, all css is restricted to .css and users with editCSS; all lua modules restricted by editLua; possibly restrict templates to editTemplate. Just a suggestion...

From what I remember that was already done in Gadgets-2.0.

There also is Extension:CodeReview but status is historical reference, perhaps reopening that for development would be a potential solution?

There also is Extension:CodeReview but status is historical reference, perhaps reopening that for development would be a potential solution?

CodeReview is perfectly stable and usable and (mostly) feature-complete. The {{Historical}} template was added to that page by someone who doesn't appear to be familiar with the extension. There is absolutely nothing preventing you from installing and using CodeReview -- or if there is, that's a bug and I'd love to hear about it so that we can get it fixed! Just because something isn't used on Wikimedia's wikis anymore doesn't mean it's flat-out obsolete and/or unusable.

All that being said, CodeReview is mostly a web UI for a Subversion (SVN) repository. The workflow is essentially different when it comes to a SVN repository as opposed to editing on-wiki .css and .js pages: changes to the latter are live immediately, whereas code changes committed to a SVN repo need to be manually deployed by someone.
It is my firm belief that creating a largely new system for "code reviewing" on-wiki .css and .js pages would be the easiest and cleanest solution instead of trying to repurpose CodeReview. In any case, please feel free to add me as a reviewer to CodeReview patches and I'll be happy to review 'em (unless someone else beats me to it, that is).

CodeReview + FlaggedRevs is a possible way to handle this task, although not a good one. SVN-style "linear" history sucks for code review (which is why pretty much every single projetc abandoned it when DVCS-s became mainstream), and implementing non-linear history in MediaWiki does not sound like fun, so the most promising approach is to make MediaWiki sync with the master branch of some remote repo, after which there is no point in rebuilding the code review interface in MediaWiki, since there are plenty decent ones out there.

I think relying on Git is a good idea, but I think you underestimate that by moving contributing to these files to a git, you remove it from direct access from the wiki. I think it will feel like we're taking away control form the community, even if this isn't the intention, as we in some way remove the direct editing possiblity from the wiki and make it more difficult to contribute there for normal user.

We have to find a compromise to minimize this problem, because this wouldn't ever be accepted otherwise. (Many users still remember the js wheel-war between WMF and community at German Wikipedia to well, even if it's been a long time ago and many things have changed.)

another possibility outside of Gadget 2 is a test wiki where any JS, CSS or Lua is tested and then moved over to the main WMF wikis after having a +2 code reviewer approve merging it over. The MediaWiki namespace is mostly interface messages, restricting JS & CSS editing to new permissions might make this safer as well; editusercssJS was split into separate permissions though seems to be limited use cases (someone breaking personal js/css).

Tgr removed a project: User-Tgr.
In T71445#3613657, @Tgr wrote:

the most promising approach is to make MediaWiki sync with the master branch of some remote repo, after which there is no point in rebuilding the code review interface in MediaWiki, since there are plenty decent ones out there.

Now filed as T187749: Make it possible to use code from an external repository for editor-controlled Javascript/CSS.

I know that a while ago it was mentioned that Extensions Flagged Revs + Code Review could be a potential solution. I tested yesterday on a small wikifarm and Flagged Revs does not work on the MediaWiki namespace at all, pretty sure I configured correctly. I did not grant myself the autoreview permission and did not use any of the default user groups. Any valid or invalid javascript added to MediaWiki:common.js went live immediately. Trying to hardcode "NS_MEDIAWIKI" into extension array resulted in an internal error at Special:UnreviewedPages. Extension:Approved Revs explicitly says that it doesn't work on MediaWiki namespace.

Edit: "addportlet" error was my own script, not Flagged Revs

I know that a while ago it was mentioned that Extensions Flagged Revs + Code Review could be a potential solution.

For the records, CodeReview is unmaintained so you'd have to solve T205482 first.

thanks for letting me know about T205482. Given that Flagged Revs gave me an internal error (VM1844:1 Uncaught TypeError: Cannot read property 'addPortletLink' of undefined) for trying to add in MediaWiki namespace, I do not think that placing effort and time into solving T205482 is worth it.

probably better off creating a new extension either from scratch or reusing elements from Extension Flagged Revs and Moderation. Simple Extension that has Approve/Reject buttons with a special page listing unreviewed .js, .css, Gadgets and preventing unreviewed from going live would be enough for basic usage. Also would allow any wiki to use it, not just WMF.

Last I checked, this task stalled out on deciding a solution.

Implementation with current technical options:

  1. Have a script or gadgets at Foo
  2. Copy the (then-current) version to Bar (Equivalent of branching)
  3. Apply any suggested edits
  4. Request that the edits be merged in (Equivalent of opening a pull request)
  5. Move Bar to Foo, deleting Foo in the process (Equivalent of merging a pull request)
  6. Restore old revisions of Foo (to provide continuity of history)

This would

  • Provide attribution to users who suggest the edits (since they are still credited with the revision)
  • Not require new development (perhaps some user scripts to simplify things, but not strictly speaking needed)

Thoughts?

@DannyS712 isn't that the current system that Wikipedia already uses?

Your branching/pull request concept is great! While using existing namespace could work, the need to delete and move seems messy. I think instead of having all of this within the MediaWiki namespace, a second namespace could be utilized. Keeps it much cleaner, though this would require new development resources...

MediaWiki namespace scripts/gadgets would be the version that is activated for users/visitors.
Creation of a new namespace such as "MediaWiki_Staging" would be where changes are made to the scripts/gadgets as a sort of "staging" area, not served to users/visitors.

any scripts/gadgets from MediaWiki that need changes could then be cloned/pulled into MediaWiki_Staging by click of a button. When ready to submit changes, user clicks a "submit" button to send to reviewers (new user permission) and a status message is set to "Pending Review". Page would be locked from editing or if an edit is made, status is reset to nothing and review request is canceled.

A special page to list all the pending changes to scripts/gadgets would provide reviewers option to approve or reject with reason.

  • Approve would set status to "Accepted", latest revision is copied to script/gadget in MediaWiki namespace
  • Reject would reset status back to "Failed", no changes occur

side note: based on recent changes to edit access to js, json, css in MediaWiki namespace, It would make more logical sense and add additional protections to add in a configurable variable that acts as a whitelist regarding which of the system messages can be modified, on average only like 10% are ever modified.

Actually, editinterface has been around for a long time. It allows editing of all MediaWiki: namespace pages as they are not all super sensitive, the css and js extra permissions were created because js and css pages in the MediaWiki: namespace and elsewhere are more sensitive than the average MediaWiki namespace page.

@JEumerus clarified my side note, yeah it was confusing. I was meaning to refer to how js, json, css pages under MediaWiki were split out, did not mean to suggest that EditInterface was a new permission.

I think what is needed is forking, not branching.

  1. Copy pages (make a fork). This is not available AFAIK. Sure you can copy text but that doesn't preserve history.
  2. Merge pages (pull request). Kind of available but tricky. You can do some tricks with delete and restore IIRC, but that requires admin rights and is complicated anyway.

Copying would have to be made available to standard users (e.g. user copies Mediawiki:common.css to User:Example/common.css). Then an admin could merge "User:Example/common.css" back to "Mediawiki:common.css" (visually e.g. by reviewing it as an edit).

Note that both operations would actually be useful for articles as well. You sometimes need to merge articles and there is no nice way to do that.

If we allow forking these to user css/json/js pages, when merging back there could be edit conflicts considering another edit could have been merged and the user didn't pull. How would you suggest keeping a fork to user namespace be kept up to date, might need to keep track of diff changes - this gets complicated? Sending the fork to another namespace and keeping it in one location is much better. I am suggesting a simpler system due to these projects not being finished ie partial blocks was meant to become multi-blocks.

For general issues with forking/merging pages, see T113004.

If we allow forking these to user css/json/js pages, when merging back there could be edit conflicts considering another edit could have been merged and the user didn't pull. How would you suggest keeping a fork to user namespace be kept up to date, might need to keep track of diff changes - this gets complicated?

The problems are the same if users copy the page text. The difference you don't even know which version user copied. You also don't know if user copied text and saved and then did his/her changes or changed something while copying (and saved with the changes).

So if you are merging you don't worry about that. You are reviewing changes as you see them. You either reject or accept them.

Optionally the system can warn you (as a reviewer) that changes were forked before other changes were added. You can then reject and inform user about problems. The user then need to re-fork and re-apply changes. This is also similar to how you can resolve problems on Github.

@Nux Wouldn't a simply way to utilize another namespace where the script or gadget is copied too and anyone that is authorized to make changes only touch that copied version? This way there wouldn't be any merge conflicts when merging back upon reviewer approval and the scripts/gadgets in MediaWiki namespace would be locked as production - live for viewers/users.

Quick idea that just came to me for an extension + workflow (extension part highlighted)

  1. User forks a script or gadget in own user space
  2. User makes desired modifications
  3. Others test, comment, etc. with normal talk pages
  4. Interface administrator "approves" the edit (changes can be seen via Special:ComparePages)
  5. Interface administrator visits Special:MergeCode
    1. provides the page title to be modified
    2. provides and the permanent link to the revision of the fork that was approved (in case the page is modified in the middle)
    3. provides a comment to be added to the automatic summary
    4. provides the name of the user who wrote the new code being merged in
    5. Clicks merge:
    6. New revision is saved for the target page, implementing the edit request by copying the content from the approved revision. Edit is attributed to approving interface admin (in terms of whose contributions it shown up in, page history, etc.) but automatic summary notes the original author

This would all be possible within a special page. It would:

  • Ensure that only users that can edit the actual gadget/mediawiki ns page/script can edit it using the special page
  • Provide attribution to the original author of the code (user who forked)
  • Not handle "merging" due to changes made in the script since the fork was created, but the user can just update their fork code; also doesn't deal with official branching, rebasing, etc.

Thoughts? If this had support, I might apply for a rapid grant to write such an extension - shouldn't take too long for a working version to be created

This is an N-of-one experience but I thought I should still share it:

Recently, we decided that the version(s) of Twinkle that we had on fawiki were very old and not worth maintaining and patching; therefore, we decided to pull the latest version of Twinkle and localize it. We put the original code (which is in English, and has some enwiki-only features that we needed to modify or turn off) in the MediaWiki namespace, and made a gadget out of it. Then we also copied the code for each module in Draft namespace (and changed content model to JS) and a group of around 6-7 people started localizing the code. Those who know JS well helped with all aspects of localization, but those who don't only focused on translating Twinkle interface messages from English to Persian.

In your scenario, this is akin to a single fork, in which multiple users are contributing. So regarding your point 5.F above, note that more than one user may need to be attributed.

Each time the forked (Draft) version was merged into the MediaWiki version (by yours truly, and using manual code review + Preview feature of MediaWiki), we synced the Draft version too; that is, if I found an error in the Draft, I fixed it both in the Draft and in the MediaWiki version. If the Draft was further revised from then on, we had to merge again, and we would not want to attribute every contributor again (only those who contributed since the sync). You could argue the forked version in Draft should have been deleted and recreated to have clean history, but I think that is extra work.

I can think of other ways your proposal may not be ideal, but for now, I will only focus on the issue of "multiple contributors in the 'forked' version". I like where you are going with this, but I think your model is too simple right now.

I'm not keen on the amount of work left in the hands of the merging interface admin in your proposal, @DannyS712. In particular, for an extension it should be possible to mark the forked pages as forks and indicate what page they're forked from, and have some mechanism to mark a particular revision as approved, at which point the special page could pull most of the required info automatically from the fork when an iadmin merges it back in. If it's a concern, the special page could allow certain fields to be modified pre-merge (e.g. tweaking the summary), but in general I feel much more comfortable not requiring the iadmin to manually copy so much (less chance for errors to creep in, etc).

I'm not keen on the amount of work left in the hands of the merging interface admin in your proposal, @DannyS712. In particular, for an extension it should be possible to mark the forked pages as forks and indicate what page they're forked from, and have some mechanism to mark a particular revision as approved, at which point the special page could pull most of the required info automatically from the fork when an iadmin merges it back in. If it's a concern, the special page could allow certain fields to be modified pre-merge (e.g. tweaking the summary), but in general I feel much more comfortable not requiring the iadmin to manually copy so much (less chance for errors to creep in, etc).

Interface admin only copies the approved revision id and the target page, plus its registered as a normal edit and so can easily be reverted.

User forks a script or gadget in own user space

That would require a special page too. There should be some automatic information about forked version (at least as a oid link in the version history).

The fork information should then be available when merging/comparing.

(completely forgot about this task)

In T71445#6265776, @Nux wrote:

User forks a script or gadget in own user space

That would require a special page too. There should be some automatic information about forked version (at least as a oid link in the version history).

The fork information should then be available when merging/comparing.

Sure, the automatic information at the start would be easy, though not sure about using that when merging/comparing since merging/rebasing may be an issue

Quick idea version 2 for an extension + workflow (extension part highlighted)

  1. User wants to fork a script or gadget to propose a change
    1. User visits Special:ForkCode, says what the source page is and what the target page is
    2. Current content of source page is copied to target page, with a summary giving the needed attribution
  2. User makes desired modifications
  3. Others test, comment, etc. with normal talk pages
  4. User submits edit request by requesting on the script/gadget talk page something like "Please replace with the code currently at User:Me/code.js, which will implement the new feature ABC. This has been tested and consensus supports including the new feature."
  5. Interface administrator uses Special:ComparePages to see what the changes are, and, if they agree to make the edit, continues
  6. Interface administrator visits Special:MergeCode
    1. provides the page title to be modified
    2. provides and the permanent link to the revision of the fork that was approved (in case the page is modified in the middle)
    3. provides a comment to be added to the automatic summary
    4. provides the name of the user who wrote the new code being merged in
    5. Clicks merge:
    6. New revision is saved for the target page, implementing the edit request by copying the content from the approved revision. Edit is attributed to approving interface admin (in terms of whose contributions it shown up in, page history, etc.) but automatic summary notes the original author. Something like "Updating with code written by User:FooBar. (rest of edit summary with explanation)

This would all be possible within the special pages. It would:

  • Ensure that only users that can edit the actual gadget/mediawiki ns page/script can edit it using the special page
  • Provide attribution to the original author of the code (user who forked)
  • Not handle "merging" due to changes made in the script since the fork was created, but the user can just update their fork code; also doesn't deal with official branching, rebasing, etc.
  • Not require database tables, special user rights, integration with external tools, etc.
Remagoxer renamed this task from Implement a sane code-review process for MediaWiki JS/CSS pages on Wikimedia sites to Implement a proper code-review process for MediaWiki JS/CSS pages on Wikimedia sites.Jun 26 2022, 3:27 PM

Quick idea version 2 for an extension + workflow

Some quick "diff" proposals for your idea (below), with the caveat that I am not convinced this is the way to go.

With the existence of a WMF Gitlab instance in this equation, I'm inclined to think the vast majority of this task's scope should be implemented there, and then it is how to connect the wiki side to the off-wiki (i.e. Gitlab) process that is the main problem. For example, being able to have decision artefacts on wiki, holding discussions on wiki, and seeing changes in the on wiki watchlist. We need some way to gain the other stuff without losing those bits (because they are critical to the long tail of contributors who will never visit Gitlab and have no clue what it is).

  1. User wants to fork a script or gadget to propose a change
    1. User visits Special:ForkCode, says what the source page is and what the target page is

"User selects the Fork option in the page actions menu, right next to the Move option. The page contents is copied to the same base page name as a subpage of that user's user page. If there is an existing page it is overwritten and there is no option to choose a different name (but the forking can always be done manually as before). A page property/special MCR slot/ad hoc edit summary syntax gives the source page for both attribution and subsequent merging reasons."

It could still be implemented as a special page (that can be used manually), but for the main use case it should be hidden behind some suitable UI and manual user inputs automated.

  1. User submits edit request by requesting on the script/gadget talk page something like "Please replace with the code currently at User:Me/code.js, which will implement the new feature ABC. This has been tested and consensus supports including the new feature."

"User requests merge using the Merge command in the page actions menu. The page to be merged to is extracted from page property/special MCR slot/ad hoc edit summary syntax. The code to be merged is saved as a flagged revision in the target page (NB: clashes with permissions-based access control, may be hard to solve) with a edit summary identifying the source of the merge request for both attribution and review purposes. Location of relevant discussion and consensus etc. is left as a social construct on the local wiki (the sandbox talk page, a central technical or general village pump, etc.)."

  1. Interface administrator uses Special:ComparePages to see what the changes are, and, if they agree to make the edit, continues

"Interface administrator sees there are flagged revisions notice on their watchlist, and uses ordinary diff tools to see what the changes are. Presumably a link in the edit summary will lead them to any relevant discussions."

  1. Interface administrator visits Special:MergeCode

"Interface admin uses the flaggedrev tools to accept or reject the change, possibly including a manual review comment (possibly identifying the "main author" of the code being merged, but source page link and rev history attribution makes that unnecessary).

This would all be possible within the special pages.

Yes, but with some lightweight UI / process sugar to make it a lot less tedious and prone to human mistakes.

  • Not require database tables, special user rights, integration with external tools, etc.

Modulo the clash between flaggedrevs and rights-based access control that I am uncertain would be reconcilable without major surgery.

@DannyS712 @Xover
It would seem that this proposed extension should just perform the functionality. This will likely require installation of Extension:FlaggedRevs or implementing a similar lightweight functionality for the new special page.
As for handing merging, I propose using Special:MergeHistory to combine the specific revision approved for merge into the original page in MediaWiki namespace. Since this would lockout editing the mediawiki namespace pages, there would be less conflicts. This would retain the historical revision and edit summary in page history as attribution. As for what happens to the forked page to user subpage, that's for the wiki community to handle as their policy.
When forking, there should be some coordination to avoid edit conflicts or loss of code functionality.

  • Forking user is responsible for maintaining up to date with original page to avoid overwrite. if it does overwrite, previous revision should remain in page history.

As for the rights based access issue

  • Handled by restricting access to the special page with a new user permission.
  • When a user who is allowed access to the special page, submits a fork or merge request, extension will check if the user has all these permissions: editinterface, editsitecss, editsitejs, editsitejson. Instead of relying on default usergroups [Interface Administrator], this would allow anyone to use the extension.

    The big question for merging is do we want just the final version of the updated working code or all the subsequent trial & error historical revisions/edit summary?