Moodle
  1. Moodle
  2. MDL-31362

Is GRADE_UPDATE_ITEM_DELETED useful or can it be deleted?

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.3
    • Component/s: Gradebook
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Check that you have debugging set to developer.

      Go into a course that has at least one student.

      Add an LTI activity that uses grades. (please see MDLQA-1419 - contains instructions to add a particular LTI 'dummy')

      Grade a student.

      Delete the activity.

      Check you get no errors.

      Show
      Check that you have debugging set to developer. Go into a course that has at least one student. Add an LTI activity that uses grades. (please see MDLQA-1419 - contains instructions to add a particular LTI 'dummy') Grade a student. Delete the activity. Check you get no errors.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-31362_unused_constant
    • Rank:
      37879

      Description

      GRADE_UPDATE_ITEM_DELETED isnt really used in code. Its defined in /lib/grade/constants.php but never used.

      Is it meant to be returned when a grade item is deleted? Or is it meant to be returned when you try to update a grade item that has been deleted? Or is it pointless so it can be safely removed?

        Activity

        Hide
        Andrew Davis added a comment - - edited

        I'm guessing but I think there were plans back in around 2007 to have grade_update() return GRADE_UPDATE_ITEM_DELETED when grade_update() was used to delete a grade item. Instead GRADE_UPDATE_OK is being returned.

        I could change grade_update() to return GRADE_UPDATE_ITEM_DELETED but this is part of the grade API so changing its return value is non-trivial. The current behaviour is not really wrong so I think we should just stick with removing GRADE_UPDATE_ITEM_DELETED to avoid future confusion.

        Show
        Andrew Davis added a comment - - edited I'm guessing but I think there were plans back in around 2007 to have grade_update() return GRADE_UPDATE_ITEM_DELETED when grade_update() was used to delete a grade item. Instead GRADE_UPDATE_OK is being returned. I could change grade_update() to return GRADE_UPDATE_ITEM_DELETED but this is part of the grade API so changing its return value is non-trivial. The current behaviour is not really wrong so I think we should just stick with removing GRADE_UPDATE_ITEM_DELETED to avoid future confusion.
        Hide
        Andrew Davis added a comment -

        Adding a branch.

        I suspect this should only go into master. There's no real need to break anyone's contrib code in stable if they have done the same as the lti module and attempted to future proof their code.

        Show
        Andrew Davis added a comment - Adding a branch. I suspect this should only go into master. There's no real need to break anyone's contrib code in stable if they have done the same as the lti module and attempted to future proof their code.
        Hide
        Ankit Agarwal added a comment -

        Hi Andrew,
        Looks safe to me as well to delete it.
        +1 to integrate to master only
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Andrew, Looks safe to me as well to delete it. +1 to integrate to master only Thanks
        Hide
        Andrew Davis added a comment -

        Submitting for master only integration

        Show
        Andrew Davis added a comment - Submitting for master only integration
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Some hours ago...

        the main moodle.git repository has 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
        Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has 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
        Sam Hemelryk added a comment -

        Thanks Andrew this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Andrew this has been integrated now
        Hide
        Tim Barker added a comment -

        Will finish this off tomorrow

        Show
        Tim Barker added a comment - Will finish this off tomorrow
        Hide
        Tim Barker added a comment -

        I can't test this until I have a URL for an LTI (that accepts grades), which I don't currently have, so for now testing is on hold.

        Show
        Tim Barker added a comment - I can't test this until I have a URL for an LTI (that accepts grades), which I don't currently have, so for now testing is on hold.
        Hide
        Aparup Banerjee added a comment -

        ok got me some LTI to eat "http://tracker.moodle.org/browse/MDLQA-1419 and http://tracker.moodle.org/browse/MDLQA-1418" - thanks MD

        Show
        Aparup Banerjee added a comment - ok got me some LTI to eat "http://tracker.moodle.org/browse/MDLQA-1419 and http://tracker.moodle.org/browse/MDLQA-1418 " - thanks MD
        Hide
        Aparup Banerjee added a comment -

        seems to work fine for me..
        tested on qa.moodle.net (integration.git) with http://www.imsglobal.org/developers/LTI/test/v1p1/tool.php

        sends and receives grades fine.
        deletes fine - no errors.

        Show
        Aparup Banerjee added a comment - seems to work fine for me.. tested on qa.moodle.net (integration.git) with http://www.imsglobal.org/developers/LTI/test/v1p1/tool.php sends and receives grades fine. deletes fine - no errors.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        FCT (fixed, closing, thanks). Ciao

        "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
        ~ Benjamin Disraeli

        Show
        Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

          People

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

            Dates

            • Created:
              Updated:
              Resolved: