Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken
Summary: Suggestions: 'Organize by' and correct display of tab descriptions broken
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P3 major (vote)
Assignee: Jacek Ablewicz
QA Contact: Marcel de Rooy
URL:
Keywords:
: 11609 11667 12154 12257 12433 (view as bug list)
Depends on: 9307
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-30 17:12 UTC by Katrin Fischer
Modified: 2015-12-03 22:02 UTC (History)
13 users (show)

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


Attachments
screenshot (52.50 KB, image/png)
2013-06-30 17:12 UTC, Katrin Fischer
Details
Bug 10519: Fix display of tab for ordered suggestions (1.27 KB, patch)
2013-07-01 21:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10519: Fix display of tab for ordered suggestions (1.33 KB, patch)
2013-07-02 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken - Change "Unknown" to authorised value (1.10 KB, patch)
2013-07-02 13:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken - Change "Unknown" to authorised value (1.26 KB, patch)
2013-07-03 06:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10519: Suggestions: 'Organize by' and correct display of tab descriptions broken (6.13 KB, patch)
2013-07-03 06:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10519: Suggestions: 'Organize by' and correct display of tab descriptions broken (6.19 KB, patch)
2013-07-03 11:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken (2.54 KB, patch)
2014-06-11 07:24 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken (1.13 KB, patch)
2014-06-13 19:42 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken (2.60 KB, patch)
2014-06-13 20:08 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken (1.20 KB, patch)
2014-06-13 20:08 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken (2.69 KB, patch)
2014-07-10 09:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken (1.29 KB, patch)
2014-07-10 09:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10519: (followup) unit tests leave problematic cruft (3.09 KB, patch)
2014-07-14 19:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-06-30 17:12:42 UTC
Created attachment 19300 [details]
screenshot

When organising tabs by status the 'ordered' suggestion shows up in a tab labelled 'Unknown'. I think there might be something missing in the templates here.
Comment 1 Katrin Fischer 2013-06-30 17:15:04 UTC
Actually, there is a lot of weirdness in the tab display going on there for me, there might be more wrong about it.
Comment 2 Katrin Fischer 2013-07-01 21:21:30 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-07-01 21:43:57 UTC
While my patch fixes the display of the status tabs, I just realized the problem is a lot bigger.

We can not only organize by status, but also by library, item type, suggested byand accepted by. I think this needs more work: Maybe an additional check on the selected 'organize by' filter?
Comment 4 Katrin Fischer 2013-07-01 22:02:28 UTC
Finding more problems:
return GetAuthorisedValueByCode('SUGGEST_STATUS', $criteriumvalue ) || "Unknown";
As the template doesn't check for this value, it's untranslatable.
Comment 5 Katrin Fischer 2013-07-02 07:29:33 UTC
Changing the bug title to reflect that this has grown into a bigger problem, I am working on a patch.
Comment 6 Kyle M Hall 2013-07-02 13:29:02 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2013-07-02 13:33:48 UTC
> Finding more problems:
> return GetAuthorisedValueByCode('SUGGEST_STATUS', $criteriumvalue ) || "Unknown";
> As the template doesn't check for this value, it's untranslatable.

This isn't necessarily as bad as it may seem. The string "Unknown" will only display during an error condition. The one and only error condition I can think of where this would display would be if a suggestion was set to an authorised value, and the authorised value was then deleted.

However, it may be better to display the authorised value instead of "Unkown". I will submit a followup for that.
Comment 8 Kyle M Hall 2013-07-02 13:34:45 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-07-03 06:44:27 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2013-07-03 06:44:48 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2013-07-03 06:46:27 UTC
The last patch in combination with bug 4907 seems to make things work better, but please test thoroughly.
Comment 12 Kyle M Hall 2013-07-03 11:15:17 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2013-07-03 20:42:26 UTC
Hi Kyle, I still feel like something is not quite right with the way we handle the library pull down on the form in combination with different search options. Did it work correctly for you?
Comment 14 Marcel de Rooy 2013-08-19 10:45:54 UTC
Katrin,
If I add suggestions from two libraries (and two users), and I leave them in pending state, and I organize by Library, there is only one tab (the first library name) and I see one suggestion in this list from the second library.

