Moodle
  1. Moodle
  2. MDL-29069

Allow database and glossary entries to be unapproved

    Details

    • Testing Instructions:
      Hide

      Prerequisites:

      • A course, C1.
      • Two users enrolled in this course, U1 (as student) and U2 (as teacher).
      • A glossary activity in C1, with "Entries -> Approved by default" set to "No", and without any entries present.

      Test script (database activity):

      • Log in as U2 and go to course C1.
      • Create a new database activity, setting "Entries -> Approval required" to "Yes"
      • Go to tab "Fields" and add at least one field (of any type).
      • Click "Templates".
      • Verify: the "List template" and the "Single template" both contain the text "##approve## ##disapprove## ##export##".
      • Log in as U1.
      • Go to the database activity (as created above).
      • Add an entry to the database.
      • Log in as U2.
      • Go to the database activity.
      • Verify: One entry is shown in the list of entries, with "Approve" button present (grey tickmark icon).
      • Click the "Approve" button.
      • Verify: Green message is displayed ("Entry approved"). Entry is now shown with "Disapprove" button present (grey circle with vertical bar).
      • Click the "Disapprove" button.
      • Verify: Green message is displayed ("Entry disapproved"). Entry is again shown with "Approve" button present (grey tickmark icon).

      Test script (glossary activity):

      • Log in as U1.
      • Go to the glossary activity.
      • Add an entry to the glossary.
      • Log in as U2.
      • Go to the glossary, and click "Waiting approval".
      • Verify: One entry is shown in the list of entries to be approved, with "Approve" button present (grey tickmark icon on the top-right).
      • Click the "Approve" button.
      • Verify: List is now empty.
      • Go back to the main screen of the glossary.
      • Verify: List contains one entry, with "disapprove" button shown on the bottom-right (grey circle with vertical bar).
      • Click the "Disapprove" button.
      • Verify: List is now empty. At top-right of screen, there is a link "Waiting approval (1)".
      • Click "Waiting approval".
      • Verify: One entry is shown in the list of entries to be approved.
      Show
      Prerequisites: A course, C1. Two users enrolled in this course, U1 (as student) and U2 (as teacher). A glossary activity in C1, with "Entries -> Approved by default" set to "No", and without any entries present. Test script (database activity): Log in as U2 and go to course C1. Create a new database activity, setting "Entries -> Approval required" to "Yes" Go to tab "Fields" and add at least one field (of any type). Click "Templates". Verify: the "List template" and the "Single template" both contain the text "##approve## ##disapprove## ##export##". Log in as U1. Go to the database activity (as created above). Add an entry to the database. Log in as U2. Go to the database activity. Verify: One entry is shown in the list of entries, with "Approve" button present (grey tickmark icon). Click the "Approve" button. Verify: Green message is displayed ("Entry approved"). Entry is now shown with "Disapprove" button present (grey circle with vertical bar). Click the "Disapprove" button. Verify: Green message is displayed ("Entry disapproved"). Entry is again shown with "Approve" button present (grey tickmark icon). Test script (glossary activity): Log in as U1. Go to the glossary activity. Add an entry to the glossary. Log in as U2. Go to the glossary, and click "Waiting approval". Verify: One entry is shown in the list of entries to be approved, with "Approve" button present (grey tickmark icon on the top-right). Click the "Approve" button. Verify: List is now empty. Go back to the main screen of the glossary. Verify: List contains one entry, with "disapprove" button shown on the bottom-right (grey circle with vertical bar). Click the "Disapprove" button. Verify: List is now empty. At top-right of screen, there is a link "Waiting approval (1)". Click "Waiting approval". Verify: One entry is shown in the list of entries to be approved.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:

      Description

      It would be great to have a way of unapproving database and glossary entries which have been approved by accident i.e. setting to require approval again. Currently it can only be done through phpmyadmin http://docs.moodle.org/20/en/Database_activity_module_FAQ#How_can_I_hide_a_database_entry_which_was_previously_approved.3F

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Henning Bostelmann added a comment -

            I'm providing a patch for master. This adds the requested "disapprove" functionality to both the database and the glossary activity. Approved entries now have a small "disapprove" button, which sends them back to the "not approved" state.

            Note that this adds some language strings, but re-uses existing icons.

            For the database activity, note that there is now another "special key" ("##disapprove##") to be used in templates, and this key is part of the new standard template, however for existing activities this key will not be inserted automatically.

            The current patch is for master only. However, I can provide a backport to 2.5 or earlier (if it can be integrated).

            Show
            Henning Bostelmann added a comment - I'm providing a patch for master. This adds the requested "disapprove" functionality to both the database and the glossary activity. Approved entries now have a small "disapprove" button, which sends them back to the "not approved" state. Note that this adds some language strings, but re-uses existing icons. For the database activity, note that there is now another "special key" ("##disapprove##") to be used in templates, and this key is part of the new standard template, however for existing activities this key will not be inserted automatically. The current patch is for master only. However, I can provide a backport to 2.5 or earlier (if it can be integrated).
            Hide
            Henning Bostelmann added a comment -

            Requesting peer review for this patch.

            Show
            Henning Bostelmann added a comment - Requesting peer review for this patch.
            Hide
            Helen Foster added a comment -

            Henning, thanks for your patch. Hopefully it can be peer reviewed soon.

            Show
            Helen Foster added a comment - Henning, thanks for your patch. Hopefully it can be peer reviewed soon.
            Hide
            Adrian Greeve added a comment -

            [Y] Syntax
            [N] Whitespace
            [Y] Output
            [Y] Language
            [Y] Databases
            [Y] Testing
            [Y] Security
            [-] Documentation
            [Y] Git
            [Y] Sanity check

            Hello Henning,

            Everything here looks quite good. I only have one small comment.

            In the new sections that you have created you have tabbed the indents into the code.

            • mod/data/lib.php from line 1282 - 1288
            • mod/glossary/lib.php from line 1242 - 1243

            Once you have changed these to spaces, you will be ready for integration.

            Thanks.

            Show
            Adrian Greeve added a comment - [Y] Syntax [N] Whitespace [Y] Output [Y] Language [Y] Databases [Y] Testing [Y] Security [-] Documentation [Y] Git [Y] Sanity check Hello Henning, Everything here looks quite good. I only have one small comment. In the new sections that you have created you have tabbed the indents into the code. mod/data/lib.php from line 1282 - 1288 mod/glossary/lib.php from line 1242 - 1243 Once you have changed these to spaces, you will be ready for integration. Thanks.
            Hide
            Henning Bostelmann added a comment -

            Thanks for reviewing this, Adrian.

            I have now fixed the whitespace issues, rebased the branch, and provided a 2.5 backport (not sure whether this minor feature change can be integrated into the stable branch, but it's actually only a cherry-pick).

            Show
            Henning Bostelmann added a comment - Thanks for reviewing this, Adrian. I have now fixed the whitespace issues, rebased the branch, and provided a 2.5 backport (not sure whether this minor feature change can be integrated into the stable branch, but it's actually only a cherry-pick).
            Hide
            Adrian Greeve added a comment -

            Great, thanks for that.

            Submitting for integration.

            Show
            Adrian Greeve added a comment - Great, thanks for that. Submitting for integration.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi Henning, just found a bunch of points that would be great to fix before allowing this to land:

            1) Some warnings from the code-checker: http://ci.stronk7.com/job/Precheck%20remote%20branch/541/artifact/work/smurf.html

            2) The new log action (disapprove entry) has to be added to:

            • the corresponding mod/glossary/db/log.php file
            • restore. See define_restore_log_rules() for those modules.

            Apart from that, it looks perfect. Final detail, I don't think this can land to stables now (new functionality). So this issue will add the new code only for master.

            Later you can create a backport request and it will be evaluated.

            Well done, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi Henning, just found a bunch of points that would be great to fix before allowing this to land: 1) Some warnings from the code-checker: http://ci.stronk7.com/job/Precheck%20remote%20branch/541/artifact/work/smurf.html 2) The new log action (disapprove entry) has to be added to: the corresponding mod/glossary/db/log.php file restore. See define_restore_log_rules() for those modules. Apart from that, it looks perfect. Final detail, I don't think this can land to stables now (new functionality). So this issue will add the new code only for master. Later you can create a backport request and it will be evaluated. Well done, ciao
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Henning Bostelmann added a comment -

            I have now added the log actions; thanks for pointing this out.

            I have also fixed the coding style problems as requested - however, now the changes are inconsistent in style with the surrounding code.

            Show
            Henning Bostelmann added a comment - I have now added the log actions; thanks for pointing this out. I have also fixed the coding style problems as requested - however, now the changes are inconsistent in style with the surrounding code.
            Hide
            Henning Bostelmann added a comment -

            Requesting peer review again (seems I can't directly put this up for integration review).

            Show
            Henning Bostelmann added a comment - Requesting peer review again (seems I can't directly put this up for integration review).
            Hide
            Adrian Greeve added a comment -

            Submitting back to integration.

            Show
            Adrian Greeve added a comment - Submitting back to integration.
            Hide
            Dan Poltawski added a comment -

            Integrated to master - thanks Henning!

            Show
            Dan Poltawski added a comment - Integrated to master - thanks Henning!
            Hide
            Jason Fowler added a comment -

            Great work Henning, passes the tests

            Show
            Jason Fowler added a comment - Great work Henning, passes the tests
            Hide
            Damyon Wiese added a comment -

            This issue along with 77 of it's friends has been sent upstream and released to the world.

            Thankyou for your contribution.

            Show
            Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.
            Hide
            Mary Cooch added a comment -

            FYI QA test on a glossary entry is here MDLQA-6670 (for 2.6; also added to the master) I'm thinking the button should really say 'unapprove' rather than 'disapprove' because they have different meanings.

            Show
            Mary Cooch added a comment - FYI QA test on a glossary entry is here MDLQA-6670 (for 2.6; also added to the master) I'm thinking the button should really say 'unapprove' rather than 'disapprove' because they have different meanings.
            Hide
            Mary Cooch added a comment -

            Removing docs_required as this is documented now in http://docs.moodle.org/26/en/Using_Database and in http://docs.moodle.org/26/en/Using_Glossary

            Show
            Mary Cooch added a comment - Removing docs_required as this is documented now in http://docs.moodle.org/26/en/Using_Database and in http://docs.moodle.org/26/en/Using_Glossary
            Hide
            Helen Foster added a comment -

            I agree with Mary that we should use 'Unapprove' rather than 'Disapprove' so have changed it in en_fix for Moodle 2.6.1 and also the master copy of the QA test.

            Show
            Helen Foster added a comment - I agree with Mary that we should use 'Unapprove' rather than 'Disapprove' so have changed it in en_fix for Moodle 2.6.1 and also the master copy of the QA test.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Just arguing with Helen that "unapprove", that has landed to a lot of places... is questionable (as a verb, it's correct as "unapproved" adjetive). Perhaps using something like "undo approval" is 100% correct, not ambiguous and means exactly what is expected. Any change will be performed @ MDL-43590, in any case.

            Show
            Eloy Lafuente (stronk7) added a comment - Just arguing with Helen that "unapprove", that has landed to a lot of places... is questionable (as a verb, it's correct as "unapproved" adjetive). Perhaps using something like "undo approval" is 100% correct, not ambiguous and means exactly what is expected. Any change will be performed @ MDL-43590 , in any case.

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: