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.

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

Andrew Nicols
added a comment - 04/Jan/12 7:13 PM 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.

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.

Andrew Davis
added a comment - 11/Jan/12 10:51 AM 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.

Andrew Nicols
added a comment - 11/Jan/12 7:58 PM 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.

Andrew Nicols
added a comment - 19/Jan/12 5:40 PM 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.

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.

Eloy Lafuente (stronk7)
added a comment - 17/Feb/12 10:10 AM 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

Eloy Lafuente (stronk7)
added a comment - 20/Feb/12 8:13 PM - 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

Andrew Nicols
added a comment - 20/Feb/12 9:10 PM 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

Sam Hemelryk
added a comment - 21/Feb/12 5:34 AM 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.

Eloy Lafuente (stronk7)
added a comment - 23/Feb/12 7:34 PM 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