[Non-security] General improvements for the Newsletter extension
Open, NormalPublic

Description

Review of 631882e3779 of Newsletter extension (Jan 29, 2017)

  • "NewsletterDb.php" line 388 - getNewslettersUserIsPublisherOf() - Why is that a left join instead of an inner join? @MtDu
  • NewsletterDb.php - getAllNewsletters(). This function seems to not be used, so I'm kind of confused why its here, seems to just be adding complexity. More importantly, if it was used, this would involve a full table scan and has no paging which seems bad. I would suggest removing it to prevent the temptation of someone in the future using it. @MtDu
  • "NewsletterEditPage.php" line 215 - The where condition in the select is wrong. It will return all active newsletters. @srishakatux
  • (minor) The behaviour for a failed csrf check is confusing to the user.
  • The subscriber count on Special:newsletters should be put throught $lang->formatNum(). (i.e. If user language is arabic, it should output arabic numerals). @srishakatux Note: This bug does not exist
  • In Special:Newsletters - Sorting by if the current user is subscribed doesn't make sense when filtering by if the current user is subscribed.
  • * "SpecialNewsletterCreate.php" line 52 - The mainpage field should specify the exists option so oojs does not suggest non-existent pages that would only error later. Ditto for the page title of issue field in the announce page. @MtDu
  • newsletter-subscribe-text is shown on the unsubscribe page. It should be a message about unsubscribing not subscribing @MtDu
  • The deletion/undeletion model doesn't seem quite right. If someone deletes a newsletter, its impossible to create a new newsletter with that name. If a newsletter exists at a title, but previous revisions are deleted, you cannot undelete those old revisions. The most obvious solution to this would be store all the subscribers (and possibly the list of issues. However as it stands the nl_issue table seems unused?) in the Newsletter json page. Then all the tables would functionally depend on the page, and you could just use MediaWiki's deletion handling. This would also simplify things as you could use MediaWiki's LinksUpdate / Content::getSecondaryDataUpdates() to manage all the db stuff for you. The downside is that every subscribe action would be an edit, and there could be some scalability issues if a newsletter had > 10,000 subscribers. Alternatively, you could disassociate the nl_ tables from the page name, and instead store nl_id in the json page. That way you could have multiple newsletters with the same page name (provided only one is nl_active at one time).
  • Missing message action-newsletter-restore, right-newsletter-restore. Also not in AvailableRights or GroupPermissions.
  • The newsletter namespace should probably be localizable.
  • When creating a newsletter with a "main page" not in NS_MAIN, the NS_NEWSLETTER pages links to the main page in the wrong namespace.
  • The interaction between NS_NEWSLETTER and Special:Newsletter is kind of confusing. It might be clearer to use the page title instead of the newsletter id (So to the user, it looks like the special page is operating on the NS_NEWSLETTER page, instead of on some separate thing, similar to how Special:movepage works). On the other hand, it probably doesn't matter.
  • "BaseNewsletterPresentationModel.php" line 18 - If you're using makeTitleSafe, you should check if it returns null and throw an exception or something (If you want it to always return a title, you could use makeTitle, which skips validation, but will always return a title object). @MtDu
  • "NewsletterContent.php" line 241 - It doesn't matter much since parser cache is disabled, but nonetheless, $wgOut should not be used in getNewsletterActionButtons(). You should use the ParserOutput object instead @srishakatux
  • The fact that the Newsletter pages are totally uncached and load the entire subscriber list from the DB on every view seems non-ideal. I guess its ok as long as the subscriber list stays small.
  • "NewsletterEditPage.php" line 38, line 131 - using getEscapedName() in a rawParams doesn't really make sense. Why not use the normal name on a normal param. @MtDu
  • There's a lot of duplicated code between Special:CreateNewsletter and NewsletterEdit. I think it might be better for Special:Createnewsletter to simply ask for a name and then redirect to the EditPage. If not that, the code should definitely be generalized to avoid the repetition.
  • There seems to be quite a bit of duplication between NewsletterContent::getNavigationLinks() and SpecialNewsletter::getNavigationLinks(). This should be refactored to share code where appropriate.
  • The edit page seems to be missing a copyright notice. It should probably have one. @srishakatux
  • On creating a new newsletter mainpage with an existing one - the error message shown is: "An existing newsletter has the same main page. Please enter another title." This should possibly be reworded to "An existing newsletter has the same main page. Please enter another Main Page". or something similar ?

Removed T115095 as its not a blocker to the security review

^^ This whole list looks so promising for the Wikimedia Hackathon program planned as per https://www.mediawiki.org/wiki/Wikimedia_Hackathon_2017/Register_and_Attend#Mentoring_program

Adding in the project now.

Note that although not part of the security review, some of the more serious of these may still block deployment (lack of copyright notice. The db queries that are more severely unperformant listed at T159083). Those things arent may area, so if for example someone from legal (for the lack of copyright notice) or jcrespo (for the db queries from T159083) said they were ok, then itd be ok, but as it stands those things should be considered deployment blockers

Change 354461 had a related patch set uploaded (by MtDu; owner: MtDu):
[mediawiki/extensions/Newsletter@master] Use INNER JOIN instead of LEFT JOIN because INNER JOIN does not include unmatched entries, whereas left returns null when there is no match.

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

