Actually, there is a lot of weirdness in the tab display going on there for me, there might be more wrong about it.

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?

Finding more problems:
return GetAuthorisedValueByCode('SUGGEST_STATUS', $criteriumvalue ) || "Unknown";
As the template doesn't check for this value, it's untranslatable.

Changing the bug title to reflect that this has grown into a bigger problem, I am working on a patch.

> 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.

The last patch in combination with bug 4907 seems to make things work better, but please test thoroughly.

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?

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?

Thx for taking a look Marcel - I will try to reproduce. Especially the Library filter is a bit tricky.

*** Bug 11667 has been marked as a duplicate of this bug. ***

*** Bug 11609 has been marked as a duplicate of this bug. ***

*** Bug 12154 has been marked as a duplicate of this bug. ***

I hope to get back to this after feature freeze.

... but won't be unhappy if someone else adopts this bug :)

(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).

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?

I hope that made sense... it's way too hot here today for me :)

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.

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

(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?

(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.

(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

Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>

Works as test plan outlines.

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

Christopher

*** Bug 12433 has been marked as a duplicate of this bug. ***

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.

Will finish this QA soon.

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>

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>

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.

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>

Patches pushed to master.

Thanks Jacek!

*** Bug 12257 has been marked as a duplicate of this bug. ***

Pushed to 3.16.x for inclusion in 3.16.4.

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