Bug 9307 - Allow libraries to add/edit status for suggestion management.
Summary: Allow libraries to add/edit status for suggestion management.
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 10519
  Show dependency treegraph
 
Reported: 2012-12-19 16:16 UTC by Kyle M Hall
Modified: 2014-12-07 20:02 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9307 [1] - Allow libraries to add/edit status for suggestion management. - Perltidy suggestion/suggestion.pl (22.44 KB, patch)
2012-12-19 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9307 [2] - Allow libraries to add/edit status for suggestion management. (35.54 KB, patch)
2012-12-19 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9307 - Allow libraries to add/edit status for suggestion management. (26.18 KB, patch)
2012-12-20 14:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9307 - Allow libraries to add/edit status for suggestion management. (28.60 KB, patch)
2013-01-17 18:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9307 - Allow libraries to add/edit status for suggestion management. (28.56 KB, patch)
2013-03-21 14:34 UTC, Brendan Gallagher
Details | Diff | Splinter Review
[PATCH][SIGNED-OFF] Bug 9307 - Allow libraries to add/edit status for suggestion management. (29.55 KB, patch)
2013-03-22 13:38 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 9307 - Allow libraries to add/edit status for suggestion management. (29.31 KB, patch)
2013-06-17 17:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9307 - Allow libraries to add/edit status for suggestion management. (29.38 KB, patch)
2013-06-18 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9307: QA Followup (1.54 KB, patch)
2013-06-18 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-12-19 16:16:27 UTC
We would like to be able to edit or add our own custom tabs to the Suggestion
Management page.
Comment 1 Kyle M Hall 2012-12-19 16:33:12 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-12-19 16:33:22 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2012-12-19 21:38:57 UTC
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?
Comment 4 Kyle M Hall 2012-12-20 13:38:00 UTC
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?
Comment 5 Kyle M Hall 2012-12-20 14:35:51 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-12-20 14:37:41 UTC
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.
Comment 7 Julian Maurice 2013-01-08 15:05:51 UTC
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.
Comment 8 Kyle M Hall 2013-01-17 18:02:29 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2013-01-17 18:03:38 UTC
> 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!
Comment 10 Mathieu Saby 2013-03-17 16:12:31 UTC
Patch does not apply. Could you rebase it please?
M. Saby
Comment 11 Brendan Gallagher 2013-03-21 14:34:19 UTC Comment hidden (obsolete)
Comment 12 Mathieu Saby 2013-03-21 20:14:03 UTC
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
Comment 13 Mathieu Saby 2013-03-22 13:35:41 UTC
It is ok for Kyle, so I sign off. Maybe point 8 could be fixed later.
M. Saby
Comment 14 Mathieu Saby 2013-03-22 13:38:54 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-06-14 09:19:36 UTC
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?
Comment 16 Kyle M Hall 2013-06-17 17:35:28 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-06-18 10:41:24 UTC
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>
Comment 18 Jonathan Druart 2013-06-18 10:41:30 UTC
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>
Comment 19 Jonathan Druart 2013-06-18 10:42:40 UTC
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.
Comment 20 Galen Charlton 2013-06-18 14:09:35 UTC
(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?
Comment 21 Kyle M Hall 2013-06-18 14:30:23 UTC
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?
Comment 22 Galen Charlton 2013-06-19 15:19:06 UTC
Pushed to master, along with a followup tweaking the help text.  Thanks, Kyle!
Comment 23 Katrin Fischer 2013-07-01 21:06:53 UTC
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?