Page MenuHomePhabricator

Rewrite AddMe gadget on meta
Closed, ResolvedPublic21 Estimated Story Points

Description

Background

The Community Wishlist Survey uses a fork of the AddMe gadget to make voting easier. A fork was required as the original AddMe didn't support our template-based setup.

The old AddMe also had a number of flaws, including but not limited to:

  • use of the deprecated jQuery.UI library
  • forces the user to browse to the page they voted on
  • annoying little animations

Acceptance criteria

  • Rewrite the AddMe script from scratch using OOUI.
  • You should be able to add votes and not have the page be reloaded, much like DiscussionTools.
  • The gadget should be testable in a survey sandbox environment (T319234).
  • No IE11 compatibility (or any IE for that matter)

Details

TitleReferenceAuthorSource BranchDest Branch
Simplify the dialog's action processrepos/commtech/add-me!2samwilsongithub/fork/samwilson/process-chainmain
Customize query in GitLab

Event Timeline

MusikAnimal set the point value for this task to 8.Oct 5 2022, 7:03 PM
Aklapper renamed this task from Rewrite AddMe to Rewrite AddMe gadget on meta.Oct 6 2022, 7:53 AM

@Quiddity Pinging you in case you have any input on what the new AddMe should do that the old one doesn't? I'll have a demo to share very soon :)

Requesting early feedback from CommTech devs!

Source code: https://gitlab.wikimedia.org/repos/commtech/add-me

  • Please feel free to make a PR for anything.
  • I'm happy to move this to Wikimedia's GitHub, if we want.
  • I'm specifically seeking input on AddMe.js#L225-L228. For some reason, OOUI's getActionProcess() isn't waiting for the submit() promise to finish. So, I'm doing .next( () => 500 ) to make it wait for 500 milliseconds (bad!), though it seems to work reliably. I'm sure I'm doing something wrong, but I can't figure it out...
  • I may eventually move the Dialog class and related code to a separate file, but I'm going to wait on that as it's so much easier to work using only one file during development (as @import doesn't work reliably in the browser, and it's a pain to have to recompile the JS on every change).

Usage:

@Quiddity Pinging you in case you have any input on what the new AddMe should do that the old one doesn't? I'll have a demo to share very soon :)

I forget any specifics of how the old version worked, but things that spring to mind to prioritize:

  • Translatability -- (clear docs on where translators should go, and what they need to do, to help)
    • Testing in RTL -- (of course!)
  • Re-usability -- (clear details on how to set-up a new page to utilize it, including links to 2 or 3 examples (which ideally utilize it in different ways))

That's all I can think of, for now, beyond your notes in the Description. :)

I'm getting local modifications when I run build; have made a PR: https://github.com/MusikAnimal/add-me/pull/1

For the process steps that are used in the dialog buttons, I've attempted to simplify it: https://github.com/MusikAnimal/add-me/pull/2

MusikAnimal changed the point value for this task from 8 to 21.Nov 17 2022, 12:10 AM

Changing estimate to encapsulate all the work that was put into this. Other tickets might follow if QA finds any problems.

QA notes:

  • This is the for the new voting gadget to be used in the Wishlist Survey, but it is also a replacement for the original AddMe which is meant to be multi-purpose, namely for grant proposals.
  • Refer to the new documentation for intended functionality.
  • You can do your testing over at https://test.wikipedia.org/wiki/Wikipedia:AddMe-test. The gadget should already be enabled by default.
  • The gadget messages have been translated to Spanish and Arabic, if you want to test multilingual and/or RTL. Note it is only the popup dialog itself that is translated, so you will still see English content on the test page.
  • If you want to test changing the configuration, I can give you admin rights on testwiki if you don't already have them. The "wishlist-survey" config is where we want it for the survey, so you shouldn't need to change that, but feel free to add a new AddMe project if you so desire.
  • Test as little or as much as you'd like. This gadget has a lot of functionality and I don't expect you to test everything :)

Let me know if you have questions, and many thanks for your time!

@MusikAnimal A couple of observations:

  • When I am the first user voting on something, I get two javascript errors:
