Hi Kyle,

I am not so happy with this change, trying to explain why:

1) Currently authorized values are one language only - we have no way to make them more than 'one language at a time'. So if we change something that is translated in the templates now back to database, that makes me unhappy. Especially if it's strings in the OPAC.

-                    [% IF ( suggestions_loo.ASKED ) %]Requested[% END %]
-                    [% IF ( suggestions_loo.CHECKED ) %]Checked by the library[% END %]
-                    [% IF ( suggestions_loo.ACCEPTED ) %]Accepted by the library[% END %]
-                    [% IF ( suggestions_loo.ORDERED ) %]Ordered by the library[% END %]
-                    [% IF ( suggestions_loo.REJECTED ) %]Suggestion declined [% END %]
                     [% IF ( suggestions_loo.AVAILABLE ) %]Available in the library[% END %]
+                    [% KohaAuthorisedValues.GetByCode( 'SUGGEST_STATUS', suggestions_loo.STATUS, 1 ) %

2) You only added the new authorized value to the English sample files. This makes me unhappy too, because it means when you use any non-English installer, the suggestions are going to be broken.

3) The status values are hardcoded in other places afaikt. Some status changes also send out emails to the patrons, like rejecting or accepting their suggestion. Ordering and receiving should, although I am not sure that is working right now (I filed bug reports sometime somewhere). If you leave it to the library to choose the status codes, the chance they will accidentally break functionality being not aware of this is quite high.

Would it be possible to only allow to add additional status or or allow to overwrite hardcoded status by the library?

I was thinking about that. I'll write a new patch that just adds new statuses, and doesn't affect the old ones.

Kyle

(In reply to comment #3)
> Hi Kyle,
> 
> I am not so happy with this change, trying to explain why:
> 
> 1) Currently authorized values are one language only - we have no way to
> make them more than 'one language at a time'. So if we change something that
> is translated in the templates now back to database, that makes me unhappy.
> Especially if it's strings in the OPAC.
> 
> -                    [% IF ( suggestions_loo.ASKED ) %]Requested[% END %]
> -                    [% IF ( suggestions_loo.CHECKED ) %]Checked by the
> library[% END %]
> -                    [% IF ( suggestions_loo.ACCEPTED ) %]Accepted by the
> library[% END %]
> -                    [% IF ( suggestions_loo.ORDERED ) %]Ordered by the
> library[% END %]
> -                    [% IF ( suggestions_loo.REJECTED ) %]Suggestion
> declined [% END %]
>                      [% IF ( suggestions_loo.AVAILABLE ) %]Available in the
> library[% END %]
> +                    [% KohaAuthorisedValues.GetByCode( 'SUGGEST_STATUS',
> suggestions_loo.STATUS, 1 ) %
> 
> 2) You only added the new authorized value to the English sample files. This
> makes me unhappy too, because it means when you use any non-English
> installer, the suggestions are going to be broken.
> 
> 3) The status values are hardcoded in other places afaikt. Some status
> changes also send out emails to the patrons, like rejecting or accepting
> their suggestion. Ordering and receiving should, although I am not sure that
> is working right now (I filed bug reports sometime somewhere). If you leave
> it to the library to choose the status codes, the chance they will
> accidentally break functionality being not aware of this is quite high.
> 
> Would it be possible to only allow to add additional status or or allow to
> overwrite hardcoded status by the library?

Test plan:
1) Apply patch
2) Create one or more new SUGGESTION_STATUS authorised values
3) Create some purchase suggestions and try using the new authorised value based statuses.

Hi Kyle,

I am not able to create SUGGEST_STATUS authorised values, as the html form restrict the size of category name to 10 characters.
By adding some authorised values using sql i am able to test and it seems to work.

Some remarks:
- If I use an authorised value STATUS1 for a suggestion, then change the authorised value code to STATUS1OLD or delete the authorised value, "Status" cell in suggestions table become empty but the suggestion is still under the tab "Status1". I think all suggestions without a valid status should go into a "Unknown" tab or something like that.
- Why do you use the code for the tab title instead of the description ?
- Since user has to create the category, I think a little paragraph in the online help about the new authorised value category would be appreciated.


I'm forced to set status to Failed QA, because of the restriction on authorised value category name length. Sorry.

> I am not able to create SUGGEST_STATUS authorised values, as the html form
> restrict the size of category name to 10 characters.

Fixed!

> - If I use an authorised value STATUS1 for a suggestion, then change the
> authorised value code to STATUS1OLD or delete the authorised value, "Status"
> cell in suggestions table become empty but the suggestion is still under the
> tab "Status1". I think all suggestions without a valid status should go into
> a "Unknown" tab or something like that.

