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

Allow for different displayformat when approving entries

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Glossary
    • Testing Instructions:
      Hide

      To test functionality

      • create a new glossary:
        • set the display format to a format which doesn't display the name of the submitting user (e.g. "Simple, dictionary style"); and
        • set the approval display format to default to the same as the display format.
      • as another user create a new glossary entry;
      • as an editing teacher view the approvals page:
        • Notice that the display format is the same as the display format setting
      • edit the glossary settings:
        • set the approval display format to another format (e.g. Full with Author).
      • view the approvals page to ensure that the new display format has taken effect
      • view the other browse pages (alphabet, category, date, and author) to ensure that it hasn't changed the normal displayformat.
      Show
      To test functionality create a new glossary: set the display format to a format which doesn't display the name of the submitting user (e.g. "Simple, dictionary style"); and set the approval display format to default to the same as the display format. as another user create a new glossary entry; as an editing teacher view the approvals page: Notice that the display format is the same as the display format setting edit the glossary settings: set the approval display format to another format (e.g. Full with Author). view the approvals page to ensure that the new display format has taken effect view the other browse pages (alphabet, category, date, and author) to ensure that it hasn't changed the normal displayformat.
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31014-master-4

      Description

      We've had a request from one of our users to display the name of the submitting user when approving new glossary entries.

      In my opinion it makes sense to make this an option as an alternative displayformat for the approvals page.

      Replication Instructions

      • create a new glossary;
      • set the display format to a format which doesn't display the name of the submitting user (e.g. "Simple, dictionary style");
      • as another user create a new glossary entry;
      • as an editing teacher view the approvals page.

      Before applying this patch, it is not possible to determine which user has submitted a glossary entry
      After applying this patch, an option is available to allow users to select which displayformat is selected

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment -

            I've set the default approvaldisplayformat to be fullwithauthor as I think that this really makes sense in most scenarios, but please feel free to correct me

            I've also set the upgrade script to set the default approvaldisplayformat for existing glossaries to be the existing displayformat.

            Show
            dobedobedoh Andrew Nicols added a comment - I've set the default approvaldisplayformat to be fullwithauthor as I think that this really makes sense in most scenarios, but please feel free to correct me I've also set the upgrade script to set the default approvaldisplayformat for existing glossaries to be the existing displayformat.
            Hide
            andyjdavis Andrew Davis added a comment -

            Would it be possible to have approvaldisplayformat default to something that means "just use whatever format I selected for displayformat"? That way, for people who want to use the same format for both viewing and approvals, they only have to choose their format once. Existing users will only expect to set the format once. If they set it once, don't notice the new option, then go to approve an entry and the glossary appears to magically switch to "fullwithauthor" they will be confused.

            Shift the replication instructions into the issue description. Make sure that the testing instructions include approving using the same format as viewing, approving using a different format than viewing and also, if possible, approving a glossary entry that predates the fix ie one that has been through the upgrade process. That last one can be tricky though. It requires someone to read the testing instructions before pulling down the latest code.

            Show
            andyjdavis Andrew Davis added a comment - Would it be possible to have approvaldisplayformat default to something that means "just use whatever format I selected for displayformat"? That way, for people who want to use the same format for both viewing and approvals, they only have to choose their format once. Existing users will only expect to set the format once. If they set it once, don't notice the new option, then go to approve an entry and the glossary appears to magically switch to "fullwithauthor" they will be confused. Shift the replication instructions into the issue description. Make sure that the testing instructions include approving using the same format as viewing, approving using a different format than viewing and also, if possible, approving a glossary entry that predates the fix ie one that has been through the upgrade process. That last one can be tricky though. It requires someone to read the testing instructions before pulling down the latest code.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Your wish is my command sir,

            I've updated the patch to allow the approval format to default to the same setting as the displayformat and I've made this default too.

            Show
            dobedobedoh Andrew Nicols added a comment - Your wish is my command sir, I've updated the patch to allow the approval format to default to the same setting as the displayformat and I've made this default too.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Andrew,

            Not sure whether you saw my last update on this - I've made the changes you requested and wondered if you could peer review again?

            I've also rebased on the latest master.

            Show
            dobedobedoh Andrew Nicols added a comment - Andrew, Not sure whether you saw my last update on this - I've made the changes you requested and wondered if you could peer review again? I've also rebased on the latest master.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            This rebases cleanly on the latest master.

            Show
            dobedobedoh Andrew Nicols added a comment - This rebases cleanly on the latest master.
            Hide
            salvetore Michael de Raadt added a comment -

            This will require the user docs to be updated after it is integrated.

            Show
            salvetore Michael de Raadt added a comment - This will require the user docs to be updated after it is integrated.
            Hide
            andyjdavis Andrew Davis added a comment -

            Sorry for the delay. That all looks fine. I'll go ahead and put this up for integration.

            Show
            andyjdavis Andrew Davis added a comment - Sorry for the delay. That all looks fine. I'll go ahead and put this up for integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) 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
            dougiamas Martin Dougiamas added a comment -

            +10 from me for 2.3. Good feature.

            Show
            dougiamas Martin Dougiamas added a comment - +10 from me for 2.3. Good feature.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Andrew, in it goes!

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Andrew, in it goes!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Side comment, I think the UPDATE after the ALTER is:

            • Unnecessary. If the column is not null and has one default, all DBs should be filling it automatically with the "default".
            • Incorrect: One simple set_field() should be enough if the update is necessary. Why using one execute() there?

            +1 to amend.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Side comment, I think the UPDATE after the ALTER is: Unnecessary. If the column is not null and has one default, all DBs should be filling it automatically with the "default". Incorrect: One simple set_field() should be enough if the update is necessary. Why using one execute() there? +1 to amend. Ciao
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Indeed you are correct Eloy. The original implementation didn't allow for a 'default' and I hadn't considered this in the refactor.

            I've removed the execute (and yes it should have been a set_field anyway).

            Tested behaviour on postgres.

            I've also rebased on the latest master

            Show
            dobedobedoh Andrew Nicols added a comment - Indeed you are correct Eloy. The original implementation didn't allow for a 'default' and I hadn't considered this in the refactor. I've removed the execute (and yes it should have been a set_field anyway). Tested behaviour on postgres. I've also rebased on the latest master
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Good catch thanks Eloy and thanks Andrew for producing an amended branch.
            The amended branch has been integrated now and this is up for testing once more with the revised upgrade routine.

            Show
            samhemelryk Sam Hemelryk added a comment - Good catch thanks Eloy and thanks Andrew for producing an amended branch. The amended branch has been integrated now and this is up for testing once more with the revised upgrade routine.
            Hide
            abgreeve Adrian Greeve added a comment -

            Tested with master. Everything works as expected. Test passed.

            Show
            abgreeve Adrian Greeve added a comment - Tested with master. Everything works as expected. Test passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Some changes to Moodle should be milestones in the project by themselves.

            This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao
            Hide
            rezeau Joseph Rézeau added a comment -

            This is a very useful new feature. Thanks!

            Show
            rezeau Joseph Rézeau added a comment - This is a very useful new feature. Thanks!
            Hide
            marycooch Mary Cooch added a comment -

            documented now, thanks

            Show
            marycooch Mary Cooch added a comment - documented now, thanks

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12