ommit coruses fix issue 42 #82
Conversation
There was a problem hiding this comment.
Thanks for the PR - I've added a couple of notes above, but we also need this to be sent through to the "latest" branch before we can backport it to the 401_STABLE branch - eg it needs to land in the 404_STABLE branch first, then can be backported to older branches as needed.
Hopefully you can tidy that up as it would be great to have land in the code - thanks again!
lang/en/subcourse.php
Outdated
| @@ -71,6 +71,8 @@ | |||
| $string['settings:coursepageprintgrade_desc'] = 'Display grade from referenced course on course page.'; | |||
| $string['settings:coursepageprintprogress'] = 'Progress on course page'; | |||
| $string['settings:coursepageprintprogress_desc'] = 'Display progress from referenced course on course page.'; | |||
| $string['settings:displayhiddencourses'] = 'Display Hidden Courses'; | |||
There was a problem hiding this comment.
Moodle uses sentence case - not camel case - can you please change to "Display hidden courses"
lang/en/subcourse.php
Outdated
| @@ -71,6 +71,8 @@ | |||
| $string['settings:coursepageprintgrade_desc'] = 'Display grade from referenced course on course page.'; | |||
| $string['settings:coursepageprintprogress'] = 'Progress on course page'; | |||
| $string['settings:coursepageprintprogress_desc'] = 'Display progress from referenced course on course page.'; | |||
| $string['settings:displayhiddencourses'] = 'Display Hidden Courses'; | |||
| $string['settings:displayhiddencourses_desc'] = 'When selecting a subcourse from list hidden coruses will be displayed.'; | |||
There was a problem hiding this comment.
typo "coruses" -> "courses" but some better text might be something like:
"Allow hidden courses to be selected when editing a subcourse"
mod_form.php
Outdated
| $options[$mycourse->id] .= $hiddenlabel; | ||
| if($config->displayhiddencourses || $mycourse->id == $currentrefcourseid){ | ||
| $hiddenlabel = ' '.get_string('hiddencourse', 'subcourse'); | ||
| $options[$mycourse->id] = $courselabel.$hiddenlabel; |
There was a problem hiding this comment.
this line will trigger codiing guideline failures - the gitlab pipelines might show them, but you might want to investigate installing moodle's codechecker tools locally or integrating them with your IDE as well - do a search on your IDE name (vscode etc) and moodle coding guidelines and you should find how to set it up
mod_form.php
Outdated
| $hiddenlabel = ' '.get_string('hiddencourse', 'subcourse'); | ||
| $options[$mycourse->id] = $courselabel.$hiddenlabel; | ||
| } | ||
| } |
There was a problem hiding this comment.
the else should be on the same line as the closing bracket:
} else {
Not:
}
else {
There was a problem hiding this comment.
ok i fixed lint.
Do I need also to fix the outher parts of code that failing lint?
There was a problem hiding this comment.
all pull requests with improvements are welcome, but the main thing is to make sure a pull request doesn't add any new code that fails lint checks :-)
There was a problem hiding this comment.
Yeah but my code did not caused any NEW errors.
https://github.com/catalyst/moodle-mod_subcourse/actions/runs/6808426454
So you are saying that in order to contribute I need to fix the plugin to work without lint errors? FIXING YOUR CODE?
I am fixing errors from code that I did not made.
And runinng again the tests.
The only test failing appears to be a behat tests of completion.
I removed the line to check if this is was a case of malformed test. Because I tested myself the case and it was working as it should.
https://github.com/EsdrasCaleb/moodle-mod_subcourse/actions/runs/9571702501/job/26389413751
And voala! it worked.
So the problem that the condition in line 59 of completion_course.feature is malformed and causing the error (must have something to do with the theme change in 4.0). I could try to fix the behat test but I am not specialized in this..
Well here is a functionality that was requested one year ago, that my client is requesting also, if you could adapt my solution to your plugin I would be satisfied
#42
There was a problem hiding this comment.
thanks @EsdrasCaleb - I've merged your main one in for the 403 branch - it does introduce one new lint error related to line endings, but I'll fix that manually myself.
you don't have to fix all the lint errors when working on moodle plugins, although if you do fix them it will make a developers job easier. :-)
the main thing to be aware of is that when contributing work upstream you need to try and do it on the "latest" branch available in the upstream repo first - then once that's approved and working the developer may backport it to older branches.
There was a problem hiding this comment.
This versions has no lint erros anymore . But that 403 has
There was a problem hiding this comment.
the github actions aren't triggering any codecheckout lint errors that I could see on the latest branch, although there are still the failing behat tests which have been faliing for a while.
There was a problem hiding this comment.
thanks for sending through the PR! :-)
|
I've manually cherry-picked this one to the 401 stable branch based on your commit on the 4.3 branch - thanks! |
Fix issue #42