Fixed!

> - Why do you use the code for the tab title instead of the description ?

Fixed!

> - Since user has to create the category, I think a little paragraph in the
> online help about the new authorised value category would be appreciated.

Fixed!

Patch does not apply. Could you rebase it please?
M. Saby

Hello Kyle
It looks good, but I have 2 little questions before signing off.
My test plan :
1. Applying the patch
2. Checking existing suggestions, in staff view and opac :  ok
3. Creating a new suggestion in staff and in opac :  ok
4. Moving a suggestion into one of the old categories : ok
5. Adding a new authorized value category named SUGGEST_VALUES and fill it with 3 values :
value    Description    Description (OPAC) =)> 
tata    tatapro         tataopac
titi    titipro         titiopac
toto    totopro         totoopac
For "tata", I defined a branch restriction on branche B.
6. Connecting in staff interface as a librarian of branch A : I could only use codes titipro and totopro. On OPAC, these codes were displayed as totoopac and titiopac
7. Connecting as librarian of branch B : I could use the 3 codes
8. Checking tabs visible : new tabs for tata, titi, toto, but also a new "Unknown" tab with 0 record.

So, my 2 questions : is it a wanted behavior to give the ability to librarians of some branch to use only some code?
is it wanted to see a "Unknown" tab with 0 records?

If it is ok for you, I can sign it off.

Mathieu Saby

It is ok for Kyle, so I sign off. Maybe point 8 could be fixed later.
M. Saby

CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/help/suggestion/suggestion.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt

Kyle, could you rebase this patch please?

Created attachment 19112 [details] [review]
Bug 9307 - Allow libraries to add/edit status for suggestion management.

This patch allows for the addition of new suggestion status by adding
authorised values of the category SUGGEST_STATUS

I sign it off. Details of my test plan :
1. Applying the patch
2. Checking existing suggestions, in staff view and opac :  ok
3. Creating a new suggestion in staff and in opac :  ok
4. Moving a suggestion into one of the old categories : ok
5. Adding a new authorized value category named SUGGEST_STATUS and fill it with 3 values :
value    Description    Description (OPAC) =)>
tata    tatapro         tataopac
titi    titipro         titiopac
toto    totopro         totoopac
For "tata", I defined a branch restriction on branche B.
6. Connecting in staff interface as a librarian of branch A : I could only use codes titipro and totopro. On OPAC, these codes were displayed as totoopac and titiopac
7. Connecting as librarian of branch B : I could use the 3 codes
8. Checking tabs visible : new tabs for tata, titi, toto, but also a new "Unknown" tab with 0 record.

Maybe point 8 could be fixed in a followup.
I sign off the patch.

Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Created attachment 19113 [details] [review]
Bug 9307: QA Followup

- Remove an old DHTMLcalendar_dateformat variable
- Add SUGGEST_STATUS to the "Koha system categories"

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

QA comment:

(suggestion/suggestion.pl is really ugly!)

All works great. There is an issue, but not blocker, I think. If an authorized value is limited to a library, the suggestion does not appear (into the suggestions table) but the tab appears.
It is caused by a call to the C4::Search::GetDistinctValues. This routine has to be removed.

Except that, I provided a followup for 2 small issues:
1/ DHTMLcalendar_dateformat is passed to the tt, useless.
2/ I think SUGGEST_STATUS should be added to the Koha system categories.

+ I edited the commit message, there was 1 occurrence for SUGGEST_VALUES (renamed by SUGGEST_STATUS).

Marked as Passed QA.

(In reply to comment #0)
> We would like to be able to edit or add our own custom tabs to the Suggestion
> Management page.

To what end?  Do you have any specific examples of the sort of custom suggestion statuses your users would like to add?  Are there any particular workflows they want to support?

Yes, the library in question has a workflow where new requests are split up and separated by different departments, and would like to create status sets to keep them separated. I can give you more details on it once I locate them.

(In reply to comment #20)
> (In reply to comment #0)
> > We would like to be able to edit or add our own custom tabs to the Suggestion
> > Management page.
> 
> To what end?  Do you have any specific examples of the sort of custom
> suggestion statuses your users would like to add?  Are there any particular
> workflows they want to support?

Pushed to master, along with a followup tweaking the help text.  Thanks, Kyle!

Hi Kyle, bug 10519 seems to be a regression from this patch, although there is admittedly more wrong with the suggestion filters than just what I captured on the screenshot. I checked out the commit before this was pushed and the display for the ordered tab was correct.

I think it might be this line:
+    unless ( grep { /$criteriumvalue/ } qw(ASKED ACCEPTED REJECTED CHECKED) ) {
It's missing ORDERED and AVAILABLE?