Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39189

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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).

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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 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 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
            ybozhko 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
            ybozhko 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
            dougiamas 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
            dougiamas 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
            markn 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
            markn 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
            marycooch 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
            marycooch 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
            markn Mark Nelson added a comment -

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

            Show
            markn Mark Nelson added a comment - +1 to Mary's suggestion. Simple and understandable.
            Hide
            ybozhko 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
            ybozhko 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
            markn 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
            markn 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
            ybozhko 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
            ybozhko 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
            markn Mark Nelson added a comment -

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

            Show
            markn Mark Nelson added a comment - Hi Yuliya, thanks for the explanation! In that case the patch looks great, am pushing to integration.
            Hide
            markn 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
            markn 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 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 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 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 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
            markn Mark Nelson added a comment -

            Works as expected, passing.

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

            Just In Time® for stable releases, thanks!

            Closing as fixed, ciao

            Show
            stronk7 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:
                  Fix Release Date:
                  14/May/13