Moodle
  1. Moodle
  2. MDL-31713

Conditional activities not accessible after manually overriding grades.

    Details

    • Testing Instructions:
      Hide

      1. Create two assignments, both of them with a max mark of 9 or some other distinctive number that isnt 100.
      2. Set the conditional settings in the second assignment so that it can only be accessed when a grade of 70% is achieved on the first assignment.
      3. Enrol a test student.
      4. Manually override the test students grade for quiz 1 to 100%.
      5) Log in as the student to see that you can access the restricted assignment.

      If it doesnt appear initially log out, shut your browser and do whatever is necessary to get a new session.

      Show
      1. Create two assignments, both of them with a max mark of 9 or some other distinctive number that isnt 100. 2. Set the conditional settings in the second assignment so that it can only be accessed when a grade of 70% is achieved on the first assignment. 3. Enrol a test student. 4. Manually override the test students grade for quiz 1 to 100%. 5) Log in as the student to see that you can access the restricted assignment. If it doesnt appear initially log out, shut your browser and do whatever is necessary to get a new session.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-31713_override_24
    • Pull Master Branch:
      MDL-31713_override
    • Rank:
      38300

      Description

      It appears that the rawgrademax value is set to 100 when overriding a grade, rather than the max marks achievable for that grade item, which means the score returned in lib/conditionlib.php by the function get_cached_grade_score is wrong. When viewing the grade report it was visible that the students all had achieved 100% for the assignment, but were still unable to access the next assignment which required a grade of 70%. After displaying the score returned by the get_cached_grade_score function, it was shown that the actual grade was returned, not the percentage.

      The logic used in this method to return the percentage is -

      ((finalgrade - rawgrademin) * 100) / rawgrademax - rawgrademin)

      If the finalgrade is 8, the rawgrademin 0 and the rawgrademax 8, then this would return 100, however the rawgrademax is set to 100 in the DB, so the score returned is 8 so the condition is not met!

        Activity

        Hide
        faruq added a comment -

        Just wanted to say, keep up the work, and if anyone could solve this it would really help me out.

        Show
        faruq added a comment - Just wanted to say, keep up the work, and if anyone could solve this it would really help me out.
        Hide
        Andrew Davis added a comment -

        I believe I have been able to reproduce this.

        Show
        Andrew Davis added a comment - I believe I have been able to reproduce this.
        Hide
        Andrew Davis added a comment -

        I'll work on this over the weekend. Hopefully I'll have a simple fix ready to go by Monday morning.

        Show
        Andrew Davis added a comment - I'll work on this over the weekend. Hopefully I'll have a simple fix ready to go by Monday morning.
        Hide
        Andrew Davis added a comment -

        I've added a unit test that reproduce the problem. https://github.com/andyjdavis/moodle/compare/master...MDL-31713_override

        Show
        Andrew Davis added a comment - I've added a unit test that reproduce the problem. https://github.com/andyjdavis/moodle/compare/master...MDL-31713_override
        Hide
        Andrew Davis added a comment -

        Ive added a second commit that ensures that rawgrademin and max are kept up to date. rawgrademax is now correct in the database. Unfortunately the activity with the grade condition still isn't available. Something else is amiss.

        Show
        Andrew Davis added a comment - Ive added a second commit that ensures that rawgrademin and max are kept up to date. rawgrademax is now correct in the database. Unfortunately the activity with the grade condition still isn't available. Something else is amiss.
        Hide
        Andrew Davis added a comment -

        Actually now it does appear to be working. Maybe something to do with the session caching in get_cached_grade_score(). Not sure. A little off putting but I think this is actually working as it should.

        Show
        Andrew Davis added a comment - Actually now it does appear to be working. Maybe something to do with the session caching in get_cached_grade_score(). Not sure. A little off putting but I think this is actually working as it should.
        Hide
        Andrew Davis added a comment -

        Ive slightly updated the testing instructions. Putting this up for peer review.

        Show
        Andrew Davis added a comment - Ive slightly updated the testing instructions. Putting this up for peer review.
        Hide
        Mark Nelson added a comment -

        Hi Andrew, are you planning on backporting this to 2.3? I also suggest creating a master diff as well. The fix looks good. It is much simpler than I believed it would be. Thanks.

        Show
        Mark Nelson added a comment - Hi Andrew, are you planning on backporting this to 2.3? I also suggest creating a master diff as well. The fix looks good. It is much simpler than I believed it would be. Thanks.
        Hide
        Andrew Davis added a comment -

        Hi. That is a master diff URL. Im not sure how but a number of my issues have had the master diff URL seem to magically shift to the 2.4 diff url field. I will also backport this after peer review.

        Show
        Andrew Davis added a comment - Hi. That is a master diff URL. Im not sure how but a number of my issues have had the master diff URL seem to magically shift to the 2.4 diff url field. I will also backport this after peer review.
        Hide
        Andrew Davis added a comment -

        Adding the various branches. If the peer review is complete and no code changes are required go ahead and submit this for integrationn.

        Show
        Andrew Davis added a comment - Adding the various branches. If the peer review is complete and no code changes are required go ahead and submit this for integrationn.
        Hide
        Mark Nelson added a comment -

        If you were to set $SESSION->gradescorecache[$gradeitemid] to the new score ((finalgrade - rawgrademin) * 100) / rawgrademax - rawgrademin) would this remove the need to clear the cache? Or you could simply unset $SESSION->gradescorecache[$gradeitemid] and then when get_cached_grade_score is called it will see it is no longer stored in the session and work it out again.

        Show
        Mark Nelson added a comment - If you were to set $SESSION->gradescorecache [$gradeitemid] to the new score ((finalgrade - rawgrademin) * 100) / rawgrademax - rawgrademin) would this remove the need to clear the cache? Or you could simply unset $SESSION->gradescorecache [$gradeitemid] and then when get_cached_grade_score is called it will see it is no longer stored in the session and work it out again.
        Hide
        Andrew Davis added a comment -

        hrmmm, you may be correct. I'm not keen to start meddling in our caching in a piecemeal fashion so I'm going to leave this as is for now.

        If the tester or anyone else is able to pin down whether or not there is a problem here raise an MDL.

        Show
        Andrew Davis added a comment - hrmmm, you may be correct. I'm not keen to start meddling in our caching in a piecemeal fashion so I'm going to leave this as is for now. If the tester or anyone else is able to pin down whether or not there is a problem here raise an MDL.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Damyon Wiese added a comment -

        Hi Andrew and Mark,

        OK - I have looked at this for a while and the solution you have here looks right - it is just missing one line to also set the rawscaleid from the current scaleid. update_raw_grade is very similar to update_final_grade which is how I spotted that (and looking at the fields that get set by the update function).

        Regarding the caching - it doesn't appear fixable until all that caching is moved to MUC. The values in the session cache are automatically cleared - but only if the edited grade is for the current user - which is pretty much useless IMO. The ideal way would be to have a shared cache for gradeitems that can be purged for all users.

        If you can update the patch this looks good for integration - thanks.

        Show
        Damyon Wiese added a comment - Hi Andrew and Mark, OK - I have looked at this for a while and the solution you have here looks right - it is just missing one line to also set the rawscaleid from the current scaleid. update_raw_grade is very similar to update_final_grade which is how I spotted that (and looking at the fields that get set by the update function). Regarding the caching - it doesn't appear fixable until all that caching is moved to MUC. The values in the session cache are automatically cleared - but only if the edited grade is for the current user - which is pretty much useless IMO. The ideal way would be to have a shared cache for gradeitems that can be purged for all users. If you can update the patch this looks good for integration - thanks.
        Hide
        Andrew Davis added a comment -

        I've fixed up all 3 versions to include the scale. I just squished it into the previous commit.

        Show
        Andrew Davis added a comment - I've fixed up all 3 versions to include the scale. I just squished it into the previous commit.
        Hide
        Damyon Wiese added a comment -

        Thanks Andrew,

        This looks good!

        Integrated to 23, 24 and master.

        Show
        Damyon Wiese added a comment - Thanks Andrew, This looks good! Integrated to 23, 24 and master.
        Hide
        Adrian Greeve added a comment -

        Tested on the 2.3, 2.4 and master integration branches.
        Conditional activities are accessible after manually overriding grades.
        Test passed.

        Show
        Adrian Greeve added a comment - Tested on the 2.3, 2.4 and master integration branches. Conditional activities are accessible after manually overriding grades. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Did you think this day was not going to arrive ever?

        Your patience has been rewarded, yay, sent upstream, thanks!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

          People

          • Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: