http://newsletter-test.wmflabs.org/wiki/Special:ManageNewsletter

• it isn't clear how the newsletters are ordered
• permissions model is binary? publisher or not? this seems a bit weird...
• how many publishers can a single newsletter have?
  • how do I add or remove a publisher?
• action column is large unused, except for a single remove button?
  • okay, so the answer here is possibly to combine both table columns ("Permissions" and "Action") into a single "Actions" column, where "publisher" turns into a pencil icon or something, maybe grayed out if the user doesn't have publisher permission, and the "remove" button becomes a trash can icon or an "x"
• not clear what the contributors column is... single username?
  • not sure this column is needed at all
• it isn't possible to order any of the table columns
• how does this scale when there are 500 newsletters? is there pagination support?
• not sure "announce this newsletter" should live under "manage newsletters"... there should probably be a dedicated Special page for this?
• "title of issue's main page" text input should support page autocompletion using AJAX
• newsletter names should be links to the Special:Newsletter/1234 pages

http://newsletter-test.wmflabs.org/wiki/Special:Newsletter/11

• why are we outputting the newsletter ID? who cares?
• how do I edit any of the fields???
• could redo key:\n value layout with a simple sentence at the top saying "FooBar is a yearly newsletter about [[main page]]. This newsletter has 45 subscribers and is published by UserA and UserB."; this might be less awkward and easier to understand quickly
  • "Main page" probably doesn't need a "hist" link?

http://newsletter-test.wmflabs.org/wiki/Special:CreateNewsletter

• frequency input is awkward; probably shouldn't default to other with a text input
• not clear why the frequency field exists
• "Title of Main Page" should support page title autocompletion using AJAX
• if all fields are required, say so initially
• how is name of newsletter distinct from newsletter main page; are both fields needed?
• description field seems to be plaintext; needs further thought

http://newsletter-test.wmflabs.org/wiki/Special:Newsletters

• instructions say preferences can be changed at Special:Preferences... is this true currently?

• http://newsletter-test.wmflabs.org/wiki/Special:ManageNewsletter says "Contributors" while http://newsletter-test.wmflabs.org/wiki/Special:Newsletter/2 says "Publisher"; pluralization mismatch; word choice mismatch?
• You may subscribe to this newsletter by pressing the "Subscribe" button if you wish to do so. <-- this instruction really shouldn't be needed, just include the subscribe button again at the bottom? or move the button to the bottom?
• intro text at http://newsletter-test.wmflabs.org/wiki/Special:Newsletter/2 is awkward ("This page allows you to view details about specific newsletters. There is also an overview of all newsletters on this wiki."); none of that text should be necessary...
• why is "Subscriber count" at http://newsletter-test.wmflabs.org/wiki/Special:Newsletters a text input?
• why are newsletters with 0 subscribers listed at http://newsletter-test.wmflabs.org/wiki/Special:Newsletters ? maybe they should be...
• "A newsletter with the same name already exists. Try again with another name" error appears when "Title of Main Page" is already reserved; this error message is confusing and wrong; the newsletter name is not already taken, the main page is, right?
• successful submission of http://newsletter-test.wmflabs.org/wiki/Special:CreateNewsletter should probably just lead directly to http://newsletter-test.wmflabs.org/wiki/Special:ManageNewsletter with the success note at the top of the page, probably
  • or go directly to Special:Newsletter/12345

• "Description" field at http://newsletter-test.wmflabs.org/wiki/Special:Newsletters is output without sanitization
  • XSS issue for sure
• frequency text input allows weird inputs such as "after the twelfth hour of the twelfth day of the t"
• offered a non-functioning "Delete" button at http://newsletter-test.wmflabs.org/wiki/Special:Newsletter/16
  • publisher can't delete own newsletters?
  • don't offer a button a user can't press
    • results in permissions error for me
• http://newsletter-test.wmflabs.org/wiki/Special:ManageNewsletter <-- what does remove button do? seems to make table row temporarily disappear only

Thanks for this!

Note that we're going to kill the ManageNewsletter eventually and merge its functionalities with Special:Newsletter.

In T116353#1747410, @MZMcBride wrote:

• "Description" field at http://newsletter-test.wmflabs.org/wiki/Special:Newsletters is output without sanitization
  • XSS issue for sure

Has been fixed.

• publisher can't delete own newsletters?

Has been fixed. It did work originally and was broken due to a silly logic error.

In T116353#1747408, @MZMcBride wrote:

• how many publishers can a single newsletter have?

There isn't any limit and I don't think it makes sense to add a limit?

• "title of issue's main page" text input should support page autocompletion using AJAX

Makes sense to implement this in core as we now use HTMLTitleTextField. Filed T117033: HTMLTitleTextField should support ajax autocompletion.

• how do I edit any of the fields???

This will be implemented in the future.

• instructions say preferences can be changed at Special:Preferences... is this true currently?

Yes, in the Echo section of preferences. Users can choose either email or web notifications or both.