Uncaught TypeError: mw.centralNotice.isBannerShown is not a function
load.php:378:138
    jQuery 10
    require https://test.wikipedia.org/w/load.php?lang=ar&modules=startup&only=scripts&raw=1&skin=vector-2022:9
    js jQuery
    runScript https://test.wikipedia.org/w/load.php?lang=ar&modules=startup&only=scripts&raw=1&skin=vector-2022:12
    execute https://test.wikipedia.org/w/load.php?lang=ar&modules=startup&only=scripts&raw=1&skin=vector-2022:13
    doPropagation https://test.wikipedia.org/w/load.php?lang=ar&modules=startup&only=scripts&raw=1&skin=vector-2022:6
    (Async: requestIdleCallback handler)
    requestIdleCallback self-hosted:1178
    setAndPropagate https://test.wikipedia.org/w/load.php?lang=ar&modules=startup&only=scripts&raw=1&skin=vector-2022:7
    implement https://test.wikipedia.org/w/load.php?lang=ar&modules=startup&only=scripts&raw=1&skin=vector-2022:18
    <anonymous> jQuery

and

Uncaught TypeError: startMarker is null
load.php:130:917
    jQuery 3
    forEach self-hosted:202
    jQuery 5
    reloadContent https://test.wikipedia.org/w/load.php?lang=ar&modules=ext.gadget.Carousel,CleanDeleteReasons,addMe,formWizard,geonotice,refToolbar,tabbedwindow,watchlist-notice&skin=vector-2022&version=mz46i:111
    jQuery 18
    reloadContent https://test.wikipedia.org/w/load.php?lang=ar&modules=ext.gadget.Carousel,CleanDeleteReasons,addMe,formWizard,geonotice,refToolbar,tabbedwindow,watchlist-notice&skin=vector-2022&version=mz46i:110
    reloadContent self-hosted:1178
    jQuery 18
    purgePage https://test.wikipedia.org/w/load.php?lang=ar&modules=ext.gadget.Carousel,CleanDeleteReasons,addMe,formWizard,geonotice,refToolbar,tabbedwindow,watchlist-notice&skin=vector-2022&version=mz46i:110
    getActionProcess https://test.wikipedia.org/w/load.php?lang=ar&modules=ext.gadget.Carousel,CleanDeleteReasons,addMe,formWizard,geonotice,refToolbar,tabbedwindow,watchlist-notice&skin=vector-2022&version=mz46i:120
    jQuery 33

I reproduced this by creating https://test.wikipedia.org/wiki/Wikipedia:AddMe-test/Foobar.

  • When I check the "Watch proposal page" it actually watches the /Voting subpage (e.g. Wikipedia:AddMe-test/Foobar/Voting).
  • After adding a vote (without refreshing page), if I click on the "Reply" next to the new vote the reply tools do not work. Instead, I see the warning jQuery.Deferred exception: threadItem is null. I guess it is unlikely a user will try to do this and, if they refresh the page, reply does work.
  • When voting a second time, the "Try again" button is a bit confusing. At first I assumed it was giving me the option to change my comment and submit again. I tried to do this, not realising it was already trying to submit my vote, and I ended up voting twice. Should it instead be something like "Submit anyway", and perhaps don't show the form again or disable the Support button.
    • Moreover, if I dismiss the duplicate vote message, if I submit again I don't see the message a second time. Perhaps this is as designed.

Also:

Thanks for the swift and thorough review!

@MusikAnimal A couple of observations:

  • When I am the first user voting on something, I get two javascript errors:

With help from the Editing team, this should now be fixed. The first error about CentralNotice banners is unrelated and seem to happen if there's any JavaScript error, by the way (T319498).

When I check the "Watch proposal page" it actually watches the /Voting subpage (e.g. Wikipedia:AddMe-test/Foobar/Voting).

That test page doesn't perfectly simulate the survey. In the actual survey, the "proposal page" is the one with the discussion and all the votes, and the proposal itself is yet another subpage (once it's set up for translation). So anyway in the actual survey, you'd be watching the desired page; that is, the one you placed your vote on that also has the discussion section. I just wanted to simulate the handling of transcluded subpages which is why I put the votes on a dedicated subpage.

  • After adding a vote (without refreshing page), if I click on the "Reply" next to the new vote the reply tools do not work. Instead, I see the warning jQuery.Deferred exception: threadItem is null. I guess it is unlikely a user will try to do this and, if they refresh the page, reply does work.

It shouldn't show an error anymore, but you do get a notice "Could not find the comment you're replying to on the page. It might have been deleted or moved to another page. Please reload the page and try again." This is apparently a bug with DiscussionTools. I've filed a task at T323661.

  • When voting a second time, the "Try again" button is a bit confusing. At first I assumed it was giving me the option to change my comment and submit again. I tried to do this, not realising it was already trying to submit my vote, and I ended up voting twice. Should it instead be something like "Submit anyway", and perhaps don't show the form again or disable the Support button.

