Bug 11880 - Select the barcode index by default when searching for label items
Summary: Select the barcode index by default when searching for label items
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Frédérick Capovilla
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-28 21:31 UTC by Frédérick Capovilla
Modified: 2016-06-21 21:39 UTC (History)
12 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Select the barcode index by default then searching for label items. (1.14 KB, patch)
2014-02-28 21:31 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
[Signed-off] Select the barcode index by default then searching for label items. (1.60 KB, patch)
2014-03-04 16:13 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 11880: Select what has been selected before (4.73 KB, patch)
2015-04-24 08:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 11880: Select what has been selected before (4.95 KB, patch)
2015-04-24 09:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 11880: Select the barcode index by default then searching for label items. (1.56 KB, patch)
2015-07-03 08:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11880: Select what has been selected before (4.94 KB, patch)
2015-07-03 08:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédérick Capovilla 2014-02-28 21:31:40 UTC Comment hidden (obsolete)
Comment 1 Marc Véron 2014-03-04 16:13:30 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-03-18 12:01:57 UTC
Frédérick:

The patch does:
  IF selected:
    select option
  ELSE:
    select option

It seems it works like that currently but it is definitively not a good way to do.
Do you know if the "selected" variable is passed to the template? I don't see any occurrence in the pl script.
Maybe could we remove all previous tests and always select the barcode option?

Marked as Failed QA.
Comment 3 Frédérick Capovilla 2014-03-18 20:06:49 UTC
The code in this template seems strange.
If (selected==true), it looks like ALL the options of the selectbox be set as selected. Am I right?

When we did our patch, we didn't touch the rest of the template because we didn't know why the page was set like that.

I need a second opinion before removing the "selected" option, since this fix goes beyond this simple patch.
Comment 4 Jonathan Druart 2014-03-19 09:44:22 UTC
(In reply to Frédérick Capovilla from comment #3)
> The code in this template seems strange.
> If (selected==true), it looks like ALL the options of the selectbox be set
> as selected. Am I right?

... yes!
But I did not find any situation where "selected" is set.
Comment 5 Nicole C. Engard 2015-03-06 11:14:52 UTC
I'm confused by this. I was going to sign off but see the notes at the bottom so I'm re-marking failed qa.  If that's wrong feel free to fix.

Nicole
Comment 6 Charles Farmer 2015-03-09 15:01:24 UTC
Despite the ugliness of the template in its present form, the patch works and is a development needed by our client.

This patch should be accepted as is. Fixing the template is considerably out of the scope of this development.
Comment 7 Aleisha Amohia 2015-04-24 04:37:23 UTC
Patch applies cleanly, signing off
Comment 8 Jonathan Druart 2015-04-24 08:06:17 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-04-24 08:08:44 UTC
(In reply to Charles Farmer from comment #6)
> Despite the ugliness of the template in its present form, the patch works
> and is a development needed by our client.
> 
> This patch should be accepted as is. Fixing the template is considerably out
> of the scope of this development.

It's not because it's needed by your client that the patch should be accepted as you have written it... It's your duty to sent clean patches and make the code better, not worst.
The patch you have sent does not make any sense and adds misunderstanding to the template.

Anyway, here a patch to clean the situation, it took me 5 minutes to write, much more less than argue for hours...

Back to needs signoff.
Comment 10 Marc Véron 2015-04-24 09:19:31 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2015-07-03 08:19:44 UTC
Created attachment 40774 [details] [review]
Bug 11880: Select the barcode index by default then searching for label items.

Test case:
* Go to "Tools -> Label creator -> Manage batches"
* Click on the "New batch" button
* Click on the "Add item(s)" button
* A search window should open. The "Barcode" value should be selected in the the selectbox.

Followed test case. Patch behaves as expected.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2015-07-03 08:19:49 UTC
Created attachment 40775 [details] [review]
Bug 11880: Select what has been selected before

The previous patch wants to select the barcode option when the user is
on the item search for label, in all cases.
So even if 'title' is selected, a search done, and there is no result,
the barcode index is selected anyway.
In this case, the title option should be kept.

Test plan:
Confirm that the barcode option is the default choice, but other values
are kept if the search does not return any result.

Followed test plan, behaves as expected. Tested with all choices.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2015-07-03 08:21:17 UTC
Thx for cleaning up, Jonathan.
Comment 14 Tomás Cohen Arazi 2015-08-28 13:27:00 UTC
Patches pushed to master.

Thanks Frederick and Jonathan!
Comment 15 Liz Rea 2015-09-22 01:26:21 UTC
Pushed to 3.18.x will be in 3.18.11.