Moodle
  1. Moodle
  2. MDL-31014

Allow for different displayformat when approving entries

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      37419

      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

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        Andrew Nicols added a comment -

        This rebases cleanly on the latest master.

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

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

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

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

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

        +10 from me for 2.3. Good feature.

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

        Thanks Andrew, in it goes!

        Show
        Sam Hemelryk added a comment - Thanks Andrew, in it goes!
        Hide
        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
        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
        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
        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
        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
        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
        Adrian Greeve added a comment -

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

        Show
        Adrian Greeve added a comment - Tested with master. Everything works as expected. Test passed.
        Hide
        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
        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
        Joseph Rézeau added a comment -

        This is a very useful new feature. Thanks!

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

        documented now, thanks

        Show
        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: