Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Course completion
    • Labels:
    • Testing Instructions:
      Hide

      This is a big test all about course completions. The tester is required to be familiar with completions tracking etc somewhat.

      • enable course completion (search for 'completion' and you will see the setting and many other relevant ones too)

      test all (many) UI related to course completion mentioned here : http://docs.moodle.org/dev/Course_completion
      in particular

      1. test that there are no changes to all the settings mentioned here and that they function as documented.
      2. test course reporting for completion (point 6)
      3. test site reporting for course if possible
      4. test cron does still mark a user's course complete when all criteria are met.
      5. test backup and restore still has course completion data intact.
        (all test instructions can be further looked up in the relevant section in the doc
      1. for completeness (no pun), you should also test and upgrade from (< 2012042300.02) to master.
      Show
      This is a big test all about course completions. The tester is required to be familiar with completions tracking etc somewhat. enable course completion (search for 'completion' and you will see the setting and many other relevant ones too) test all (many) UI related to course completion mentioned here : http://docs.moodle.org/dev/Course_completion in particular test that there are no changes to all the settings mentioned here and that they function as documented. test course reporting for completion ( point 6 ) test site reporting for course if possible test cron does still mark a user's course complete when all criteria are met. test backup and restore still has course completion data intact. (all test instructions can be further looked up in the relevant section in the doc for completeness (no pun), you should also test and upgrade from (< 2012042300.02) to master.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      42566

      Description

      • we need to implement events for course completion (see parent task (also this mentions the issue) ).
        adding to /lib/db/events.php is a big no no as mentioned in the file.

      I've noted that the architecture of completion could be made to be more pluggable/modular, but that will be better done in future when progress tracking moves forward.

      • also noting that any changes here must preserve all compatibility but that doesn't look like a problem atm.

        Activity

        Hide
        Aaron Barnes added a comment -

        Hey Aparaup,

        I think if we are going to move all the files now, we might as well do it right! How about the following structure:

        completion/criteria.php (was completion_criteria.php)
        completion/criteria/grade.class.php (was completion_criteria_grade.php)
        completion/criteria/date.class.php
        completion/completion_aggregation.class.php (was completion_aggregation.php)
        completion/completion_criteria_completion.class.php (was completion_criteria_completion.php)
        completion/completion_completion.class.php (was completion_completion.php)

        What do you think?

        I think we should at least group the criteria types into a subfolder, even if we don't rename this to be .class.php

        Also, is using relative include paths OK?

        Keep up the good work!
        Aaron

        Show
        Aaron Barnes added a comment - Hey Aparaup, I think if we are going to move all the files now, we might as well do it right! How about the following structure: completion/criteria.php (was completion_criteria.php) completion/criteria/grade.class.php (was completion_criteria_grade.php) completion/criteria/date.class.php completion/completion_aggregation.class.php (was completion_aggregation.php) completion/completion_criteria_completion.class.php (was completion_criteria_completion.php) completion/completion_completion.class.php (was completion_completion.php) What do you think? I think we should at least group the criteria types into a subfolder, even if we don't rename this to be .class.php Also, is using relative include paths OK? Keep up the good work! Aaron
        Hide
        Aparup Banerjee added a comment -

        Hi Aaron,
        i did think about improving the files structure, was going to leave that till later but i guess i can do refactoring. Its just that tests will have to be thoroughly testing for any regressions.
        atm, things seem to test fine for me so i'll put up some more quick changes.

        Show
        Aparup Banerjee added a comment - Hi Aaron, i did think about improving the files structure, was going to leave that till later but i guess i can do refactoring. Its just that tests will have to be thoroughly testing for any regressions. atm, things seem to test fine for me so i'll put up some more quick changes.
        Hide
        Aaron Barnes added a comment -

        Great, let me know!

        I'll look into MDL-34232 tomorrow morning.

        Cheers,
        Aaron

        Show
        Aaron Barnes added a comment - Great, let me know! I'll look into MDL-34232 tomorrow morning. Cheers, Aaron
        Hide
        Aparup Banerjee added a comment -

        cool, thats done. (moving files into criteria)

        note: as discussed, file names weren't changed at this point due to the class names within and also due to related table names.

        perhaps one more quick peer-review and then this should likely go up for integration review.

        Show
        Aparup Banerjee added a comment - cool, thats done. (moving files into criteria) note: as discussed, file names weren't changed at this point due to the class names within and also due to related table names. perhaps one more quick peer-review and then this should likely go up for integration review.
        Hide
        Aaron Barnes added a comment -

        Hey Aparup,

        Sorry to be a pain, it all looks good but could we leave completion_criteria_completion in the completion/ folder to distinguish it from the other files in there. completion_criteria_completion isn't a criteria type, but rather the class that loads a user's completion data for the criteria.

        Otherwise the patch is awesome,
        Aaron

        Show
        Aaron Barnes added a comment - Hey Aparup, Sorry to be a pain, it all looks good but could we leave completion_criteria_completion in the completion/ folder to distinguish it from the other files in there. completion_criteria_completion isn't a criteria type, but rather the class that loads a user's completion data for the criteria. Otherwise the patch is awesome, Aaron
        Hide
        Aparup Banerjee added a comment - - edited

        np Aaron, thanks, i didn't realise that. thats done.
        hm, hope that change doesn't break this part... https://github.com/nebgor/moodle/compare/mistress...MDL-34225#L3R175

        Show
        Aparup Banerjee added a comment - - edited np Aaron, thanks, i didn't realise that. thats done. hm, hope that change doesn't break this part... https://github.com/nebgor/moodle/compare/mistress...MDL-34225#L3R175
        Hide
        Aaron Barnes added a comment -

        Hi Aparup,

        It won't, the completion class isn't loaded via that factory method.

        Cheers,
        Aaron

        Show
        Aaron Barnes added a comment - Hi Aparup, It won't, the completion class isn't loaded via that factory method. Cheers, Aaron
        Hide
        Aaron Barnes added a comment -

        Looks great to me!

        Show
        Aaron Barnes added a comment - Looks great to me!
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Hi Apu,

        The changes look good to me, and it all appears to make sense.
        Before this gets integrated I think it may be worth just checking with Petr, or Martin about this move.
        But hopefully no problems, I think it makes more sense to be there than in the lib directory.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Apu, The changes look good to me, and it all appears to make sense. Before this gets integrated I think it may be worth just checking with Petr, or Martin about this move. But hopefully no problems, I think it makes more sense to be there than in the lib directory. Cheers Sam
        Hide
        Aparup Banerjee added a comment -

        ah yep, MD had given his +1 for /completion awhile back (on condition it was backward compatible).

        Show
        Aparup Banerjee added a comment - ah yep, MD had given his +1 for /completion awhile back (on condition it was backward compatible).
        Hide
        Sam Hemelryk added a comment -

        Thanks Apu, this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Apu, this has been integrated now
        Hide
        Aparup Banerjee added a comment -

        i'm putting a dev docs requried tag here. feel free to remove it. i was just wondering if there were docs out there that needed updating. i haven't spotted anything yet though.

        Show
        Aparup Banerjee added a comment - i'm putting a dev docs requried tag here. feel free to remove it. i was just wondering if there were docs out there that needed updating. i haven't spotted anything yet though.
        Hide
        Rossiani Wijaya added a comment -

        This looks good.

        Thanks Apu.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This looks good. Thanks Apu. Test passed.
        Hide
        Aparup Banerjee added a comment -

        yay, it works!

        This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

        Thank you all for taking the time to get us here.

        cheers!

        Show
        Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!
        Hide
        Aparup Banerjee added a comment -

        i've removed the dev_docs_required now. no other docs found mentioning location of code etc. (http://docs.moodle.org/dev/Course_completion)

        Show
        Aparup Banerjee added a comment - i've removed the dev_docs_required now. no other docs found mentioning location of code etc. ( http://docs.moodle.org/dev/Course_completion )

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: