Bug 21048

Summary: suggest_status not behaving properly
Product: Koha Reporter: Lisette Scheer <lisettepalouse+koha>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, katrin.fischer, lisettepalouse+koha, m.de.rooy, martin.renvoize, nick, sandboxes
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Status Blank - members/purchase-suggestions.pl
No status when viewing suggestion
Visible when filtering suggestions
OPAC View
Bug 21048: Fix display of suggestion's status (SUGGEST_STATUS)
Bug 21048: Fix display of suggestion's status (SUGGEST_STATUS)
Multiple Status Unknown tabs
Bug 21048: Fix display of suggestion's status (SUGGEST_STATUS)
Bug 21048: (QA follow-up) Fix authorized value statuses in filter

Description Lisette Scheer 2018-07-06 16:40:34 UTC
Created attachment 76744 [details]
Status Blank - members/purchase-suggestions.pl

If you set up suggest_status in the authorized values and then select one of these AV as a status it does not behave properly. 

1. The tab in suggestions.pl is labeled 'Status unknown'
2. If you click on 'Status unknown' it will show the AV you selected in the status. 
3. If you click on the suggestion title to open the full information, the status is blank. 
4. If you edit the suggestion it now says 'No status' (the reason, if entered, is still filled.)
5. On the patron's purchase suggestion page on the staff side, the status is blank but the reason still shows up. 

Lisette
Comment 1 Lisette Scheer 2018-07-06 16:41:09 UTC
Created attachment 76745 [details]
No status when viewing suggestion
Comment 2 Lisette Scheer 2018-07-06 16:42:37 UTC
Created attachment 76746 [details]
Visible when filtering suggestions
Comment 3 Lisette Scheer 2018-07-06 16:44:46 UTC
Created attachment 76747 [details]
OPAC View

6. The OPAC side shows the authorized value instead of the OPAC description.
Comment 4 Jonathan Druart 2018-07-11 21:27:14 UTC
Created attachment 76872 [details] [review]
Bug 21048: Fix display of suggestion's status (SUGGEST_STATUS)

When a suggestion's status is set to a customized value (AV
SUGGEST_STATUS), the description is not always displayed correctly.

Test plan:
- Create different values for the AV SUGGEST_STATUS
- Create several suggestions
- Change the status using the SUGGEST_STATUS values
- Edit the suggestion and change again the status
- View the suggestions for a given patron
(members/purchase-suggestions.pl?borrowernumber=XX), the statuses must
displayed correctly
Comment 5 Jonathan Druart 2018-07-11 21:29:07 UTC
I only have recreated the first issue (and find another one when editing a suggestion). The tab displayed ok for me. Which exact version of Koha are you using?

On master I see the tabs which have status with suggestions. Emtpy tabs are not displayed.
Comment 6 Lisette Scheer 2018-07-12 17:54:40 UTC
We are on 17.11.07.000. 
On master on Sandbox It shows a bunch of tabs labeled "Status unknown (0)' and a tab for whichever ones have at least 1 purchase suggestion with that value.
Comment 7 Biblibre Sandboxes 2018-07-12 17:56:16 UTC
Patch tested with a sandbox, by Lisette Scheer <lisetteslatah@gmail.com>
Comment 8 Biblibre Sandboxes 2018-07-12 17:57:02 UTC
Created attachment 76900 [details] [review]
Bug 21048: Fix display of suggestion's status (SUGGEST_STATUS)

When a suggestion's status is set to a customized value (AV
SUGGEST_STATUS), the description is not always displayed correctly.

Test plan:
- Create different values for the AV SUGGEST_STATUS
- Create several suggestions
- Change the status using the SUGGEST_STATUS values
- Edit the suggestion and change again the status
- View the suggestions for a given patron
(members/purchase-suggestions.pl?borrowernumber=XX), the statuses must
displayed correctly

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Comment 9 Jonathan Druart 2018-07-12 18:18:24 UTC
(In reply to Lisette Scheer from comment #6)
> We are on 17.11.07.000. 
> On master on Sandbox It shows a bunch of tabs labeled "Status unknown (0)'
> and a tab for whichever ones have at least 1 purchase suggestion with that
> value.

How did you do that? :)
I have created a new suggestion on another sandbox (5) and there is only one tab!
Comment 10 Lisette Scheer 2018-07-12 19:19:10 UTC
Created attachment 76925 [details]
Multiple Status Unknown tabs

