Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Activity completion
    • Labels:
    • Testing Instructions:
      Hide
      1. Enable completion at from site admin settings
      2. Enable completion in a course from course settings
      3. Set the course to be displayed in "one section per page" mode
      4. Add multiple activities/resources to various sections with different completion criteria
      5. Make sure some of them have completion disabled
      6. Login as a student and complete a few of those activities.
      7. Make sure you can see your progress summary for each section in course index page
      8. Progress should be in the format activities completed / total activities in the section which has completion enabled
      Show
      Enable completion at from site admin settings Enable completion in a course from course settings Set the course to be displayed in "one section per page" mode Add multiple activities/resources to various sections with different completion criteria Make sure some of them have completion disabled Login as a student and complete a few of those activities. Make sure you can see your progress summary for each section in course index page Progress should be in the format activities completed / total activities in the section which has completion enabled
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-32769-master
    • Rank:
      39766

      Description

      Perhaps we should show a completion summary (if applicable, 1/3, 30%, ?)

      We also need to sort out the completion header

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Hi Aaron,

          Just adding you here, wondering if you have looked at the course completion stuff in the new split course section mode in 2.3?

          I haven't properly assesed whether we have irreversally broken this by switching to single section mode..

          Show
          Dan Poltawski added a comment - Hi Aaron, Just adding you here, wondering if you have looked at the course completion stuff in the new split course section mode in 2.3? I haven't properly assesed whether we have irreversally broken this by switching to single section mode..
          Hide
          Dan Poltawski added a comment -

          Ankit - just looking at this, have you looked at the performance impact of the functions you're calling in that?

          i.e. is it adding db queries per section?

          Show
          Dan Poltawski added a comment - Ankit - just looking at this, have you looked at the performance impact of the functions you're calling in that? i.e. is it adding db queries per section?
          Hide
          Ankit Agarwal added a comment -

          Hi Dan,
          The whole completion info is cached and fetched only once per course (refer completion_info::get_data()). So it should not have any big impact on performance, as per my understanding.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Dan, The whole completion info is cached and fetched only once per course (refer completion_info::get_data()). So it should not have any big impact on performance, as per my understanding. Thanks
          Hide
          Ankit Agarwal added a comment -

          Hi Dan,
          Putting this in "Waiting for peer-review" state.
          Feel free to pull it in whenever you like.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Dan, Putting this in "Waiting for peer-review" state. Feel free to pull it in whenever you like. Thanks
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          Looks like this is conflicting. Still not quite sure about this.

          But also styling - we prob need Barabaras input to go with what she did with the listing of activtiies.

          Show
          Dan Poltawski added a comment - Hi Ankit, Looks like this is conflicting. Still not quite sure about this. But also styling - we prob need Barabaras input to go with what she did with the listing of activtiies.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Dan,
          I have re-based the branch against the new changes you made. I used the same display class for this data as other activity summary.

          Requesting another review.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Dan, I have re-based the branch against the new changes you made. I used the same display class for this data as other activity summary. Requesting another review. Thanks
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          Sorry for delay.

          Two things to confirm with MD:

          • I think you need to stop it showing completion info 0/0 for the times when there is a section with mods but without completion.
          • I think perhaps the completion info should be on a new line rather than inline with the mods.

          Trivial: Is all this necessary, it looks a bit ugly?

          if ($completioninfo->is_enabled($thismod) != COMPLETION_TRACKING_NONE && isloggedin() &&
                                          !isguestuser() && $thismod->uservisible) {
          
          Show
          Dan Poltawski added a comment - Hi Ankit, Sorry for delay. Two things to confirm with MD: I think you need to stop it showing completion info 0/0 for the times when there is a section with mods but without completion. I think perhaps the completion info should be on a new line rather than inline with the mods. Trivial: Is all this necessary, it looks a bit ugly? if ($completioninfo->is_enabled($thismod) != COMPLETION_TRACKING_NONE && isloggedin() && !isguestuser() && $thismod->uservisible) {
          Hide
          Dan Poltawski added a comment -

          Attaching screenshot of weird 0/0 things

          Show
          Dan Poltawski added a comment - Attaching screenshot of weird 0/0 things
          Hide
          Frédéric Massart added a comment -

          Giving my feedback. I would put " $complete / $total" in the string as 2 get_string parameters. That would allow translators to change the <space>/<space> if they don't like it.

          I've also spotted on line 364 (not related to your changes though), that the mods count does not allow translators to change anything there. When internationalizing there is nothing more annoying than not being able to change the spaces before and after a colon. For instance, in French, a colon always has to be separated from the preceding word by a space.

          Show
          Frédéric Massart added a comment - Giving my feedback. I would put " $complete / $total" in the string as 2 get_string parameters. That would allow translators to change the <space>/<space> if they don't like it. I've also spotted on line 364 (not related to your changes though), that the mods count does not allow translators to change anything there. When internationalizing there is nothing more annoying than not being able to change the spaces before and after a colon. For instance, in French, a colon always has to be separated from the preceding word by a space.
          Hide
          Martin Dougiamas added a comment -

          I think:

          1) Use "Progress: 4/6" as it's clearer and connects to other strings in UI.
          2) Put it on a new line under the activities summary line.

          Show
          Martin Dougiamas added a comment - I think: 1) Use "Progress: 4/6" as it's clearer and connects to other strings in UI. 2) Put it on a new line under the activities summary line.
          Hide
          Ankit Agarwal added a comment -

          Thanks for the feedback guys,
          I have updated the patch accordingly.

          Show
          Ankit Agarwal added a comment - Thanks for the feedback guys, I have updated the patch accordingly.
          Hide
          Dan Poltawski added a comment -

          Seems OK here Ankit. I haven't tested it though.

          Show
          Dan Poltawski added a comment - Seems OK here Ankit. I haven't tested it though.
          Hide
          Ankit Agarwal added a comment - - edited

          Thanks Dan for the review, if you want to have a look at it in action you are welcome to come over to my desk anytime

          Sending this for integration.

          Show
          Ankit Agarwal added a comment - - edited Thanks Dan for the review, if you want to have a look at it in action you are welcome to come over to my desk anytime Sending this for integration.
          Hide
          Sam Hemelryk added a comment -

          Hi Ankit,

          Thanks for working on this.
          I noted the following while looking at this:

          1. The progress string should contain the complete / total as arguments like Fred suggested. I also think it should be moved to the completion lang file (saves clutter in moodle lang file).
          2. completion_info->is_enabled when called without any mod returns either COMPLETION_ENABLED or COMPLETION_DISABLED, not COMPLETION_TRACKING_NONE. This isn't really a bug as at the moment COMPLETION_DISABLED === COMPLETION_TRACKER_NONE, however if anyone changed either of those values but not both in then things would break here.
          3. In the case of the check before printing there is no need to check if completion is enabled. If $total > 0 then we know it is enabled. (because each mod check involves enabled checks).
          4. In mod check isloggedin && !isguestuser should be split out to a var to reduce calls on a section with many activities (just good practice is all)
          5. In that same mod check you should check uservisible before calling completioninfo->is_enabled. Actually there is no need to check that at all. I see the completion code is within a if statement that checks that already.
          6. instantiation of completion_info should definitely be done AFTER we check $section->sequence isn't empty.

          While reviewing I made the changes I consider worth making and have the following branch:
          https://github.com/samhemelryk/moodle/compare/wip-MDL-32769-m23

          I've tested it but only lightly, Ankit could you please look at making these changes and testing them, feel free to pinch the commit I made on the noted branch.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ankit, Thanks for working on this. I noted the following while looking at this: The progress string should contain the complete / total as arguments like Fred suggested. I also think it should be moved to the completion lang file (saves clutter in moodle lang file). completion_info->is_enabled when called without any mod returns either COMPLETION_ENABLED or COMPLETION_DISABLED, not COMPLETION_TRACKING_NONE. This isn't really a bug as at the moment COMPLETION_DISABLED === COMPLETION_TRACKER_NONE, however if anyone changed either of those values but not both in then things would break here. In the case of the check before printing there is no need to check if completion is enabled. If $total > 0 then we know it is enabled. (because each mod check involves enabled checks). In mod check isloggedin && !isguestuser should be split out to a var to reduce calls on a section with many activities (just good practice is all) In that same mod check you should check uservisible before calling completioninfo->is_enabled. Actually there is no need to check that at all. I see the completion code is within a if statement that checks that already. instantiation of completion_info should definitely be done AFTER we check $section->sequence isn't empty. While reviewing I made the changes I consider worth making and have the following branch: https://github.com/samhemelryk/moodle/compare/wip-MDL-32769-m23 I've tested it but only lightly, Ankit could you please look at making these changes and testing them, feel free to pinch the commit I made on the noted branch. Cheers Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Ankit Agarwal added a comment -

          Hi Sam,
          I went through your changes, they look good. I did some testing as well, all seems to be working fine for me.
          I have pulled in your changes to my branch.

          Submitting for integration again.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Sam, I went through your changes, they look good. I did some testing as well, all seems to be working fine for me. I have pulled in your changes to my branch. Submitting for integration again. Thanks
          Hide
          Sam Hemelryk added a comment -

          Cool, thanks Ankit, this has been integrated now

          Show
          Sam Hemelryk added a comment - Cool, thanks Ankit, this has been integrated now
          Hide
          Rossiani Wijaya added a comment -

          Hi Ankit,
          This looks great.

          However I found an issue with page resource completion tracking. This issue occurs without your fixed. I created a new issue to address that (MDL-33964).

          Passing this issue.

          Show
          Rossiani Wijaya added a comment - Hi Ankit, This looks great. However I found an issue with page resource completion tracking. This issue occurs without your fixed. I created a new issue to address that ( MDL-33964 ). Passing this issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

          Many, many thanks for your hard work!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao
          Hide
          Tim Barker added a comment -

          Removed QA tests required flag. We should only include QA tests for features on the current roadmap. Minor features that just missed out on getting into the previous release must be comprehensively tested prior to integration AND come with suitable automated regression tests such as unit tests.

          Show
          Tim Barker added a comment - Removed QA tests required flag. We should only include QA tests for features on the current roadmap. Minor features that just missed out on getting into the previous release must be comprehensively tested prior to integration AND come with suitable automated regression tests such as unit tests.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: