Moodle
  1. Moodle
  2. MDL-30764

Turning off Grouping requires editing activity twice

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.3
    • Fix Version/s: 2.1.4, 2.2.1
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      0. Enable the 'groupmembersonly' option on the test server (under development/experimental/beware of the leopard/disused filing cabinet/experimental settings, or some similar location). Add a grouping on your test course if you don't already have one.

      1. Create a new Page activity. Fill in all the required fields, then scroll down to the common settings and show advanced if required.
      + Note that the grouping dropdown is correctly greyed out until you turn on 'Group members only'.

      2. Turn on 'Group members only' and select the grouping. Save and display.

      3. Edit page settings. Set the grouping option to 'None' and turn off 'Group members only'. Save and return to course.
      + The grouping should be turned off (not displayed next to the activity name).

      Currently this does not work - it turns off group members only but does not turn off the grouping.

      Show
      0. Enable the 'groupmembersonly' option on the test server (under development/experimental/beware of the leopard/disused filing cabinet/experimental settings, or some similar location). Add a grouping on your test course if you don't already have one. 1. Create a new Page activity. Fill in all the required fields, then scroll down to the common settings and show advanced if required. + Note that the grouping dropdown is correctly greyed out until you turn on 'Group members only'. 2. Turn on 'Group members only' and select the grouping. Save and display. 3. Edit page settings. Set the grouping option to 'None' and turn off 'Group members only'. Save and return to course. + The grouping should be turned off (not displayed next to the activity name). Currently this does not work - it turns off group members only but does not turn off the grouping.
    • Workaround:
      Hide

      Edit settings twice (once to turn off grouping, then again to turn off groupmembersonly).

      Show
      Edit settings twice (once to turn off grouping, then again to turn off groupmembersonly).
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-30764-master
    • Rank:
      33692

      Description

      If you decide to turn off both the 'Grouping' and 'Group members only' options on some activity types, the grouping change is not saved because the field gets disabled, so it is not set to 'none'.

      My proposed solution is that the 'disable' logic should only apply when adding a new activity or one which has grouping set to 'None'. If grouping value in database is not none, then the field will not be disabled, regardless of the setting of groupmembersonly.

      NOTE: The same problem also affects ordinary use of groupings if you set group mode to 'None'. I found it first with groupmembersonly, but am fixing for both.

        Activity

        Hide
        Sam Marshall added a comment -

        Submitting for integration, this is a simple bugfix and hopefully ok.

        Note: You can see the change more clearly if you ignore whitespace (add ?w=1 on the github URLs). I just added the outer if; there is no change at all to the inner part except indent.

        Show
        Sam Marshall added a comment - Submitting for integration, this is a simple bugfix and hopefully ok. Note: You can see the change more clearly if you ignore whitespace (add ?w=1 on the github URLs). I just added the outer if; there is no change at all to the inner part except indent.
        Hide
        Aparup Banerjee added a comment -

        just pushing this back on track with our review process, next up peer-reviewing ..

        Show
        Aparup Banerjee added a comment - just pushing this back on track with our review process, next up peer-reviewing ..
        Hide
        Rajesh Taneja added a comment -

        Code looks Good, Sam
        Pushing it for integration review.

        Show
        Rajesh Taneja added a comment - Code looks Good, Sam Pushing it for integration review.
        Hide
        Sam Hemelryk added a comment -

        Thanks Sam this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Sam this has been integrated now
        Hide
        Adrian Greeve added a comment -

        The additional grey comments next to the page are removed when the settings are changed to no groupings.
        Nice fix.
        Test passed.
        Thanks.

        Show
        Adrian Greeve added a comment - The additional grey comments next to the page are removed when the settings are changed to no groupings. Nice fix. Test passed. Thanks.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

        Now... disconnect, relax and enjoy the next days, yay!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao
        Hide
        Marina Glancy added a comment -

        I'd like to come back to this issue. Correct me if I'm wrong. At the moment when the "Experimental setting" $CFG->enablegroupmembersonly is off (and it is off by default), the 'Grouping' field appears and is very confusing. For example, I create mod_assign, set visible or separate groups and the grouping field becomes enabled. When I select grouping it has absolutely no effect. Students from groups not in this grouping can still see and access assignment.

        I discovered while working on MDL-34397. Among other things I'm making the cm_info properties read-only with the getter method when required. I was going to make $cm->groupingid calculated (overwrite to 0 if $CFG->enablegroupmembersonly is not set) but noticed that the form displays it anyway.

        Show
        Marina Glancy added a comment - I'd like to come back to this issue. Correct me if I'm wrong. At the moment when the "Experimental setting" $CFG->enablegroupmembersonly is off (and it is off by default), the 'Grouping' field appears and is very confusing. For example, I create mod_assign, set visible or separate groups and the grouping field becomes enabled. When I select grouping it has absolutely no effect. Students from groups not in this grouping can still see and access assignment. I discovered while working on MDL-34397 . Among other things I'm making the cm_info properties read-only with the getter method when required. I was going to make $cm->groupingid calculated (overwrite to 0 if $CFG->enablegroupmembersonly is not set) but noticed that the form displays it anyway.
        Hide
        Sam Marshall added a comment -

        Marina: Regarding this issue, the 'grouping' field is still valid for activities that support group mode, even if groupmembersonly is turned off.

        Grouping does two things:

        1. Activity supports groups --> Grouping chooses which set of groups that you see (only the groups from that grouping will be shown in group dropdown)

        2. Whether activity supports groups or not, and IF groupmembersonly is turned on --> grouping limits access to people from selected grouping only

        The user interface is a bit confusing and the correct approach would be:

        1. Remove the option about whether to enable groupmembersonly in advanced setting.
        2. Remove the groupmembersonly option
        3. System should behave similar to if groupmembersonly is always on:

        • If an activity is set to 'separate groups' and you aren't in a group (either 'a group at all' if no grouping is selected, or 'a group in that grouping' if a grouping is selected) then the activity should not show on the course page etc, which is what groupmembersonlyu does.
        • If an activity is set to 'visible groups' then it should display to everyone
        • If an activity does not support groups, then if you select a grouping, it should not appear to people not in that grouping. If you don't select a grouping it should appear to everyone.

        But really that's a separate issue??

        Show
        Sam Marshall added a comment - Marina: Regarding this issue, the 'grouping' field is still valid for activities that support group mode, even if groupmembersonly is turned off. Grouping does two things: 1. Activity supports groups --> Grouping chooses which set of groups that you see (only the groups from that grouping will be shown in group dropdown) 2. Whether activity supports groups or not, and IF groupmembersonly is turned on --> grouping limits access to people from selected grouping only The user interface is a bit confusing and the correct approach would be: 1. Remove the option about whether to enable groupmembersonly in advanced setting. 2. Remove the groupmembersonly option 3. System should behave similar to if groupmembersonly is always on: If an activity is set to 'separate groups' and you aren't in a group (either 'a group at all' if no grouping is selected, or 'a group in that grouping' if a grouping is selected) then the activity should not show on the course page etc, which is what groupmembersonlyu does. If an activity is set to 'visible groups' then it should display to everyone If an activity does not support groups, then if you select a grouping, it should not appear to people not in that grouping. If you don't select a grouping it should appear to everyone. But really that's a separate issue??

          People

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

            Dates

            • Created:
              Updated:
              Resolved: