Support viewing marks, colours, links and comments in the app

[dpalou]

No description provided.

[davosmith]

Thanks for that - a quick scan through the code suggests it's all good, but I'll give it a more detailed look and try to merge it in the next couple of days.

[davosmith]

Hi Dani,

Apologies for the delay - Christmas, with associated family visits, has taken up quite a bit of my time.

I've had a look at the code (and tried it out with the mobile app). Overall it is looking great, but there are a few issues I've spotted that I wondered if you might have time to take a look at?

• There are a number of CI errors - I think that some of them are fixed in my master branch, but others look like they are directly related to the new code (please let me know if you cannot access the CI results)
• "Teacher" marks are incorrectly shown for:
  • Checklists that are set to "student only"
  • Checklist items that are set to "heading"
• "Student" marks are incorrectly shown (and can be updated) for:
  • Checklists that are set to "teacher only"

If you have a chance to look at these, then that would be fantastic. If not, let me know, and I'll take a look at them when I get a chance.

[dpalou]

Hi Davo,

sorry for the delay in answering, I was on annual leave last week.

Please ignore the activity in this PR for now, I rebased the branch using your master branch to fix some CI failures and I'll look at the rest of them now.

[davosmith]

Thanks Dani,

The work you've done here is much appreciated - I hope you had a good break last week!

Let me know when this is ready for me to take another look.

[dpalou]

I managed to fix all the CI failures except some Mustache Lint failures that can be ignored because it's complaining about some app directives. I don't know if it's possible to ignore these failures, I can ask Eloy tomorrow (he's currently on leave).

In case you want to start reviewing it, here are my latest changes:

• I rebased the branch against master, so I also changed the target branch of this Pull Request.
• I fixed most of the CI failures and squashed the fixes into existing commits. I only added PHPDoc and Mustache doc, and also I removed the defined('MOODLE_INTERNAL') || die() from classes/output/mobile.php because the linter said it wasn't needed, I'm not sure if this can cause problems in older Moodle versions.
• I added a new commit to fix the display of 'Teacher only' and 'Student only' checklists.
• I also added a new commit to fix the failing behat tests, the 2018 year is no longer part of the dropdown. I also changed the future date to avoid the behat failing again in the near future.

[davosmith]

Thanks for all those changes - I'll try to review as soon as possible.

Apologies for the 2018 error - definitely my fault, I need to update those tests to use relative dates instead (I'm sure I'd have been hit by them when getting ready for M4.4)

[dpalou]

Hi Davo, I've added a variable to ignore the mobile template in Mustache Lint (it's not possible to only ignore some lines or rules). This variable needs to be in the step that installs the ci plugin, otherwise it doesn't work.

Now the code is ready to be reviewed again :)

[davosmith]

I've read through the code changes and I can see that all automated checks have passed, so this is looking good to me.

I'm going to try and run it locally on my phone in the next couple of days, just as a final sanity check, and then I'm hoping to merge + release it.

[davosmith]

I have given this a final check over + merged it - it'll be available in the Moodle plugins directory shortly (and yes, seeing it as the second item in the 'showcase shorts' did give the the push to get on with merging it).