I had a glance through the code, but it is hard to see [quickly] where this comes from.
Could you please check?
Comment 15 Katrin Fischer 2013-08-19 10:54:51 UTC
Thx for taking a look Marcel - I will try to reproduce. Especially the Library filter is a bit tricky.
Comment 16 Katrin Fischer 2014-02-03 07:17:48 UTC
*** Bug 11667 has been marked as a duplicate of this bug. ***
Comment 17 Katrin Fischer 2014-02-04 15:05:12 UTC
*** Bug 11609 has been marked as a duplicate of this bug. ***
Comment 18 Katrin Fischer 2014-04-28 20:54:58 UTC
*** Bug 12154 has been marked as a duplicate of this bug. ***
Comment 19 Katrin Fischer 2014-04-28 20:55:19 UTC
I hope to get back to this after feature freeze.
Comment 20 Katrin Fischer 2014-04-28 20:57:18 UTC
... but won't be unhappy if someone else adopts this bug :)
Comment 21 Jacek Ablewicz 2014-06-10 08:14:04 UTC
(In reply to Katrin Fischer from comment #20)
> ... but won't be unhappy if someone else adopts this bug :)

I'm considering it. Warily :). What exactly is wrong with it? I mean, while it does not apply at the moment ("sha1 information is lacking or useless" blah blah), this part alone:

     my ($criteriumvalue,$displayby)=@_;
-    unless ( grep { /$criteriumvalue/ } qw(ASKED ACCEPTED REJECTED CHECKED) ) {
-        return GetAuthorisedValueByCode('SUGGEST_STATUS', $criteriumvalue ) || "Unknown";
+    if ($displayby =~ /status/i) {
+        if ( grep { /$criteriumvalue/ } qw(ASKED ACCEPTED REJECTED CHECKED ORDERED AVAILABLE) ) {
+            return ($criteriumvalue eq 'ASKED'?"Pending":ucfirst(lc( $criteriumvalue)));
+        } else {
+            return GetAuthorisedValueByCode('SUGGEST_STATUS', $criteriumvalue ) || $criteriumvalue;
+        }
     }
-    return ($criteriumvalue eq 'ASKED'?"Pending":ucfirst(lc( $criteriumvalue))) if ($displayby =~/status/i);
     return (GetBranchName($criteriumvalue)) if ($displayby =~/branchcode/);


is pretty perfect, IMO. Works like a charm, fixes all those ugly "Unknown"s introduced by commit 490688ea534addc3028ceac6b9a8d3f6816359d2.
Seems to be good for bug 12257 as well (while there is a working patch set in 12257, it only deals with "Unknown" person names).

So, what's a catch? ;)

BTW, something like /^$criteriumvalue$/ in grep regexp might be a bit better perhaps - in case someone puts e.g. 'ED' or 'CHECK' as authorized value code in SUGGEST_STATUS (not really important).
Comment 22 Katrin Fischer 2014-06-10 14:07:12 UTC
I think most of the patch is correct, but I was a bit worried abotu the library tabs when I submitted it and now think that leaving out that bit of code and concentrate on fixing the status and user names etc. would be good.

The library tab has a lot of problems really, when you start to think about independent branches and the like... but that's probably better done on another bug. 

I'd like to do a new version of the patch and include your suggestion - or you could do it and I could try to sign off?
Comment 23 Katrin Fischer 2014-06-10 14:10:14 UTC
I hope that made sense... it's way too hot here today for me :)
Comment 24 Jacek Ablewicz 2014-06-11 07:24:55 UTC Comment hidden (obsolete)
Comment 25 Paola Rossi 2014-06-13 16:08:09 UTC
I've applied against master 3.17.00.007

I've not be able to test the added custom statuses.

After having created 1 or 2 custom status (XX e YY) to SUGGEST_STATUS category, I couldn't attach these new statuses to any suggestion.

Editing a suggestion, the Status' dropdown list provided is:
No Status, Pending, Accepted, Checked, Rejected.

"Mark selected as" lets me choose a status among:
pending, accepted, checked, rejected.

There were no authorized values defined for SUGGEST_STATUS category before I added the two authorized values XX e YY.

So I pass the patch to "Failed QA" status.
Comment 26 Christopher Brannon 2014-06-13 16:55:47 UTC
I have to agree that this patch fails.  I too am testing on 3.17 in a sandbox, and when applied, I see no changes on the tabs.  The tabs still read as 'Unknown'.

Christopher
Comment 27 Jacek Ablewicz 2014-06-13 18:42:25 UTC
(In reply to Christopher Brannon from comment #26)
> I have to agree that this patch fails.  I too am testing on 3.17 in a
> sandbox, and when applied, I see no changes on the tabs.  The tabs still
> read as 'Unknown'.

It's kind of weird, because with this patch applied, there should no longer 
be an "Unknown" nowhere in the script nor in the template (another possibility is that "Unknown" is comming e.g. from the database, but that's not very likely IMO).

Maybe it does not (correctly) apply in sandbox, for some not yet determined reason.. I'll try to replicate this, can you provide a little more details - which sandbox did you tested it with, and with what sandbox settings?
Comment 28 Jacek Ablewicz 2014-06-13 19:05:51 UTC
(In reply to Paola Rossi from comment #25)

> I've not be able to test the added custom statuses.
> 
> After having created 1 or 2 custom status (XX e YY) to SUGGEST_STATUS
> category, I couldn't attach these new statuses to any suggestion.

Indeed, additional suggestion statuses do not currently work properly (since Bug 9261 push, unless I'm very much mistaken). Follow-up patch on the way.
Comment 29 Jacek Ablewicz 2014-06-13 19:42:26 UTC Comment hidden (obsolete)
Comment 30 Christopher Brannon 2014-06-13 19:52:57 UTC
(In reply to Jacek Ablewicz from comment #27)
> (In reply to Christopher Brannon from comment #26)
> > I have to agree that this patch fails.  I too am testing on 3.17 in a
> > sandbox, and when applied, I see no changes on the tabs.  The tabs still
> > read as 'Unknown'.
> 
> It's kind of weird, because with this patch applied, there should no longer 
> be an "Unknown" nowhere in the script nor in the template (another
> possibility is that "Unknown" is comming e.g. from the database, but that's
> not very likely IMO).
> 
> Maybe it does not (correctly) apply in sandbox, for some not yet determined
> reason.. I'll try to replicate this, can you provide a little more details -
> which sandbox did you tested it with, and with what sandbox settings?

I take it back.  I tried another sandbox and it is working.  I am seeing difference between sandbox 7 and sandbox 8.  I think 8 is having some issues.  I'll have to keep an eye on it.

The labels look great.  I agree, the organize by library selection is defunct.  I get absolutely nothing.  Also, I see that if you are using something like Acquisition Information filters, and set to Any, Any, if you make a change to a status, it clears these settings and you have to start all over.  It puts you back to the default view and the current library only.  May have to file a separate bug for that issue.

The labels are much better.

Waiting for the update for the custom statuses.

Christopher
Comment 31 Biblibre Sandboxes 2014-06-13 20:07:46 UTC
Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>
Comment 32 Biblibre Sandboxes 2014-06-13 20:08:17 UTC Comment hidden (obsolete)
Comment 33 Biblibre Sandboxes 2014-06-13 20:08:21 UTC Comment hidden (obsolete)
Comment 34 Christopher Brannon 2014-06-13 20:10:54 UTC
Works as test plan outlines.

Suggestions still need bug fixes for:
Library Sort
Maintaining filtering after status is changed.

Christopher
Comment 35 Katrin Fischer 2014-06-17 10:36:13 UTC
*** Bug 12433 has been marked as a duplicate of this bug. ***
Comment 36 Paola Rossi 2014-06-18 13:47:30 UTC
1) Special case to test for 10519:
I've applied 12257 against master 3.17.00.008.

As you certainly know, an anonymous purchase suggestion can be produced on OPAC changing "AnonSuggestions" pref to "Allow" value, and the "AnonymousPatron" pref to an "X" value (an existent patron's borrowernumber). As a patron, the AnonymousPatron "X" should be linked to a library.

I've created a new patron, and I've linked him to Centerville, the same library of the test-user I log in on intranet.
Then I've created an anonymous purchase suggestion on OPAC.

In Intranet ACQ, the AnonymousPatron "X" should be listed among the other users in the "Suggested by" dropdown list of the "Suggestion information" filter: OK, he's listed there.

In ACQ Suggestions, I've set "Centerville" in "Acquisition information" filter "For", and I've asked for all suggestions, f.i. organized by status.
In DB the anonymous suggestion has "ASKED" status, and is not linked to a library (its branchcode is NULL).
No anonymous suggestion is listed among the pending ones: KO.

In intranet, I've set "Any" in "Acquisition information" filter "For", and I've asked for all suggestions, f.i. organized by status.
This time the anonymous suggestion has been listed among the pending ones: OK.
So the problem is about the library, not about the status of the suggestion.

About KO: IMO on OPAC the koha/opac-suggestions.pl should set the AnonymousPatron's library to the anonymous suggestion branchcode, in case the APatron has one. So there could be a bug there.
Otherwise, I invite you to pass this 10519 patch to "Failed QA" status.

2) Note.
When "AnonSuggestions" pref is set to "Allow" value, koha lets the "AnonymousPatron" pref be kept to "0" value (non-existent patron), its default value.
But, if in koha every patron should be linked to a library, the "AnonymousPatron" should be linked too.
Setting to '0' the "AnonymousPatron" pref can violate a rule, and so it can give some problems.
Comment 37 Marcel de Rooy 2014-07-09 15:00:13 UTC
Will finish this QA soon.
Comment 38 Marcel de Rooy 2014-07-10 09:44:08 UTC
Created attachment 29598 [details] [review]
Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken

The tabbed display in suggestions offers different options to organize
the tabs. The descriptions on the tabs and some of the search options were
not working correctly, displaying as "Unknown".

To test:
- Add several suggestions to your installation, make sure you have:
  - suggestions from different users
  - suggestions managed by different users
  - suggestions with different statuses
  - suggestions with different selected item types

Test all the 'organize by' options (except "Organize by: Library"
- see note below), make sure that the tabs and search options
have correct descriptions and do no longer display as "Unknown".

- Add 1 or 2 custom status to SUGGEST_STATUS authorized value.
  - Verify display is still correct and your new status are displayed.

Note: "Organize by: Library" option is currently severely broken
(and not easily fixable, especially for 'IndependentBranches'
enabled). But this turns out to be a separate issue, with a different
underlaying causes, and it's outside the scope of this patch.
This should be dealt with later, in it's own bug report.

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2014-07-10 09:44:15 UTC
Created attachment 29599 [details] [review]
Bug 10519 - Suggestions: 'Organize by' and correct display of tab descriptions broken

Follow-up to fix additional suggestion statuses, which are not working
properly due to

   416:  borrowernumber           => $input->param('borrowernumber'),

being evaluated in list context inside $template->param() call arguments.

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2014-07-10 09:48:02 UTC
QA Comment:
Improves display of tab headings (especially Ordered). Also resolves display of additional suggestion statuses in combo box for Mark selected as.
Code looks good to me.
No complaints from qa tools.
Comment 41 Tomás Cohen Arazi 2014-07-14 19:34:01 UTC
Created attachment 29686 [details] [review]
Bug 10519: (followup) unit tests leave problematic cruft

On testing I've found that t/db_dependent/Suggestions.t leaves
cruft on the DB. Small followup for that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 42 Tomás Cohen Arazi 2014-07-14 19:36:44 UTC
Patches pushed to master.

Thanks Jacek!
Comment 43 Katrin Fischer 2014-08-10 16:06:25 UTC
*** Bug 12257 has been marked as a duplicate of this bug. ***
Comment 44 Galen Charlton 2014-09-23 20:27:51 UTC
Pushed to 3.16.x for inclusion in 3.16.4.
Comment 45 Fridolin Somers 2015-01-23 13:02:13 UTC
Pushed to 3.14.x will be in 3.14.13

Second commit :
c07a3d0 Bug 10519: (followup) Fix context for evalyating $borrowernumber
Is not needed, $borrowernumber does not existe on this line