Review request: Tutorial booking remote mobile addon

Review request: Tutorial booking remote mobile addon

by Neill Magill -
Number of replies: 3
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers

I have been developing our teams first Moodle Mobile plugin, for our Tutorial Booking activity

The main aim of the plugin is to allow students to sign up to and remove themselves from Tutorial booking places via the Mobile app.

Picture of the tutorial booking remote mobile plugin interface

Does anyone with Moodle Mobile development experience have any time to give it a code review before we release it to the plugin directory?


Average of ratings: -
In reply to Neill Magill

Re: Review request: Tutorial booking remote mobile addon

by Juan Leyva -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers

Hi Neill,

we can do a quick review of the plugin (I will ask someone from my team).

Please, remember to ask the Mobile app support badge for your plugin once released https://moodle.org/plugins/browse.php?list=award&id=6

Regards, Juan

Average of ratings: Useful (1)
In reply to Juan Leyva

Re: Review request: Tutorial booking remote mobile addon

by Dani Palou -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

Hi Neill,

I just did a quick review to your mobile addon and it looks great, I think you did a good job with it!

I noticed some minor things that you might want to change:

  1. In $mmaModTutorialbookingHandlers, the part about statusObserver isn't needed since your module cannot be downloaded. 
  2. In $mmaModTutorialbookingHandlers you have some injections that aren't needed (mmCoreOutdated, mmCoreNotDownloaded, ...). This isn't a big deal.
  3. We did a refactor in the content links delegate to make it easier to implement. You can see an example of how it is used now in here.
  4. In $mmaModTutorialbooking, in all your .write calls, you're returning an object instead of an error message if the call fails. We did this in some modules to make offline synchronziation easier (and because we had a wrapper function to handle these errors), but usually the WS calls will return just the error message (and your controller IS expecting a message string). For example, you call $mmaModTutorialbooking.removeSignup(id) and the .catch is expecting a message, but in the implementation of the function you're returning an object. I think you should remove all the .catch in your service, and let the WS call return the regular message as it does by default.
  5. In here and similar places you could use $mmUtil.showErrorModalDefault to make the code easier.
  6. You might want to reuse the refresh language string that we have in the core of the app (mm.core.refresh).

That's it, good work!

Dani

Average of ratings: Useful (1)