search

gjbarnard / moodle-format_topcoll

Collapsed Topics course format for Moodle.
http://docs.moodle.org/en/Collapsed_Topics_course_format
GNU General Public License v3.0
35 stars 60 forks source link

Ready for Review: add setting for displaying deadline and submissions #81

Closed NinaHerrmann closed 3 years ago

NinaHerrmann commented 3 years ago

I am working on it but just to get an idea of the things I want to do:

If you have any remarks let me know, otherwise I would continue by writing phpunit test :slightly_smiling_face:

gjb2048 commented 3 years ago

When completed, please combine into a single commit so that its easier for me to backport.

gjb2048 commented 3 years ago

Can't 'https://github.com/gjb2048/moodle-format_topcoll/pull/81/commits/5d3c540ab7d32de39ba30eb081dbf8c74882f525#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R699-R702' use 'implode' (https://www.php.net/manual/en/function.implode.php) instead?

With https://github.com/gjb2048/moodle-format_topcoll/pull/81/commits/5d3c540ab7d32de39ba30eb081dbf8c74882f525#diff-972c351ef4188f1f82cc82207e98147ccbd23e43ddf222a3e5c3f8df5e475d04R120 - better if 'Show additional information for: {$a} in the course';

As ''Show additional information for {$a} in the course'; - then better English for https://github.com/gjb2048/moodle-format_topcoll/pull/81/commits/5d3c540ab7d32de39ba30eb081dbf8c74882f525#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R385-R391 with https://github.com/gjb2048/moodle-format_topcoll/pull/81/commits/5d3c540ab7d32de39ba30eb081dbf8c74882f525#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R386 to be 'assignments, quizzes....' etc. And even better if code can be 'assignments, quizzes, lessons and feedbacks' etc. So a trailing 'and' instead of a comma.

NinaHerrmann commented 3 years ago

Thanks for your fast feedback! I implemented it, and will continue by writing test!

NinaHerrmann commented 3 years ago

Hey from my point of view I am finished. You might wonder why I did not include a testcase for setting admin settings to 1 for each activity and check whether the default value is set to 1. I spent some time trying to find a way to avoid caching of moodle but somehow $courseformatoptions in topcoll/lib.php is cached. (https://github.com/gjb2048/moodle-format_topcoll/blob/9b058ac2de33cf3932cf7804794fd75bd608c5fd/lib.php#L386). If I include the logic before the if it works just fine. If you have any idea to avoid that please contact me. In case you agree that the test is sufficient for a rather small feature, please tell me and I will merge all changes into one commit. Cheers, Nina

gjb2048 commented 3 years ago

Hey from my point of view I am finished. You might wonder why I did not include a testcase for setting admin settings to 1 for each activity and check whether the default value is set to 1. I spent some time trying to find a way to avoid caching of moodle but somehow $courseformatoptions in topcoll/lib.php is cached. (

https://github.com/gjb2048/moodle-format_topcoll/blob/9b058ac2de33cf3932cf7804794fd75bd608c5fd/lib.php#L386

). If I include the logic before the if it works just fine. If you have any idea to avoid that please contact me. In case you agree that the test is sufficient for a rather small feature, please tell me and I will merge all changes into one commit. Cheers, Nina

Line 386 is standard practice if you look at core topics lib.php.

gjb2048 commented 3 years ago

https://github.com/gjb2048/moodle-format_topcoll/pull/81/files#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R538 etc. is invalid as you're getting a site setting for the format that does not exist - i.e. /course/format/topcoll/settings.php.

gjb2048 commented 3 years ago

Multiple line comments (https://github.com/gjb2048/moodle-format_topcoll/pull/81/files#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R532-R535) need to use '/*' etc.

gjb2048 commented 3 years ago

Don't add a comma when it is not needed: https://github.com/gjb2048/moodle-format_topcoll/pull/81/files#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R529

gjb2048 commented 3 years ago

Here: https://github.com/gjb2048/moodle-format_topcoll/pull/81/files#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R532-R545 have this logic: https://github.com/gjb2048/moodle-format_topcoll/pull/81/files#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R755-R758 sort of thing.

gjb2048 commented 3 years ago

So only add https://github.com/gjb2048/moodle-format_topcoll/blob/70bbd4264c974b07a901bc69700ba8d0b92c8592/lib.php#L543-L545 on a https://github.com/gjb2048/moodle-format_topcoll/blob/70bbd4264c974b07a901bc69700ba8d0b92c8592/lib.php#L537

gjb2048 commented 3 years ago

https://github.com/gjb2048/moodle-format_topcoll/pull/81/files#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R386-R406 needs to be within https://github.com/gjb2048/moodle-format_topcoll/pull/81/files#diff-8f5ff72ef1cf59199cfc4bbf16f2475d09e448601242f0d2c51bb94bd4aa1395R407

gjb2048 commented 3 years ago

@NinaHerrmann Any updates to this please? As I don't want to have to increase my workload when making a 3.10 version of CT and forward porting this. Or you'll have to update as 'master' will become for M3.10.

gjb2048 commented 3 years ago

@NinaHerrmann Ok, I've made more changes, so you'll need to rebase.

gjb2048 commented 3 years ago

@NinaHerrmann Ok, getting annoyed now, spent time on this and no reply from you.

gjb2048 commented 3 years ago

@NinaHerrmann Still waiting for a reply from you.

NinaHerrmann commented 3 years ago

@gjb2048 Sorry for the late answer I will rebase it and come back when I tested it.

NinaHerrmann commented 3 years ago

Sorry for the mess, I looked for the fast way to merge the commits into one to make it clearer. I will move it to a new pull request and move all your comments to the new one.

gjb2048 commented 3 years ago

@NinaHerrmann So has #91 superseded this?

gjb2048 commented 3 years ago

Superceded by #91.