Last modified: 2014-10-09 19:03:58 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T34128, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 32128 - Review and deploy OnlineStatusBar extension
Review and deploy OnlineStatusBar extension
Status: NEW
Product: Wikimedia
Classification: Unclassified
Extension setup (Other open bugs)
unspecified
All All
: Normal enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on: 40603 37132
Blocks: 22516 31235
  Show dependency treegraph
 
Reported: 2011-11-01 20:09 UTC by Peter Bena
Modified: 2014-10-09 19:03 UTC (History)
12 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Peter Bena 2011-11-01 20:09:19 UTC
Hi, I would like to ask for a full code review of trunk/extensions/OnlineStatusBar

I believe there is a lot to fix or improve, especially concerning its performance, so any feedback would be really appreciated, thanks!
Comment 1 Mark A. Hershberger 2011-11-01 23:03:40 UTC
Think "Extension requests" is a better place for this request.  Also, it may be more effective to find someone in #mediawiki on IRC and ask there.
Comment 2 Peter Bena 2011-11-02 20:34:37 UTC
That's the place where they sent me here :)
Comment 3 Bawolff (Brian Wolff) 2011-11-25 23:50:48 UTC
Also related - bug 26246.

Also, if the proposal on wikipedia turns out to be succesful, you'll want to mark this as blocking bug 31235.

-------


I happened to take a superficial look at the extension today (Saw it mentioned in the signpost, and wondered what it was about). One thing you might consider doing is combinding some of the CSS files, so that the stuff that is the same in all of them goes in a common css file, and the one or two lines that differ go in the skin specific css files. Another thing I noticed is underscores in some variable names (but that's a stylistic thing that doesn't matter).

Also, I'm not sure how well this extension would play with squid caching. Logged out users will probably see outdated statuses as far as i can tell.

Cheers
Comment 4 Bawolff (Brian Wolff) 2011-11-26 00:21:22 UTC
Other things I notice:

*The wording on the options in the preferences is kind of weird (Do you want X? where most other preferences are not worded as a question)
*"Purge user page everytime when you login or logout" is an implentation detail, and should not be a preference
*flash of semi-unstyled content. The css for this extension should be loaded at the top
*It's weird to be able to have different statuses like busy, on-line etc, but not be able to set them (you can set the default certainly on special:preferences, but the user shouldn't have to be constantly going to special:prefs to change their status, preferences should be used for rarely changed options. There should be some other method (perhaps, if user is viewing own user page, have a drop down box where the status is displayed)
* <b>Notice</b>:  Undefined variable: w_time in <b>/var/www/w/extensions/OnlineStatusBar/OnlineStatusBar.status.php</b> on line <b>99</b><br />
Displayed directly after i made an edit to my user page
*Note, my comment before about squid might be incorrect, the purge i see in the code should take care of that I think.
Comment 5 Peter Bena 2011-11-28 10:08:34 UTC
Thank you,

CSS:
I don't know if it would have some effect if I split it to more files, because now only one file needs to be loaded when page is opened, if I split it, I would need to load more files, so it would have probably negative effect on loading of page.

Regarding java script - it would be cool if there was option to change it directly but since I don't understand js, it's not possible for me to implement it, but if anyone could do that, it would be great.

I believe that most of other issues you mentioned were fixed during weekend, and if not I am still working on it.
Comment 6 Peter Bena 2011-11-28 12:26:52 UTC
So the discussion on english wp was closed in favor of installation of this:
http://en.wikipedia.org/wiki/Wikipedia:Village_pump_(proposals)#Online_Status

Therefore I would like to ask for more input concerning this extension, what all should be implemented or fixed or what do you think that isn't correct.

I tried to reword some of options according to feedback of english wikipedia users (native english speakers), so I hope it should be correct now.

In case that there were no more concerns, it would be nice if someone could do full code review or help me to finish it so that it could be deployed to english wp. Thank you
Comment 7 Peter Bena 2011-11-28 12:41:25 UTC
I would like to remind that extension is now installed here http://hub.tm-irc.org/test/wiki/User_talk:Petrb (example of status)
and that anyone can create as many user accounts as they need to test it,