Change 249792 had a related patch set uploaded (by Florianschmidtwelzow):
Enable Title and User suggestions on Special:ManageNewsletter

https://gerrit.wikimedia.org/r/249792

In T116353#1747408, @MZMcBride wrote:

http://newsletter-test.wmflabs.org/wiki/Special:ManageNewsletter

• it isn't clear how the newsletters are ordered

https://phabricator.wikimedia.org/T117043

• permissions model is binary? publisher or not? this seems a bit weird...

We did have 'owners' and 'publishers' earlier.. but then we removed the owners and decided to give the sysops the 'newsletter-addpublisher' permission because the roles of owner were limited.

• how many publishers can a single newsletter have?
  • how do I add or remove a publisher?

You can remove a publisher in Special:ManageNewsletter using the 'Remove' button you see on the 'Action' column. Only a publisher or a sysop can do this action.

• not clear what the contributors column is... single username?
  • not sure this column is needed at all

List the publishers or those who contribute to the newsletter.

• another instance of not sanitizing output at http://newsletter-test.wmflabs.org/wiki/Special:Newsletter/24/unsubscribe
  • if the newsletter name contains arbitrary HTML such as <b>, the name gets output into the HTML DOM directly without sanitization
  • validate all user input and sanitize all user output!
• if you insert "Special:BlankPage" into the "Title of Main Page" text field at http://newsletter-test.wmflabs.org/wiki/Special:CreateNewsletter, the error message tells you "Special:BlankPage does not exist", where "Special:BlankPage" is a blue link; this is weird behavior
• might be nice to consider using Special:Newsletter/Create, Special:Newsletter/Manage, etc. instead of Special:CreateNewsletter, Special:ManageNewsletter, etc. in order to reduce polluting the Special namespace... or maybe not, probably doesn't matter

Poking around again briefly, it looks like there's still plenty of work to be done here; lots of the feedback from this task is still unresolved. I can revisit this task again in a few weeks if there's interest in having another review.

In T116353#1770776, @MZMcBride wrote:

• another instance of not sanitizing output at http://newsletter-test.wmflabs.org/wiki/Special:Newsletter/24/unsubscribe
  • if the newsletter name contains arbitrary HTML such as <b>, the name gets output into the HTML DOM directly without sanitization

It shouldn't allow scripts to be executed but it allows wikitext because it goes through Message's parse(). I'm actually not sure if we should be allowing random text in name field though. Needs more thought. Either way, it shouldn't be parsed like that.

• validate all user input and sanitize all user output!

It shouldn't allow scripts to be run now. Tracked in T110491 and T116382.

• if you insert "Special:BlankPage" into the "Title of Main Page" text field at http://newsletter-test.wmflabs.org/wiki/Special:CreateNewsletter, the error message tells you "Special:BlankPage does not exist", where "Special:BlankPage" is a blue link; this is weird behavior

This is the default message thrown by HTMLTitleTextField from core. I have submitted https://gerrit.wikimedia.org/r/250184 for this.

• might be nice to consider using Special:Newsletter/Create, Special:Newsletter/Manage, etc. instead of Special:CreateNewsletter, Special:ManageNewsletter, etc. in order to reduce polluting the Special namespace... or maybe not, probably doesn't matter

Related to T107555: Consistency of names across Newsletter extension special pages. Sort of.

Poking around again briefly, it looks like there's still plenty of work to be done here; lots of the feedback from this task is still unresolved. I can revisit this task again in a few weeks if there's interest in having another review.

Yes, I agree that there's still work that needs to be done before deployment and will start to work on major improvements after next week (if doesn't get resolved by then!) as I will have more time then. Additional feedback is always welcome!

In T116353#1771167, @Glaisher wrote:

In T116353#1770776, @MZMcBride wrote:

• if you insert "Special:BlankPage" into the "Title of Main Page" text field at http://newsletter-test.wmflabs.org/wiki/Special:CreateNewsletter, the error message tells you "Special:BlankPage does not exist", where "Special:BlankPage" is a blue link; this is weird behavior

This is the default message thrown by HTMLTitleTextField from core. I have submitted https://gerrit.wikimedia.org/r/250184 for this.

It seems like this field should be setting 'creatable' => true so special pages aren't accepted?

In T116353#1771639, @Legoktm wrote:

It seems like this field should be setting 'creatable' => true so special pages aren't accepted?

Yes, thanks. I didn't know that this existed and thought that MZMcBride referred to it being linked for non-existent pages.

Change 250462 had a related patch set uploaded (by Glaisher):
Switch Special:CreateNewsletter to use OOUI form

https://gerrit.wikimedia.org/r/250462

Change 250463 had a related patch set uploaded (by Glaisher):
Remove 'id' field and 'hist' link from Special:Newsletter

https://gerrit.wikimedia.org/r/250463

Change 250462 merged by jenkins-bot:
Switch Special:CreateNewsletter to use OOUI form

https://gerrit.wikimedia.org/r/250462

