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

          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