thank you for your feedback
Comment 8 Bawolff (Brian Wolff) 2011-11-28 14:05:35 UTC
(In reply to comment #5)
> Thank you,
> 
> CSS:
> I don't know if it would have some effect if I split it to more files, because
> now only one file needs to be loaded when page is opened, if I split it, I
> would need to load more files, so it would have probably negative effect on
> loading of page.
> 

Nope, the ResourceLoader will combine them all before sending them to the user. Only one css file will be downloaded in either case.
Comment 9 Sam Reed (reedy) 2011-11-28 14:06:56 UTC
Why's it been added against bug 31235?

I've not seen a request for it to be enabled on WMF wikis
Comment 10 Bawolff (Brian Wolff) 2011-11-28 14:08:13 UTC
(In reply to comment #9)
> Why's it been added against bug 31235?
> 
> I've not seen a request for it to be enabled on WMF wikis

Comment 6: http://en.wikipedia.org/wiki/Wikipedia:Village_pump_%28proposals%29#Online_Status (perma-link http://en.wikipedia.org/w/index.php?title=Wikipedia:Village_pump_%28proposals%29&oldid=462882512#Online_Status )
Comment 11 Peter Bena 2011-11-28 14:13:24 UTC
Right, I will try to split them, but how can I make it load on top? Is it
possible to make it load the css before other parts of the page?

Concerning option to purge user page, I think it should remain in options,
because there may be users who:
1 - want to show status bar / not using magic word - in this case it should not
purge
2 - want to hide status bar / using magic word - in this case it should purge
3 - want to use both - in this case it should purge

Reedy: sorry I am sort of new to this, I didn't know there is a queue you mentioned, I will try to insert it there soon
Comment 12 Bawolff (Brian Wolff) 2011-11-28 14:48:30 UTC
(In reply to comment #11)
> Right, I will try to split them, but how can I make it load on top? Is it
> possible to make it load the css before other parts of the page?

Use the OutputPage method addModuleStyles instead of addModules. 

> Concerning option to purge user page, I think it should remain in options,
> because there may be users who:
> 1 - want to show status bar / not using magic word - in this case it should not
> purge
[...]

Even in that case, you'd want to purge the page. (The status bar gets cached too)
Comment 13 Peter Bena 2011-11-28 14:52:38 UTC
(In reply to comment #12)
> > Concerning option to purge user page, I think it should remain in options,
> > because there may be users who:
> > 1 - want to show status bar / not using magic word - in this case it should not
> > purge
> [...]
> Even in that case, you'd want to purge the page. (The status bar gets cached
> too)

For some reason it works even without purge (at least on my test wiki, while magic word needs purge), why? If it's necessary I will do that but I wanted to avoid purge if possible

Thanks for your help
Comment 14 Bawolff (Brian Wolff) 2011-11-28 15:23:00 UTC
> 
> For some reason it works even without purge (at least on my test wiki, while
> magic word needs purge), why? If it's necessary I will do that but I wanted to
> avoid purge if possible
> 
> Thanks for your help

You're right, that isn't in the parser cache. (I wonder why it wasn't being cleared away for me in my testing). However, it would still be cached by squids/file cache if those caches are in use [but could probably use something that clears them without doing a full purge if overly concerned]. In any case, if purge is not desired in those cases, should be detected based on other options, not something user specifiable.
Comment 15 Peter Bena 2011-11-28 15:29:58 UTC
(In reply to comment #14)
> > 
> > For some reason it works even without purge (at least on my test wiki, while
> > magic word needs purge), why? If it's necessary I will do that but I wanted to
> > avoid purge if possible
> > 
> > Thanks for your help
> You're right, that isn't in the parser cache. (I wonder why it wasn't being
> cleared away for me in my testing). However, it would still be cached by
> squids/file cache if those caches are in use [but could probably use something
> that clears them without doing a full purge if overly concerned]. In any case,
> if purge is not desired in those cases, should be detected based on other
> options, not something user specifiable.

Problem is that you can't always detect if user wants this or not, they can for instance change backgroud color of their user page, using magic word while having the status bar as well (that's why there is 'for advanced users')

If it's problem it can be removed, however I think that giving people more options to customize it would be better, or at least should be overridable in LS.php?
Comment 16 Peter Bena 2011-11-30 13:51:47 UTC
Althought both caches are installed on test wiki where I am testing it, I have no problems with that, you noticed that there could be problem with squid cache, but unless I have different configuration on server than is on production I think it should be ok, because when status expires, user is flagged offline even without purge, so maybe it's not needed, I added some code which should handle cache expiry times for this reason, I hope it helps a bit, do you see any other problem?
Comment 17 Peter Bena 2011-11-30 16:57:20 UTC
Hm, there is probably bug in core, but I would like to check before I report it, setCacheExpiry purge all caches excepting browser cache in firefox, so actually it has troubles in firefox (user status doesn't update, unless browser cache is turned off), is here any workaround to flush browser cache which works for all browsers?
Comment 18 Bawolff (Brian Wolff) 2011-11-30 19:30:21 UTC
(In reply to comment #17)
> Hm, there is probably bug in core, but I would like to check before I report
> it, setCacheExpiry purge all caches excepting browser cache in firefox, so
> actually it has troubles in firefox (user status doesn't update, unless browser
> cache is turned off), is here any workaround to flush browser cache which works
> for all browsers?

I assume you mean updateCacheExpiry (method of ParserOutput), in which case yes this is normal. It will only update the time the parser cache expires. Squid still needs to be explicitly purged. Browsers should not be caching user pages when  (Or more specificly, they should do if-modified-since checks every time). However, ParserOutput::updateCacheExpiry doesn't update the last modified times, so server's would still send HTTP 304 not modified in those cases.
Comment 19 Krinkle 2011-11-30 21:01:38 UTC
A rough review / suggestion:

--

Purging a user page (and any other place it will potentially be shown on) because the user online status changes seems wrong. I haven't looked at the extension deeply yet to know wether or not and if so how it does this, but if it does, it shouldn't.

I suggest solving this with AJAX instead.

* users can set a preference to join/leave exposing their status
* users can set a preference to do or don't show other ppls statuses

By default we will probably want to make the first preference false and the second one true.

Then the extension would have a scripts&style module, with position=>top, loaded through addModules instead of addModuleStyles.
* a stylesheet + extra stylesheets for per-skin perfection
* one or more javascript files that will load this information from the API and display in on the appropiate pages

The module would load dependent on the user preference (use the BeforePageDisplay hook, then conditionally $out->addModules).

And, as noted, the information would have to be made available through an API module.

While taking a quick look at the sql file, I noticed it uses column username (VARCHAR 255) and timestamp (char 14). I suggest user IDs instead and timestamps per MediaWiki convention (and columnames prefixed)
* os_user int unsigned NOT NULL
* os_timestamp binary(14) NOT NULL default '19700101000000'
(mysql types based on user.user_id and logging.log_timestamp [1])

--
Krinkle

[1] http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/maintenance/tables.sql?view=markup
Comment 20 Krinkle 2011-11-30 21:02:58 UTC
> * users can set a preference to join/leave exposing their status

rephrase, * users can set a preference to opt-in/out exposing their status
Comment 21 Peter Bena 2011-11-30 21:15:56 UTC
Thank you for review:

SQL:
it uses char's because it allows even tracking of IP users (if user allow that in LS.php)  it's disabled by default, however if I changed it to user ID it would not be possible (but since it was developed based on request on enwp where tracking of IP users would be disabled, I don't think it's important) + also using user name seems to me easier since I do not need to translate it to user ID and back

Timestamp will be updated asap

AJAX:
I don't know much about ajax but I believe you are correct :)
The default options are already in configuration (if users have it enabled as default etc.)

API:
I will try to implement it there soon

Regarding cache - I don't know how different it would be when AJAX would be used, so maybe the current problems with cache flushing wouldn't occur using that, so I won't care about that for now.
Comment 22 Peter Bena 2011-12-01 14:01:01 UTC
I implemented api so that now you can do api.php?action=query&prop=onlinestatus&onlinestatususer=user to get status of user, unfortunately I still don't know how to implement AJAX part I will try to find it out :)
Comment 23 Peter Bena 2011-12-08 13:09:59 UTC
thanks to Brion it's updated so you can review it now
design is now little bit different, the online bar doesn't even query db, but it retrieve it using api every two minutes (when user wants to display it)
Comment 24 Peter Bena 2011-12-22 13:32:04 UTC
is there any progress in review of the source?
Comment 25 Peter Bena 2012-01-30 13:41:51 UTC
Assigned to Krinkle since he said he is going to review it
Comment 26 Krinkle 2012-02-02 00:36:08 UTC
OK. Lots of progress has been made so I'll give it a full extension review this time (on a wiki page).

Note that I'm going to FOSDEM this weekend and after that I'll be offline for 2 weeks. I'll be back February 20 and this'll be reviewed that week :)
Comment 27 Sumana Harihareswara 2012-04-04 14:21:51 UTC
Krinkle: ping.  If you can't do this within the next month, please say so and I'll try to find someone else to do so.
Comment 28 Krinkle 2012-04-04 17:49:11 UTC
(In reply to comment #27)
> Krinkle: ping.  If you can't do this within the next month, please say so and
> I'll try to find someone else to do so.

I won't have time to do this in my spare time as I have other things I plan to work on in the coming weeks/months.

As foundation work, I basically do whatever I'm assigned to. So the regular channels apply here. This is currently not on my agenda and I'm currently assigned to 2 medium-long term projects that are currently taking up all my available hours per week  (ResourceLoader and Continuous integration) and that likely won't change for at least another month.
Comment 29 Sumana Harihareswara 2012-04-05 00:02:42 UTC
Adding the design keyword - Brandon, could you take a look at the UI and give some feedback here?
Comment 30 Platonides 2012-04-10 16:33:21 UTC
I thought I had given some feedback about this extension time ago (perhaps through irc, I don't seem to have edited this bug).
I think it would be better to use memcached for this, as it's not something we want to keep or replicate.
Additionally, it seems to support Online/Away/Offline, with Away being calculated automatically. Seems desirable that it allowed to mark yourself as being away or offline without having to change your preferences. Also, some user scripts with this kind of goal, support a Busy status, so this should probably allow that, too.
Comment 31 Peter Bena 2012-04-10 16:42:19 UTC
It uses memcached
Comment 32 Sumana Harihareswara 2012-04-10 17:58:06 UTC
Trevor is taking over code review for this extension as part of his 20% community service time per https://www.mediawiki.org/wiki/Wikimedia_engineering_20%25_policy .
Comment 33 Peter Bena 2012-04-10 19:10:57 UTC
Thanks Trevor! Let me know if you found any issue
Comment 34 Sumana Harihareswara 2012-04-17 17:27:13 UTC
Trevor began this review last week and it is continuing.

<TrevorParscal> yeah, so i'm reviewing en masse because there are so many deferred revs and things need one good pass
<TrevorParscal> also, i'm doing a design review at the same time

Assigning to Trevor.  Trevor will communicate with Peter to request fixes.
Comment 35 Sumana Harihareswara 2012-06-12 18:10:08 UTC
https://www.mediawiki.org/w/index.php?path=%2Ftrunk%2Fextensions%2FOnlineStatusBar&title=Special%3ACode%2FMediaWiki has the updates and code review Krinkle has done, and we'll be moving the extension to Gerrit this week.  Useful bits of IRC conversation from today:


<TrevorParscal> I just OK'd Timo's whitespace fixes
<TrevorParscal> I think this extension is ready to be moved to gerrit personally
<Krinkle> yeah, we both did a review and a small pass with fixes over the code. afaik it is fully reviewed and fixed up. I did open 1 or 2 bugs in bugzilla that I think have to be fixed before deployment on production wikis
<Krinkle> they block deployment of any kind imho, but moving the repo into gerrit is fine
<TrevorParscal> it's gone through UI review (by me) and UI fixup (by petr), JS/CSS rewrite (by me( and JS/CSS review (by Timo) and PHP review (by me and Timo) and fix up (by Petr and Timo) and finally more review (by me)
<^demon> Let's get it on the list so it'll get done this week.
<Krinkle> TrevorParscal: I didn't put this in review or in bugzilla, but something else I dislike about it is how it displays the information "John Doe is now <blib> offline" or "John Doe is now <blib> hidden" or "John Doe is now <blib> status is unknown"
<TrevorParscal> Krinkle: yeah, I suggested that the <blip> be moved to the far right and the sentence be more consistent
Comment 36 Sumana Harihareswara 2012-08-17 19:55:37 UTC
It's now in Git/Gerrit: https://gerrit.wikimedia.org/r/gitweb?p=mediawiki/extensions/OnlineStatusBar.git;a=summary

Timo, Trevor: do you approve this for deployment?
Comment 37 Krinkle 2012-08-24 14:03:46 UTC
Deployment where for which community/communities?

If this is deployment beyond testwiki, then the design is imho a requirement and a blocker for deployment (both usability wise, as well as technically speaking due to the implementation with the problematic absolute-position which conflicts with many other components and gadgets).

I don't know the design has been improved on either front since I last reviewed it. The blocker bug as dependency on this one for it is still open at this time.
Comment 38 Krinkle 2012-08-24 16:09:34 UTC
-design: there is no design needed to deploy this extension (which is what this bug is for). The blocking bug, bug 37132, is design related and already has the design keyword.
Comment 39 Bartosz Dziewoński 2012-09-24 20:35:33 UTC
And the "John Doe is now status is unknown" issue has been fixed as well (by Krinkle) in https://gerrit.wikimedia.org/r/#/c/22180/.
Comment 40 Sumana Harihareswara 2012-10-11 01:19:17 UTC
http://en.wikipedia.org/w/index.php?title=Wikipedia:Village_pump_%28proposals%29&oldid=462882512#Online_Status is at least the beginnings of consensus to add something like OnlineStatusBar to English Wikipedia, and it looks like nearly all the blockers and critiques have been resolved.  Trevor and Timo, tell me if I'm wrong.

I want to ensure the design department has looked at this and given a thumbs-up, per https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment , so I've added the extension to https://www.mediawiki.org/wiki/User_experience_review_queue#Pending and am cc'ing the Editor Engagement product manager and Jorm from design. We've already gotten an OK from tech on the deployability of the code, so once design & product management gives OnlineStatusBar the OK, we can use that existing en.wp consensus to move forward and ask Sam Reed to put OSB on the deploy schedule.

(All of that is In My Opinion -- tell me if I'm wrong.)
Comment 41 Krinkle 2012-10-11 01:40:32 UTC
On the tech side it is okay now indeed. Though I believe there are various concerns still from the design, usability and/or accessibility perspective. It could be sufficient for a trial deployment, but there's certainly room for improvement.
Comment 42 Andre Klapper 2013-03-24 19:16:29 UTC
(In reply to comment #40)
> We've already gotten an OK from tech on the deployability of the code, so 
> once design & product management gives OnlineStatusBar the OK

It looks like there hasn't been any progress here for the last five months - CC'ing some more design people, asking for an extension review.
Comment 43 Greg Grossmeier 2013-08-29 18:23:18 UTC
Hello, this is a quasi-automated-but-not-really message:

I am reviewing all tracking bugs for extensions to review and deploy to WMF servers. See the list here:
https://bugzilla.wikimedia.org/showdependencytree.cgi?id=31235&hide_resolved=1

The [[mw:Review queue]] page lists the steps necessary to complete the review. I have copied them below and done some initial filling out based on what I can easily gleen from this bug and any linked to sources that are obvious. If I miss something/state something false, please do correct me.

Also, if you haven't yet done so, please review the information on and linked to from:
https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment


== TODO/Check list ==
Extension page on mediawiki.org: https://www.mediawiki.org/wiki/Extension:OnlineStatusBar
Bugzilla component: Yes
Extension in Gerrit: https://gerrit.wikimedia.org/r/#/admin/projects/mediawiki/extensions/OnlineStatusBar
Design Review: not done
Archeticecture/Performance Review: maybe (per Krinkle)?
Security Review: not done (right?)
Screencast (if applicable): not that I can find.
Community support: Looks to be for enwp.

Suggestions for next steps: Please email the Design mailing list at https://lists.wikimedia.org/mailman/listinfo/design asking for an explicit design review.

Also, ask on wikitech-l for a security review.

Thanks.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links