https://bugzilla.wikimedia.org/show_bug.cgi?id=69785
--- Comment #12 from Andrew Green <[email protected]> --- Reviewed most of the code, except the javascript. The first issue to deal with is the repetition of code from core. An option that I suggested is to extend SpecialRecentChanges, like SpecialRecentChangesLinked does. A minor refactoring in core would be needed. Details: - The parent's buildMainQueryConds() could be used. - For makeOptionsLink() the additional parameters could be added to $options and call parent::makeOptionsLink(). - I *think* the parent's doMainQuery() could also be used. The additional conditions could be added before calling. (It looks like the query options end up being essentially the same. There's some extra stuff in the parent method, not sure if any of it would be harmful.) - In theory, the differences in optionsPanel() could be handled by getExtraOptions() and a minor change in core. In getExtraOptions() the namespace control could be fetched from the parent's namespaceFilterForm(). The refactoring in core could let subclasses of SpecialRecentChanges control the options that are displayed as links below where you select the number of changes/age in days. - The remaining method with repetition, outputChangesList(), is substantially different from SpecialRecentChanges in many ways, so the repetition of scattered fragments there is basically fine, I think. Just mentioning it in an inline comment would be enough. Regarding this point, Nischay pointed out that SpecialRecentChanges was not designed to be inherited, and that instead a generic base class should be created. I think that's reasonable, so there's another minor refactoring in core that could be considered. In any case, I'm hoping to get more feedback on this; that's why I'm taking the discussion out of the e-mail exchanges. (I don't see any other good place for it; if anyone has a better suggestion, please let me know. :) ) I'm posting my other specific comments here, since it also seems like the best place for them. (Line numbers reference commit 1f0640.) - When a user enrolls in a course, they still see the old version of the activity feed. - Did you consider taking into account course start and end dates? - SpecialRecentActivityFeed line 49: the $listed parameter should be set to false? It doesn't seem to do much (?) but we should be consistent... - I know it's common practice in Mediawiki, but I don't really like leaking database column names further afield than necessary. So maybe instead of having to send in a DB column like 'rc_user' in setAdditionalConds(), the class could have a way of taking a list of users? I think that would make for a cleaner internal API. - SpecialRecentActivityFeed line 289: maybe put the check for 0 rows sooner? - SpecialRecentActivityFeed line 301: instead of adding the module here, can you use addModules()? - Maybe $this->defaultParams could have a better name? Maybe something like additionalUrlParams? Also, the setter setParams() should have exactly the same name as the variable. - SpecialRecentActivityFeed lines 358-362: unused local variables. - Remaining style issues (it's OK to leave these 'till last): consistent spacing between methods, @see annotation for overriding methods, explicit method visibility level and, ideally, more inline comments explaining how things work. Thanks!! -- You are receiving this mail because: You are the assignee for the bug. You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
