Moodle
  1. Moodle
  2. MDL-39189

"Roles can assign badges" feature does not work with multiple roles

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Badges
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a course badge with criteria "Manual issue by a role". Set up criteria options with several roles (e.g. teacher and course creator). Enable the badge.
      2. Create a user account and give it more than one role in this course.
      3. Login as this new user.
      4. Go to award badge link from "Badge management" or click "Award badge" from Recipients tab.
      5. If the user has more than one role that they can use to award the badge, they should have a drop down to select the role they would like to use.
      6. Award the badge, make sure that nothing breaks.

      Show
      1. Create a course badge with criteria "Manual issue by a role". Set up criteria options with several roles (e.g. teacher and course creator). Enable the badge. 2. Create a user account and give it more than one role in this course. 3. Login as this new user. 4. Go to award badge link from "Badge management" or click "Award badge" from Recipients tab. 5. If the user has more than one role that they can use to award the badge, they should have a drop down to select the role they would like to use. 6. Award the badge, make sure that nothing breaks.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-39189_master
    • Rank:
      49792

      Description

      Roles can assign badges feature does not work with multiple roles.

      Comment from MDL-39049:

      Hi Yuliya,

      While the patch fixes this specific issue - I spotted a logic error.
      A user can have multiple roles in a course and you are only checking the first one returned from "get_user_roles" - you need to loop through that list and only print an error if none of the user roles are in the accepted roles list.
      How to reproduce: Create a badge that can be awarded by the Teacher role. Give a user teacher + course creator roles in a course. Attempt to award the badge as that teacher (should be allowed).

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          Hello, there is a general rule that actions should be allowed based on capabilities, not roles. In <2.0 we used roles for performance reasons because we could not do SQL joins on capabilities, the enrolment plugins should be the only exception. We should be replacing all role-based hacks (such as graded roles in gradebook) with capabilities. In any case course creator role should be used only for creation of courses, nothing else - using it in unrelated examples creates unnecessary confusion.

          Show
          Petr Škoda added a comment - Hello, there is a general rule that actions should be allowed based on capabilities, not roles. In <2.0 we used roles for performance reasons because we could not do SQL joins on capabilities, the enrolment plugins should be the only exception. We should be replacing all role-based hacks (such as graded roles in gradebook) with capabilities. In any case course creator role should be used only for creation of courses, nothing else - using it in unrelated examples creates unnecessary confusion.
          Hide
          Petr Škoda added a comment - - edited

          On the other hand there does not seem to be any other way (apart from groups and groupings) to restrict manual badge assignments...

          Show
          Petr Škoda added a comment - - edited On the other hand there does not seem to be any other way (apart from groups and groupings) to restrict manual badge assignments...
          Hide
          Damyon Wiese added a comment -

          Note: I went through the same thoughts about using roles as Petr when looking at the initial badges patch - the only alternative I can see would be specify a capability that is required to assign a badge - but then teachers are exposed to all the capabilities and need to know which roles have each capability in order to achieve the same thing as just choosing a role.

          Show
          Damyon Wiese added a comment - Note: I went through the same thoughts about using roles as Petr when looking at the initial badges patch - the only alternative I can see would be specify a capability that is required to assign a badge - but then teachers are exposed to all the capabilities and need to know which roles have each capability in order to achieve the same thing as just choosing a role.
          Hide
          Yuliya Bozhko added a comment -

          There is a capability that defines which role can issue a badge.

          I was following similar idea used by course completion that has manual completion by a role.

          Show
          Yuliya Bozhko added a comment - There is a capability that defines which role can issue a badge. I was following similar idea used by course completion that has manual completion by a role.
          Hide
          Martin Dougiamas added a comment -

          Given that it looks we are keeping roles here at this stage, is the attached patch good to go?

          Show
          Martin Dougiamas added a comment - Given that it looks we are keeping roles here at this stage, is the attached patch good to go?
          Hide
          Mark Nelson added a comment -

          Hi Yuliya,

          Patch looks good to me. Just a quick note -

          if (empty($CFG->badges_allowcoursebadges)) {
              print_error('coursebadgesdisabled', 'badges');
          } else {
              require_login($badge->courseid);
              $navurl = new moodle_url('/badges/index.php', array('type' => $badge->type, 'id' => $badge->courseid));
          }
          
          1. The else is not necessary in the code above as the print_error will throw a moodle_exception and the rest of the code will not be executed.
          2. You have a variable named "$accepted_roles", in Moodle we avoid using underscores in our variable names (please see http://docs.moodle.org/dev/Coding_style#Variables). Really this should be changed to $acceptedroles.
          3. The wording "Please select a role on behalf of which you would like to award this badge:" is kind of awkward, how about - "Please select the role you would like to award this badge on behalf of"?
          4. I also noticed when I changed the role in the "Please select a role on behalf of which you would like to award this badge:" select box that the existing badge recipients changed. Should this list really change because the role has changed? The badge is the same regardless of which role awarded it, right? Or am I wrong about that assumption?

          Thanks,

          Mark

          Show
          Mark Nelson added a comment - Hi Yuliya, Patch looks good to me. Just a quick note - if (empty($CFG->badges_allowcoursebadges)) { print_error('coursebadgesdisabled', 'badges'); } else { require_login($badge->courseid); $navurl = new moodle_url('/badges/index.php', array('type' => $badge->type, 'id' => $badge->courseid)); } The else is not necessary in the code above as the print_error will throw a moodle_exception and the rest of the code will not be executed. You have a variable named "$accepted_roles", in Moodle we avoid using underscores in our variable names (please see http://docs.moodle.org/dev/Coding_style#Variables ). Really this should be changed to $acceptedroles. The wording "Please select a role on behalf of which you would like to award this badge:" is kind of awkward, how about - "Please select the role you would like to award this badge on behalf of"? I also noticed when I changed the role in the "Please select a role on behalf of which you would like to award this badge:" select box that the existing badge recipients changed. Should this list really change because the role has changed? The badge is the same regardless of which role awarded it, right? Or am I wrong about that assumption? Thanks, Mark
          Hide
          Mary Cooch added a comment -

          Just a quick comment: how about "Please select the role you'd like to use to award this badge"?

          Show
          Mary Cooch added a comment - Just a quick comment: how about "Please select the role you'd like to use to award this badge"?
          Hide
          Mark Nelson added a comment -

          +1 to Mary's suggestion. Simple and understandable.

          Show
          Mark Nelson added a comment - +1 to Mary's suggestion. Simple and understandable.
          Hide
          Yuliya Bozhko added a comment -

          Hi Mark,

          I applied the changed you asked for. Hope it looks better now.

          As for point 4, list of recipients changed based on the role that awards a badge. This is done to avoid issuing badges to the same person over again. Also, issuing by role criteria can have ALL condition where it would be important to see which role already issued the badge and which hasn't.

          Yuliya

          Show
          Yuliya Bozhko added a comment - Hi Mark, I applied the changed you asked for. Hope it looks better now. As for point 4, list of recipients changed based on the role that awards a badge. This is done to avoid issuing badges to the same person over again. Also, issuing by role criteria can have ALL condition where it would be important to see which role already issued the badge and which hasn't. Yuliya
          Hide
          Mark Nelson added a comment -

          Hi Yuliya,

          Slightly confused about this statement - "This is done to avoid issuing badges to the same person over again." As a user with multiple roles awarding a badge I can simply change my role and award a badge to a user again as they are not listed in the recipient list once the role has been changed. Regarding your last point, I agree, but what happens when the badge is awarded to a user twice by different roles? Which role do we display, or do we display both? Sorry if my lack of experience and understanding of the badges system is causing confusion.

          Show
          Mark Nelson added a comment - Hi Yuliya, Slightly confused about this statement - "This is done to avoid issuing badges to the same person over again." As a user with multiple roles awarding a badge I can simply change my role and award a badge to a user again as they are not listed in the recipient list once the role has been changed. Regarding your last point, I agree, but what happens when the badge is awarded to a user twice by different roles? Which role do we display, or do we display both? Sorry if my lack of experience and understanding of the badges system is causing confusion.
          Hide
          Yuliya Bozhko added a comment -

          Hi Mark,

          It's not the user who issues the badge, but a role. Users with multiple roles will have an option to use both of them to issue badges. If the badge is awarded twice by different roles, both roles are displayed and criteria (ALL/ANY) is given. In some cases it will be required for a badge to be awarded by several different roles. When there are, let's say, three teachers in the course and they go to award a badge as a teacher, each of them will see the list of users who already got the badge from a teacher, so they won't need to award the badge again.

          Show
          Yuliya Bozhko added a comment - Hi Mark, It's not the user who issues the badge, but a role. Users with multiple roles will have an option to use both of them to issue badges. If the badge is awarded twice by different roles, both roles are displayed and criteria (ALL/ANY) is given. In some cases it will be required for a badge to be awarded by several different roles. When there are, let's say, three teachers in the course and they go to award a badge as a teacher, each of them will see the list of users who already got the badge from a teacher, so they won't need to award the badge again.
          Hide
          Mark Nelson added a comment -

          Hi Yuliya, thanks for the explanation! In that case the patch looks great, am pushing to integration.

          Show
          Mark Nelson added a comment - Hi Yuliya, thanks for the explanation! In that case the patch looks great, am pushing to integration.
          Hide
          Mark Nelson added a comment -

          Just a note, personally I would not include the space at the end of the sentence in your language file, but would do it after the get_string call. However, I could not find any rule on this so I guess it comes down to personal preference.

          Show
          Mark Nelson added a comment - Just a note, personally I would not include the space at the end of the sentence in your language file, but would do it after the get_string call. However, I could not find any rule on this so I guess it comes down to personal preference.
          Hide
          Damyon Wiese added a comment -

          Hi Yuliya,

          The added ui elements / functionality do not make sense to me. If I have multiple roles that can be used to award a badge, what difference does it make if I choose a specific role to award a badge? By changing this role it allows me to award the same badge to the same user more than once. And it is hard to see which users have been awarded a badge without switching between all the roles I can assign from.

          Show
          Damyon Wiese added a comment - Hi Yuliya, The added ui elements / functionality do not make sense to me. If I have multiple roles that can be used to award a badge, what difference does it make if I choose a specific role to award a badge? By changing this role it allows me to award the same badge to the same user more than once. And it is hard to see which users have been awarded a badge without switching between all the roles I can assign from.
          Hide
          Damyon Wiese added a comment -

          Passing unit tests, passing behat tests, passing sanity test.

          Integrated to master, thanks Yuliya!

          Notes: As per my above comment, I think the UI could do with some improvement in this area - it is not obvious what the point of assigning the same badge from multiple roles is. This can be done later though.

          Show
          Damyon Wiese added a comment - Passing unit tests, passing behat tests, passing sanity test. Integrated to master, thanks Yuliya! Notes: As per my above comment, I think the UI could do with some improvement in this area - it is not obvious what the point of assigning the same badge from multiple roles is. This can be done later though.
          Hide
          Mark Nelson added a comment -

          Works as expected, passing.

          Show
          Mark Nelson added a comment - Works as expected, passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just In Time® for stable releases, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Just In Time® for stable releases, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: