Change Details

>>! In T115095#3004776, @Bawolff wrote: > Review of 631882e3779 of Newsletter extension (Jan 29, 2017) > [x] "NewsletterDb.php" line 388 - getNewslettersUserIsPublisherOf() - Why is that a left join instead of an inner join? @MtDu [x] 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. * (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). * 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. [x] 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). * "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. * 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. * 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. * [x] The edit page seems to be missing a copyright notice. It should probably have one. @srishakatux

>>! In T115095#3004776, @Bawolff wrote: > Review of 631882e3779 of Newsletter extension (Jan 29, 2017) > [x] "NewsletterDb.php" line 388 - getNewslettersUserIsPublisherOf() - Why is that a left join instead of an inner join? @MtDu [x] 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 [x] "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). * 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. [x] 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). * "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. * 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. * 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. * [x] The edit page seems to be missing a copyright notice. It should probably have one. @srishakatux

>>! In T115095#3004776, @Bawolff wrote: > Review of 631882e3779 of Newsletter extension (Jan 29, 2017) > [x] "NewsletterDb.php" line 388 - getNewslettersUserIsPublisherOf() - Why is that a left join instead of an inner join? @MtDu [x] 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 *[x] "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). * 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. [x] 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). * "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. * 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. * 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. * [x] The edit page seems to be missing a copyright notice. It should probably have one. @srishakatux