Do you have an estimate of when this would be ready to go to production?

@Chris McMahon: Andrew Green has started looking over the code and will be doing a formal review soon, I think. Functionally, it's in good shape from my perspective. (Nischay will be adding more functionality, but it's already an improvement over the current EducationProgram course activity feature.)

We want to make sure we can show it to education program leaders who are using the current version. My hope is that it will be ready for production by mid-September.

I ask because we have a loose policy of not installing new features on beta labs without a solid release date for the feature.

We do this to reduce the amount of risk that beta labs users face.

This surfaced recently on a thread on the QA mail list having to do with adverse effects of testing HHVM in beta labs: https://lists.wikimedia.org/pipermail/qa/2014-August/001856.html

While we in Release Engineering can change the config to add a new extension to beta labs, it requires Operations to merge it, and before that happens it should undergo full code review, security review, etc. before it gets put on a public shared test environment.

Thanks, that's very helpful info. So we'll wait on the code review before putting it on beta labs, and in the meantime, I'll see about putting it on another labs instance.

Thanks, beta labs should be very much like production. We can work with Greg and Antoine and Ops to get this to beta and on to prod.

Adding Chris Steipp because this will need security review at some point.

If this extension is being built for $something, where are this extension's bug reports being tracked?

Checklist

1. Create Extension: mediawiki.org page for developers and people who will install or configure the extension.
2. Create Help:Extension: mediawiki.org page for the end user documentation. Cross-link it with the above.
3. Request a component in [[Bugzilla]].
4. Get the extension code in [[Gerrit]].
5. Request (and respond to) a product review, if applicable
6. Request (and respond to) a [[WMF Project Design Review Process|design review]], if applicable.
7. Request (and respond to) a performance review.
8. Request (and respond to) a security review.
9. Show community support/desire for the extension to be deployed, if applicable.

If this extension is going to be deployed to prod, there should be a master bug for the prod deployment and then a bug blocking it for going to beta labs. I didn't see this until now because it's not blocking the main review queue bug.

In any case, this code isn't ready to be deployed anywhere. The JS isn't following our code conventions at all, appears to have been copied from an on-wiki userscript (claiming it is "borrowed"), but doesn't have any licensing information. There's also no default license set for the extension, which isn't good. The PHP code is mixing tabs and spaces, and looks like a bunch of it was copied from core.

andrew.green.df wrote:

Thanks everyone for the comments and suggestions!!!

(In reply to Kunal Mehta (Legoktm) from comment #9)

this code isn't ready to be deployed

Agreed.

Regarding coding conventions and licensing, my understanding is that they are being worked on or will be worked on soon.

For the code that was copied from core: there needs to be some refactoring on both sides (the new extension and/or core itself) to avoid this, or if for some unusual reason that's not possible, a "FIXME" note has to be included.

I haven't had time to fully review the code yet :| only an initial, superficial review done so far. (Since I didn't see any Gerrit changes, I just sent comments by e-mail.)

The creation of this initial version was supported by the Wiki Education Foundation [1][2] following a design and user interviews by a student working with the WMF [3]. WMF Features (Terry) did give an initial green light for seeking the required reviews and exploring how to get this feature merged. I don't know how all this might change in view of recent discussions on process for new features, but I think it's unlikely there will be objections from Education Program users. (Maybe @Sage can comment more on that...)

:)

[1] http://wikiedu.org/
[2] https://meta.wikimedia.org/wiki/Wiki_Education_Foundation
[3] See https://www.mediawiki.org/wiki/Editor_campaigns/Activity_feed

Thanks everyone.

Complete checklist items:

*MW extension page: https://www.mediawiki.org/wiki/Extension:RecentActivityFeed
*Gerrit: https://gerrit.wikimedia.org/r/#/admin/projects/mediawiki/extensions/RecentActivityFeed
*Component request: bug 70622

The others are still pending. Nischay will fix the code styling issues, we got permission for the original script author to use it under MIT License and we'll use that for the whole project as well. I'll be seeking community feedback on the working prototype before we move toward deployment. I'm hoping that a formal design review will not be necessary, as (a) it's a pretty clearcut improvement over the niche feature of the EP extension it replaces, and (b) Steven Walling OK'd the basic design as a Campaigns features during the previous internship project where the mockup was created.

A test instance is up here: http://education.wmflabs.org/w/index.php?title=Education_Program:WikiWorks/MediaWiki_Extension_Design_%28Fall_2014%29&days=30&from=&action=epcourseactivity

andrew.green.df wrote:

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!!

@Ragesoss is RecentActivityFeed still a project or has it been abandoned?

@hashar: it's been abandoned.