I find the following too, unused functions from includes/NewsletterStore.php

  1. getNewslettersUserIsPublisherOf
  2. getNewsletterForPageId

many more, I would say

Change 354461 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Remove unused functions from NewsletterStore

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

Change 354553 had a related patch set uploaded (by MtDu; owner: MtDu):
[mediawiki/extensions/Newsletter@master] Use newsletter-unsubscribe-text instead of newsletter-subscribe-text

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

Change 354553 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Use newsletter-unsubscribe-text instead of newsletter-subscribe-text

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

01tonythomas updated the task description. (Show Details)May 19 2017, 9:09 PM

Change 354597 had a related patch set uploaded (by Srishakatux; owner: Srishakatux):
[mediawiki/extensions/Newsletter@master] Added license header to NewsletterEditPage.php

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

Change 354597 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Added license header to NewsletterEditPage.php

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

srishakatux updated the task description. (Show Details)May 19 2017, 9:41 PM
srishakatux added a subscriber: srishakatux.
srishakatux updated the task description. (Show Details)May 19 2017, 9:48 PM
srishakatux updated the task description. (Show Details)
srishakatux updated the task description. (Show Details)
srishakatux updated the task description. (Show Details)
srishakatux added a subscriber: MtDu.

Change 354604 had a related patch set uploaded (by Srishakatux; owner: Srishakatux):
[mediawiki/extensions/Newsletter@master] Fixed the where condition in the NewsletterEditPage.php

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

Change 354694 had a related patch set uploaded (by Srishakatux; owner: Srishakatux):
[mediawiki/extensions/Newsletter@master] Added space between brackets in the NewsLetterEditPage.php

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

Change 354694 abandoned by 01tonythomas:
Added space between brackets in the NewsLetterEditPage.php

Reason:
obselete after https://gerrit.wikimedia.org/r/#/c/354604

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

Change 354604 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Improve the query to validate mainpage_id in NewsletterEditPage.php

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

srishakatux updated the task description. (Show Details)May 20 2017, 5:37 PM
srishakatux updated the task description. (Show Details)May 20 2017, 7:43 PM

Change 354897 had a related patch set uploaded (by MtDu; owner: MtDu):
[mediawiki/extensions/Newsletter@master] Don't suggest pages that don't exist for mainpage

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

MtDu added a comment.May 21 2017, 8:13 AM

Note: For "SpecialNewsletterCreate.php" line 52, the UI does not change as a result of https://gerrit.wikimedia.org/r/354897, only that mainpage validation exists, using HTMLForm validation.

The UI will remain the same, that the suggestions will turn red when the page title does not exist.

Change 354897 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Use HTML Form Validation to ensure that the mainpage exists

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

Change 354898 had a related patch set uploaded (by MtDu; owner: MtDu):
[mediawiki/extensions/Newsletter@master] Add newsletter-restore as an action and right

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

MtDu updated the task description. (Show Details)May 21 2017, 8:27 AM

Change 354901 had a related patch set uploaded (by MtDu; owner: MtDu):
[mediawiki/extensions/Newsletter@master] Use params and getName instead of rawParams and getEscapedName

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

Change 354907 had a related patch set uploaded (by MtDu; owner: MtDu):
[mediawiki/extensions/Newsletter@master] Check if makeTitleSafe returns null in getNewsletterUrl()

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

MtDu updated the task description. (Show Details)May 21 2017, 8:47 AM

Change 354898 merged by Brian Wolff:
[mediawiki/extensions/Newsletter@master] Add newsletter-restore as an action and right

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

Change 354901 merged by Brian Wolff:
[mediawiki/extensions/Newsletter@master] Use params and getName instead of rawParams and getEscapedName

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

MtDu updated the task description. (Show Details)May 21 2017, 9:04 AM

Change 354907 merged by Brian Wolff:
[mediawiki/extensions/Newsletter@master] Check if makeTitleSafe returns null in getNewsletterUrl()

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

srishakatux updated the task description. (Show Details)May 21 2017, 9:25 AM
srishakatux updated the task description. (Show Details)May 21 2017, 9:30 AM

Change 354920 had a related patch set uploaded (by MtDu; owner: MtDu):
[mediawiki/extensions/Newsletter@master] Actually get the Newsletter name by using $this->newsletter

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

srishakatux updated the task description. (Show Details)May 21 2017, 9:32 AM

Change 354920 merged by 01tonythomas:
[mediawiki/extensions/Newsletter@master] Actually get the Newsletter name by using $this->newsletter

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

Change 354929 had a related patch set uploaded (by Srishakatux; owner: Srishakatux):
[mediawiki/extensions/Newsletter@master] Replace wgOut with ParserOutput object in NewsletterContent.php

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

Change 354930 had a related patch set uploaded (by 01tonythomas; owner: 01tonythomas):
[mediawiki/extensions/Newsletter@master] Addnewsletter queries should go into a single transaction

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

srishakatux updated the task description. (Show Details)May 21 2017, 11:46 AM
01tonythomas updated the task description. (Show Details)May 21 2017, 1:05 PM

Change 354930 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Add newsletter queries should go into a single transaction

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

Change 354929 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Replaced wgOut with ParserOutput object in NewsletterContent.php

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