I have the error set to be a warning, which is supposed to display "Continue" and not "Try again", but that for some reason isn't working.

Note however that it should not submit your vote if you click "Dismiss". Just wanted to make sure that was clear, as what you're saying almost made it sound like it submitted anyway, but I'm not seeing that behaviour.

  • Moreover, if I dismiss the duplicate vote message, if I submit again I don't see the message a second time. Perhaps this is as designed.

Yes, this is by design.

Also:

Good question. I've definitely seen voters add elaborate votes that include paragraphs, images, etc., so it's something I think we want to allow, but I'm not sure how to best handle the situation you describe. Newlines will cause the parser to start a new <ul> element, which is an accessibility problem.

For now I've decided to replace newlines with <p> tags. This will visually still appear like new lines but stay within the same <li> element, and hence will not leak outside the outer <ul> element. This trick seems to work, but will surely occasionally have unwanted side effects depending on what the user decides to put in their comment. Play around with it (if you want) and let me know what you think! Weighing it out in my head, it's probably better to guard against breaking the <ul> (something I used to fix manually over the years) than catering to a rare use case that needs newlines. Any complex wikitext, etc., should probably go in the "Discussion" section anyway.

  • If you support and your comment has a newline, if you try to vote again you are not warned about it being a duplicate (perhaps the regex does not like newlines?)

Following the aforementioned fix, this shouldn't happen anymore.

When I check the "Watch proposal page" it actually watches the /Voting subpage (e.g. Wikipedia:AddMe-test/Foobar/Voting).

That test page doesn't perfectly simulate the survey. In the actual survey, the "proposal page" is the one with the discussion and all the votes, and the proposal itself is yet another subpage (once it's set up for translation). So anyway in the actual survey, you'd be watching the desired page; that is, the one you placed your vote on that also has the discussion section. I just wanted to simulate the handling of transcluded subpages which is why I put the votes on a dedicated subpage.

@MusikAnimal How will it work in the survey? I tried to emulate a proposal from the previous wishlist and created https://test.wikipedia.org/wiki/Wikipedia:AddMe-test/Wish_1 and https://test.wikipedia.org/wiki/Wikipedia:AddMe-test/Wish_2 but I cannot support either of them.

