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

Award criteria for ALL cohort - WHERE clause broken

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Peer review in progress
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 3.5.12, 3.6.10, 3.7.6, 3.8.3, 3.9, 3.10.1, 3.11
    • Fix Version/s: None
    • Component/s: Badges
    • Labels:
    • Testing Instructions:
      Hide

      I'll pull some testing instructions out of my comments below.

      The code ends up functioning correctly as-is, but it's only because review() in award_criteria_cohort.php catches the error. Let me see if I can explain.

      Consider the following scenario:

      1. Create two cohorts (id X and Y)
      2. Create a badge that requires a user to be in both cohorts. Don't enable the badge.
      3. Create a user (id Z) and put them in the second cohort (id Y).
      4. Enable the badge (this is only evident when review_all_criteria() in badge/classes/badge.php is called)

      The SQL generated by get_completed_criteria_sql() in award_criteria_cohort.php would produce the user ID (Z) of the user, even though that user is not in both cohorts, because the WHERE clause doesn't aggregate the AND clauses correctly. The generated WHERE clause ends up reading:

      WHERE bi.badgeid IS NULL AND u.id != :guestid AND
          u.deleted = 0 AND cm.cohortid = :cohortid2

      When it should read:

      WHERE bi.badgeid IS NULL AND u.id != :guestid AND
          u.deleted = 0 AND cm.cohortid = :cohortid1 AND cm.cohortid = :cohortid2

       

      Show
      I'll pull some testing instructions out of my comments below. The code ends up functioning correctly as-is, but it's only because review() in award_criteria_cohort.php catches the error . Let me see if I can explain. Consider the following scenario: Create two cohorts (id X and Y) Create a badge that requires a user to be in both cohorts. Don't enable the badge. Create a user (id Z) and put them in the second cohort (id Y). Enable the badge (this is only evident when review_all_criteria() in badge/classes/badge.php is called) The SQL generated by get_completed_criteria_sql() in award_criteria_cohort.php would produce the user ID (Z) of the user, even though that user is not in both cohorts, because the WHERE clause doesn't aggregate the AND clauses correctly. The generated WHERE clause ends up reading: WHERE bi.badgeid IS NULL AND u.id != :guestid AND     u.deleted = 0 AND cm.cohortid = :cohortid2 When it should read: WHERE bi.badgeid IS NULL AND u.id != :guestid AND     u.deleted = 0 AND cm.cohortid = :cohortid1 AND cm.cohortid = :cohortid2  
    • Affected Branches:
      MOODLE_310_STABLE, MOODLE_311_STABLE, MOODLE_35_STABLE, MOODLE_36_STABLE, MOODLE_37_STABLE, MOODLE_38_STABLE, MOODLE_39_STABLE
    • Pull 3.9 Branch:
    • Pull 3.10 Branch:
      MDL-69102-310
    • Pull 3.11 Branch:
      MDL-69102-311
    • Pull Master Branch:
      MDL-69102-master

      Description

      The code to award a badge when the criteria requires being a member of multiple cohorts is broken.

      It appears that the WHERE clause that's generated when seeing if a person is a member of ALL required cohorts doesn't work.

      In criteria/award_criteria_cohort line 247ish reads:

      $where = ' AND cm.cohortid = :cohortid'.$i;

      When I believe it should read:

      $where .= ' AND cm.cohortid = :cohortid'.$i;

      So a person could gain the badge if they were only a member of the last cohort, not all cohorts, because the AND clause only includes the last cohortid.

      I was looking at MDL-63120 and ran across this line, which I'm pretty sure is just a simple oversight bug, but I'm definitely not a) a php programmer or b) overly familiar with the cohort award code.

        Attachments

          Activity

            People

            Assignee:
            Unassigned Unassigned
            Reporter:
            martygilbert Marty Gilbert
            Peer reviewer:
            Ilya Tregubov
            Participants:
            Component watchers:
            Yuliya Bozhko, Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Ilya Tregubov, Sara Arjona (@sarjona)
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Dates

              Created:
              Updated:

                Time Tracking

                Estimated:
                Original Estimate - 0 minutes
                0m
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 1 minute
                1m