This was on BibLibre sandbox 06 both on master and with your patch applied.
I created 3 suggest_status AV, made 3 purchase suggestions and assigned one to 2 of the statuses and left one pending. Each time it added a new tab, but left the status unknown tabs visible. 

There were the same number of status unknown tabs when I had 1 AV as when I had 3.  

I tried on BibLibre sandbox 07 on master and before I even added any suggest_status AV I put in a purchase suggestion and it did this again. 

Perhaps it is an issue with the BibLibre sandboxes? I'd try on another but those are the only ones I've been able to get to work today. 
Lisette
Comment 11 Marcel de Rooy 2018-07-13 09:54:16 UTC
Line 680 still contains:
[% IF s.authorised_value == selected_status %]
Around line 308 you are replacing it by suggestion.STATUS. Please explain. If I am not mistaken, it comes from: my $suggestion_ref  = $input->Vars; Where is the field STATUS filled?

See the following warn when I haved added an additional status and I am saving a suggestion with that status.
No suggestions TEST3 letter transported by email at /usr/share/koha/devclone/C4/Letters.pm line 602.
Comment 12 Jonathan Druart 2018-07-13 11:27:37 UTC
(In reply to Marcel de Rooy from comment #11)
> Line 680 still contains:
> [% IF s.authorised_value == selected_status %]
> Around line 308 you are replacing it by suggestion.STATUS. Please explain.
> If I am not mistaken, it comes from: my $suggestion_ref  = $input->Vars;
> Where is the field STATUS filled?

361     suggestion   => $suggestion_ref,

> See the following warn when I haved added an additional status and I am
> saving a suggestion with that status.
> No suggestions TEST3 letter transported by email at
> /usr/share/koha/devclone/C4/Letters.pm line 602.

I guess this existed prior to this change. It looks like a configuration issue, or an expected behavior, ModSuggestion is sending an email to the author of the suggestion when the status is changed. The notice template used is the one with the same code as the status (that is pretty bad, it should at least be prefixed by something). I would not consider this as blocker for this bugfix/
Comment 13 Jonathan Druart 2018-07-13 11:46:52 UTC
(In reply to Lisette Scheer from comment #10)
> Created attachment 76925 [details]
> Multiple Status Unknown tabs

This is very weird. I do not have access to sandbox and cannot recreate locally so it's hard help.
Comment 14 Katrin Fischer 2018-07-15 17:43:14 UTC
Hi Lisette, 
can you share how you set up your SUGGEST_STATUS values?
Comment 15 Marcel de Rooy 2018-07-16 09:34:12 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Marcel de Rooy from comment #11)
> > Line 680 still contains:
> > [% IF s.authorised_value == selected_status %]
> > Around line 308 you are replacing it by suggestion.STATUS. Please explain.
> > If I am not mistaken, it comes from: my $suggestion_ref  = $input->Vars;
> > Where is the field STATUS filled?
> 
> 361     suggestion   => $suggestion_ref,
OK. But what about line 680? The selected_status there is not filled anywhere?

> > See the following warn when I haved added an additional status and I am
> > saving a suggestion with that status.
> > No suggestions TEST3 letter transported by email at
> > /usr/share/koha/devclone/C4/Letters.pm line 602.
> 
> I guess this existed prior to this change. It looks like a configuration
> issue, or an expected behavior, ModSuggestion is sending an email to the
> author of the suggestion when the status is changed. The notice template
> used is the one with the same code as the status (that is pretty bad, it
> should at least be prefixed by something). I would not consider this as
> blocker for this bugfix/
No, it is certainly not a blocker here.
Comment 16 Jonathan Druart 2018-07-16 12:39:27 UTC
(In reply to Marcel de Rooy from comment #15)
> (In reply to Jonathan Druart from comment #12)
> > (In reply to Marcel de Rooy from comment #11)
> > > Line 680 still contains:
> > > [% IF s.authorised_value == selected_status %]
> > > Around line 308 you are replacing it by suggestion.STATUS. Please explain.
> > > If I am not mistaken, it comes from: my $suggestion_ref  = $input->Vars;
> > > Where is the field STATUS filled?
> > 
> > 361     suggestion   => $suggestion_ref,
> OK. But what about line 680? The selected_status there is not filled
> anywhere?

It sounds like a bug but I cannot recreate from the UI, maybe this is never used?
Could you have a look?
Comment 17 Lisette Scheer 2018-07-18 21:35:09 UTC
(In reply to Katrin Fischer from comment #14)
> Hi Lisette, 
> can you share how you set up your SUGGEST_STATUS values?

The ones on the Valnet system were already set up when I got here. 
On the sandboxes I tested on here are my steps:
Go to Administration
Go to Authorized Values
Select 'SUGGEST_STATUS'
Click the "New Value for SUGGEST_STATUS" button. 
Type in an unique AV (doesn't match any other AV in SUGGEST_STATUS or other AVs)
Type in a description and OPAC description
Click save. 

Lisette
Comment 18 Marcel de Rooy 2018-07-27 08:14:27 UTC
(In reply to Jonathan Druart from comment #16)
> (In reply to Marcel de Rooy from comment #15)
> > (In reply to Jonathan Druart from comment #12)
> > > (In reply to Marcel de Rooy from comment #11)
> > > > Line 680 still contains:
> > > > [% IF s.authorised_value == selected_status %]
> > > > Around line 308 you are replacing it by suggestion.STATUS. Please explain.
> > > > If I am not mistaken, it comes from: my $suggestion_ref  = $input->Vars;
> > > > Where is the field STATUS filled?
> > > 
> > > 361     suggestion   => $suggestion_ref,
> > OK. But what about line 680? The selected_status there is not filled
> > anywhere?
> 
> It sounds like a bug but I cannot recreate from the UI, maybe this is never
> used?
> Could you have a look?

It is a very small bug. I will fill selected_status in the script instead of suggestion.STATUS since the template uses suggestion here as a loop variable.
Comment 19 Marcel de Rooy 2018-07-27 08:15:16 UTC
Created attachment 77298 [details] [review]
Bug 21048: Fix display of suggestion's status (SUGGEST_STATUS)

When a suggestion's status is set to a customized value (AV
SUGGEST_STATUS), the description is not always displayed correctly.

Test plan:
- Create different values for the AV SUGGEST_STATUS
- Create several suggestions
- Change the status using the SUGGEST_STATUS values
- Edit the suggestion and change again the status
- View the suggestions for a given patron
(members/purchase-suggestions.pl?borrowernumber=XX), the statuses must
displayed correctly

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2018-07-27 08:15:21 UTC
Created attachment 77299 [details] [review]
Bug 21048: (QA follow-up) Fix authorized value statuses in filter

If you select an authorized value status in the filter, you want to keep
it too just like normal statuses as CHECKED.
The variable selected_status was not filled. The minimal fix is adding that
template variable in the script. Note that suggestion.STATUS is out of scope
within a loop using suggestion as loop var.

Note: Adding a regular status like CHECKED as an authorized value with
same code but another description works, but is kind of confusing ;) Not
in the scope of this report though.

Test plan:
Select an authval status in the filter. Apply the filter and verify again
that it is still selected under Suggestion information.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Nick Clemens 2018-08-08 21:33:36 UTC
Awesome work all!

Pushed to master for 18.11
Comment 22 Martin Renvoize 2018-08-09 10:20:54 UTC
Pushed to 18.05.x for 18.05.03
Comment 23 Fridolin Somers 2018-09-21 10:16:31 UTC
Pushed to 17.11.x for 17.11.10