Extension:Echo/Review

From MediaWiki.org
Jump to navigation Jump to search
  • Selective events ([1])
  • API module ([2]; 180 lines, mostly boilerplate)
  • You might want to make getNotifications's LIMIT value configurable. Kaldari (talk) 20:07, 1 August 2012 (UTC)
  • Object model ([3]; 500 lines)
  • Notification.php
  • line 27 - should it be $obj->user = false;?
  • Property initialization can be put inside the constructor, line 6,7 and line 28,29 seems to be duplicated.
  • $this->readTimeStamp doesn't seem to be used, in line 64, it's assigned with null other than $this->readTimeStamp
  • Subscription.php
  • line 29, should just use !is_string() instead of is_object() and should document the 63 length limit, it should be configurable as well
  • Event.php
  • line 62 - should further check if the type is a valid and enabled type
  • NotificationController.php ([4]; 190 lines)
  • (minor) The tabbing/formatting on lines 21-26 is confusing. Kaldari (talk) 21:06, 1 August 2012 (UTC)
  • formatNotification should probably return false (or do something) if $wgEchoNotificationFormatters[$eventType] isn't set. Kaldari (talk) 21:06, 1 August 2012 (UTC)
  • CSS and JS, particularly ext.echo.overlay.js ([5]; ~300 lines)
  • This global seems like it might be better put in the mw.config object somehow.
  • Seems like there are a lot of bits that could do with code-convention review, but noting the deployment window today, I'll hold off.
  • Would it be better to clone the <a> tag here? You might be able to remove a DOM query. Also, since you refer to that link many times in the course of the same function, you could declare a variable to store it, further avoiding unnecessary DOM queries.
  • It feels wrong to check for classes like this, maybe use $.hasClass()? Also, while clever, the selector magic is a little hard to read.
  • Cheers! --MarkTraceur (talk) 19:16, 1 August 2012 (UTC)
  • In ext.echo.overlay.js, I would divide the code into 'set-up' and 'execution' and only put the execution part inside the document.ready wrapper ( $( function() {} ). That way, when document.ready fires, all the set-up is already loaded. Probably doesn't actually make much difference in practice, but that's how I would write it if I were building it. Kaldari (talk) 19:53, 1 August 2012 (UTC)
  • Need to remove the console.log() statement before deploy. Kaldari (talk) 19:55, 1 August 2012 (UTC)
  • SpecialNotifications.php ([6]; 62 lines)
  • It looks like some of this code is duplicated in the API module. Would it be possible to call the API's function instead? Kaldari (talk) 21:11, 1 August 2012 (UTC)
  • Notifier.php ([7]; 38 lines)
  • For future development, we might want to have the email sent by a job
  • Email formatting ([8]; 30–40 lines of 168 lines)
  • Comments would be useful here. Kaldari (talk) 21:13, 1 August 2012 (UTC)
  • Hooks.php ([9]; 250 lines)
  • line 39 - User::newFromName() maybe return false, should check that before calling getId()
  • line 41 - creating $user again seems redundant
  • Needs more inline comments. Kaldari (talk) 20:03, 1 August 2012 (UTC)

Total: ~1500 lines of code.