Change 250463 merged by jenkins-bot:
Remove 'id' field and 'hist' link from Special:Newsletter

https://gerrit.wikimedia.org/r/250463

Change 249792 merged by jenkins-bot:
Enable Title and User suggestions on Special:ManageNewsletter

https://gerrit.wikimedia.org/r/249792

Change 267449 had a related patch set uploaded (by Glaisher):
Remove intro text from some pages

https://gerrit.wikimedia.org/r/267449

Change 267449 merged by jenkins-bot:
Remove intro text from some pages

https://gerrit.wikimedia.org/r/267449

Change 274449 had a related patch set uploaded (by Glaisher):
Improve navigation links on special pages' subtitle

https://gerrit.wikimedia.org/r/274449

Change 274449 merged by jenkins-bot:
Improve navigation links on special pages' subtitle

https://gerrit.wikimedia.org/r/274449

Some problems reported were sold already. I created some subtasks based on feedback about problems that still exist.

Then some comments:

In T110642#2006505, @MZMcBride wrote:

• http://newsletter-test.wmflabs.org/wiki/Special:CreateNewsletter has a weird and confusing "Title of Main Page" label

What we are asking for is exactly the title of the main page of the newsletter, so at least it is accurate. Do you have suggestions for a better title? "Newsletter main page"?

• The "subscribe" button at http://newsletter-test.wmflabs.org/wiki/Special:Newsletter/6 shouldn't require confirmation on a separate screen

I agree that a direct subscribe would be better, but I think this is what we had before, and then @Glaisher change it for non-JS compatibility. Is this accurate?

• Description field doesn't parse wikitext (http://newsletter-test.wmflabs.org/wiki/Special:Newsletters)

I think parsing wikitext would complicate things a lot (i.e. not allowing people to insert images, templates, etc. It is meant to be a plain description, and I think plain text is just fine.

• Breadcrumbs at http://newsletter-test.wmflabs.org/wiki/Special:ManageNewsletter aren't aware of the current page (current breadcrumb should turn bold and be de-linked)

The current behavior is consistent with MediaWiki behavior, see for instance https://www.mediawiki.org/wiki/Phabricator/Help

In T116353#2167708, @Qgil wrote:

In T110642#2006505, @MZMcBride wrote:

• http://newsletter-test.wmflabs.org/wiki/Special:CreateNewsletter has a weird and confusing "Title of Main Page" label

What we are asking for is exactly the title of the main page of the newsletter, so at least it is accurate. Do you have suggestions for a better title? "Newsletter main page"?

T124527#1961041 has some related comments.

• The "subscribe" button at http://newsletter-test.wmflabs.org/wiki/Special:Newsletter/6 shouldn't require confirmation on a separate screen

I agree that a direct subscribe would be better, but I think this is what we had before, and then @Glaisher change it for non-JS compatibility. Is this accurate?

That was on Special:Newsletters when we used to have radio buttons (see T110191). It would be easy to make the code in ext.newsletter.newsletters.js reusable and attach a click handler to the (un)subscribe button on Special:Newsletter to achieve this.

• Breadcrumbs at http://newsletter-test.wmflabs.org/wiki/Special:ManageNewsletter aren't aware of the current page (current breadcrumb should turn bold and be de-linked)

This was fixed in https://gerrit.wikimedia.org/r/274449

Here are some suggestions!

List of newsletters
Title row should text should be left-aligned to match content below
Increase line-height or spacing between rows
Subscribe: instead of button that says subscribe/unsubscribe, use check boxes
Filter: instead of filter at the top to see all newsletters/subscribed/unsubscribed, make the subscribe column in the table sortable
Subscriber count: also make this sortable
Suggestion: add a column to the table for latest issue date. make this column sortable so that users can see the most recent issues at the top.

M159 newsletter extension - list

10

Newsletter details
Change text sizes to make title stand out and provide better hierarchy (see mock)
Title - serif 21px
Make main page title, publishers and subscribers bold
Move unsubscribe button to bottom

M160 newsletter extension - details

10

Create a newsletter
Move title of main page to top, add a description with instructions on creating a new page. Something like, "Create a new page with the title of your newsletter. Add that title here."

In T116353#2175737, @KHammerstein wrote:

Here are some suggestions!

Thank you, Katie!

Thank you @KHammerstein ! I agree with all your suggestions. I wonder whether we have any technical limitations with checkboxes and/or sortable columns? We have been going a bit back & forth with this.

In any case, I don't think any of the remaining points is blocker for a deployment in production. I will move this task to the corresponding column in our workboard. Progress!

In T116353#2175737, @KHammerstein wrote:

Subscribe: instead of button that says subscribe/unsubscribe, use check boxes

Initially this used to have radio buttons but I changed that to use links because it didn't work for no-JS users. See my comment above just above yours and the linked task. Unless it's possible to link to the subscription page with checkboxes for non-JS users, this cannot be done.

@Qgil as this is a subtask of the deployment goal I assume it is complete and can be closed?