For example, when trying to support Wish_2 from the main page, I get a nosuchsection error in the popup and the API returns { code: "nosuchsection", info: "There is no section 1.2." ...

When trying to support from the Wish_2 proposal page itself, but get There was an error with the AddMe gadget: The 'data-addme-page' attribute is not a subpage of the current page. Please report this issue at Meta talk:AddMe.

Below is the API request when trying to support from the main page. I wonder if the section should be 3 rather than 1.2.

action=edit
format=json
title=Wikipedia:AddMe-test/Wish_2
section=1.2
summary=Support proposal
starttimestamp=2022-11-23T07:54:04Z
nocreate=true
watchlist=nochange
appendtext=
* {{support}}  ~~~~

Note however that it should not submit your vote if you click "Dismiss". Just wanted to make sure that was clear, as what you're saying almost made it sound like it submitted anyway, but I'm not seeing that behaviour.

No, clicking Dismiss just returned me to the form.

How will it work in the survey? I tried to emulate a proposal from the previous wishlist and created https://test.wikipedia.org/wiki/Wikipedia:AddMe-test/Wish_1 and https://test.wikipedia.org/wiki/Wikipedia:AddMe-test/Wish_2 but I cannot support either of them.

For example, when trying to support Wish_2 from the main page, I get a nosuchsection error

I wonder if the section should be 3 rather than 1.2.

Fixed! I was using the wrong data from the action=parse response. "1.2" refers to the visual numbering (depending on the skin), whereas we actually wanted the index property which in this case is indeed 3.

When trying to support from the Wish_2 proposal page itself, but get There was an error with the AddMe gadget: The 'data-addme-page' attribute is not a subpage of the current page. Please report this issue at Meta talk:AddMe.

I've deployed a change that allows the addme-page to be the current page (and not just subpages), so this example should work now. There's no reason to specify the current page as that's the default, but it seems sensible to allow it.

In the survey, all the buttons have the data-addme-page attribute set, and it's always to a subpage. Looks like I did a poor job at recreating the proposal pages, huh? ;) Sorry about that! But you found bugs in the process, which is great.

Thanks for the awesome QA as always!

When trying to support from the Wish_2 proposal page itself, but get There was an error with the AddMe gadget: The 'data-addme-page' attribute is not a subpage of the current page. Please report this issue at Meta talk:AddMe.

I've deployed a change that allows the addme-page to be the current page (and not just subpages), so this example should work now. There's no reason to specify the current page as that's the default, but it seems sensible to allow it.

Thanks. I am able to support them from the subpage. But I cannot support them from the top page (Wikipedia:AddMe-test).

For Wish_1, if I have not voted before I get a nosuchsection error in the popup. The POST request is below, and appears to be trying to edit the current page:

action=edit
format=json
title=Wikipedia:AddMe-test
section=T-1
summary=Support proposal
starttimestamp=2022-12-02T06:57:31Z
nocreate=true
watchlist=nochange
appendtext=
* {{support}}  ~~~~

If I have voted before and I click "Try again", it supports Proposal_0, the POST request being:

action=edit
format=json
title=Wikipedia:AddMe-test/Proposal_0/Voting
section=1
summary=Support proposal
starttimestamp=2022-12-02T07:07:58Z
nocreate=true
watchlist=nochange
appendtext=
* {{support}}  ~~~~

For Wish_2, I get a popup: There was an error with the AddMe gadget: The 'data-addme-page' attribute is not a subpage of the current page. Please report this issue at Meta talk:AddMe.

Thanks. I am able to support them from the subpage. But I cannot support them from the top page (Wikipedia:AddMe-test).

Fixed! So when I changed it so that the data-addme-page was allowed to be the same as the current page (T319463#8434478), I actually broke the transclusion support part! Careless error, now fixed.

Let me know if all looks OK now on your end.

Oh, another thing I wanted to mention: lately I've noticed race conditions where AddMe isn't loading properly and there are JS errors in the console. This seems to only happen when serving from my localhost (i.e. during development), and not with the gadget, so I think it's fine and/or somewhat expected, but I thought I'd mention it just in case you see such issues. In my case, simply refreshing usually fixes it. If it happens with the gadget, though, we definitely have a problem.

@MusikAnimal One thing I notice voting on Wikipedia:AddMe-test is the memory usage increases each time I vote, sometimes by as much as 15mb per vote (on Firefox). Some of this does get recovered eventually, but not all of it. I don't know if this represents a big issue. I don't know how many wishes a user is likely to support for each category without refreshing.

Thanks. I am able to support them from the subpage. But I cannot support them from the top page (Wikipedia:AddMe-test).

Fixed! So when I changed it so that the data-addme-page was allowed to be the same as the current page (T319463#8434478), I actually broke the transclusion support part! Careless error, now fixed.

Let me know if all looks OK now on your end.

Thanks, I can now vote from the top page. So the expectation is that you should always set data-addme-page if the voting is being done from a different page? Should we document that?

Oh, another thing I wanted to mention: lately I've noticed race conditions where AddMe isn't loading properly and there are JS errors in the console. This seems to only happen when serving from my localhost (i.e. during development), and not with the gadget, so I think it's fine and/or somewhat expected, but I thought I'd mention it just in case you see such issues. In my case, simply refreshing usually fixes it. If it happens with the gadget, though, we definitely have a problem.

I have not seen this yet so far on testwiki.

@MusikAnimal One thing I notice voting on Wikipedia:AddMe-test is the memory usage increases each time I vote, sometimes by as much as 15mb per vote (on Firefox). Some of this does get recovered eventually, but not all of it. I don't know if this represents a big issue. I don't know how many wishes a user is likely to support for each category without refreshing.

Interesting! The live refresh code is more or less copied verbatim from DiscussionTools. I did a quick test and the Reply tool also consumes about 15MB per refresh.

In the survey, we try to ensure no single category is too large, so if we find DiscussionTools and AddMe are really slow we may have to move proposals. I believe the largest category we've had is Larger suggestions. There DiscussionTools doesn't seem to work at all, and AddMe likely won't either. I think most users will sort of grasp that because it's such a large page, some things are bound to fail. The Larger suggestions page by itself (without any interaction) causes my browser to go really slow! If we need it, we can create a "Larger discussion 2" page or whatever.

Thanks, I can now vote from the top page. So the expectation is that you should always set data-addme-page if the voting is being done from a different page? Should we document that?

Yep! I've added some clarity around this with https://meta.wikimedia.org/?diff=24273911

OK, thanks. I think that is all from me.

@GMikesell-WMF did you have anything more you wanted to do or shall I move this into Product Sign-off?

@dom_walden I did not come across anything else. I'm ok for you to move it to Production Sign-off.

NRodriguez subscribed.

Wow, the adrenaline of Resolving a 21 pointer ticket.