Bug 9533 - batch item modification should ignore defaults
Summary: batch item modification should ignore defaults
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Kyle M Hall (khall)
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 10887
  Show dependency treegraph
 
Reported: 2013-02-01 16:59 UTC by Nicole C. Engard
Modified: 2015-02-16 14:44 UTC (History)
8 users (show)

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


Attachments
Bug 9533 - batch item modification should ignore defaults (3.89 KB, patch)
2013-04-17 13:39 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 9533 - batch item modification should ignore defaults (3.89 KB, patch)
2013-04-17 13:42 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 9533 - batch item modification should ignore defaults (3.89 KB, patch)
2013-05-01 15:23 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 9533 - batch item modification should ignore defaults (4.07 KB, patch)
2013-05-01 15:24 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
[Signed Off] Bug 9533 - batch item modification should ignore defaults (4.13 KB, patch)
2013-05-02 07:54 UTC, David Cook
Details | Diff | Splinter Review
Bug 9533: Use default values by default (1.23 KB, patch)
2013-05-03 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9533: Use default values by default (1.28 KB, patch)
2013-05-03 17:22 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
[PASSED QA] Bug 9533 - batch item modification should ignore defaults (4.46 KB, patch)
2013-05-12 12:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 9533: Use default values by default (1.40 KB, patch)
2013-05-12 12:06 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2013-02-01 16:59:12 UTC
If you have a default value set for an item field in the framework it shows up on the batch item modification form.  That form should be completely blank I think and ignore all defaults.
Comment 1 Kyle M Hall (khall) 2013-04-17 13:39:29 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall (khall) 2013-04-17 13:42:40 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2013-04-17 15:52:05 UTC
This works correctly but there is one small problem with the markup: The checkbox needs to work when you click on the accompanying text. Here's the markup I recommend:

<fieldset class="rows">
    <legend>Use default values</legend>
    <ol class="radio">
        <li>
            <label>
                <input type="checkbox" name="use_default_values" id="use_default_values" />
                Populate fields with default values from default framework</label>
        </li>
    </ol>
</fieldset>
Comment 4 Kyle M Hall (khall) 2013-05-01 15:23:06 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall (khall) 2013-05-01 15:24:03 UTC Comment hidden (obsolete)
Comment 6 David Cook 2013-05-02 07:54:38 UTC Comment hidden (obsolete)
Comment 7 David Cook 2013-05-02 07:55:03 UTC
Works as described.
Comment 8 Jonathan Druart 2013-05-02 13:54:19 UTC
Maybe should we enable the checkbox by default?
Comment 9 Jonathan Druart 2013-05-03 14:15:30 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall (khall) 2013-05-03 17:22:46 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-05-06 07:57:18 UTC
Kyle, I just see you wrote in your commit message "off by default". So maybe my patch is useless? I don't want to impose this choice :)
Comment 12 Kyle M Hall 2013-05-06 19:33:35 UTC
(In reply to comment #11)
> Kyle, I just see you wrote in your commit message "off by default". So maybe
> my patch is useless? I don't want to impose this choice :)

I personally think it should be off by default, but it can be switched by a bit of jquery either way.
Comment 13 Jonathan Druart 2013-05-07 14:11:34 UTC
I prefer someone else qa the last patch.
Comment 14 Katrin Fischer 2013-05-12 10:53:39 UTC
Hm, I think maybe having this off by default would be safer?
I wonder if the field is not empty, will the default overwrite what you have in there?
Comment 15 Katrin Fischer 2013-05-12 11:44:06 UTC
Testing this I was surprised to find the option on the upload page instead of on the page where I actually define the modifications.
I tend to check all checkboxes and settings I made twice before I do a change like this, with the options being spread over 2 pages, I can't do that. I think the option should go on the second step.
Comment 16 Katrin Fischer 2013-05-12 11:47:48 UTC
Thinking more about this, a possible enhancement would be to make visible which fields will be set to which default value. There is no way to tell now which changes will be done or not done when you check/uncheck the checkbox. I know that this is more complicated so it should go on a new bug as an enhancement request once have figured this out.
Comment 17 Katrin Fischer 2013-05-12 12:01:54 UTC
Ok, I think I need to take a break, because after some more testing I understand better. Ignore last 2 comments, I will try again:

1) Having the feature off by default might be better in my opinion, because it reduces the risk of unintended changes.

2) I can see that the default values for text fields show up in the form, which is good as it makes visible which changes will happen. But there is a bug here:
If you use a default itemtype, the pull down will not be preselected. This seems to happen for all values with pull downs. 

3) I'd still like to see the checkbox on the second page instead of the first, but it might not be as big as a problem as I initially thought.
Comment 18 Katrin Fischer 2013-05-12 12:05:37 UTC
Created attachment 18094 [details] [review]
[PASSED QA] Bug 9533 - batch item modification should ignore defaults

This patch makes the pre-population of the fields for the batch
item editor with the default values from the default framework
optional and off by default.

Test Plan:
1) Apply patch
2) Add default values for cost and replacement cost to the
   default framework.
3) Browse to Tools / Batch item modification
4) Choose a file, or entire some barcodes in the text area
5) Ensure that the 'Populate fields with default values from default
   framework' is *not* checked
6) Click 'Continue'
7) Observe that the fields for cost and replacement cost are blank
8) Click the 'back' button on your browser
9) Check the checkbox to enable the default value
10) Click 'Continue'
11) Observe that the fields for cost and replacement cost now contain
    the default values you assigned them in the default MARC framework

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Switch works correctly, all tests and QA script pass.

Note: there is a bug with default values for all non text input
fields. Default values won't be applied for those fields as the
pull downs are not preselected correctly with the default values
defined in the framework.
Comment 19 Katrin Fischer 2013-05-12 12:06:49 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2013-05-12 12:07:52 UTC
Comment on attachment 18095 [details] [review]
[PASSED QA] Bug 9533: Use default values by default

Obsoleting second patch, but also passing QA on it. The change is alright
if we want the checkbox to be checked by default.
Comment 21 Marcel de Rooy 2013-05-23 08:28:24 UTC
Version back to master.
Comment 22 Galen Charlton 2013-07-05 15:57:39 UTC
Pushed to master.  Thanks, Kyle!

I do, however, agree with Katrin that the ability to control which default values are presented really belongs on the item field form, not the upload form.
Comment 23 Tomás Cohen Arazi (tcohen) 2013-07-15 23:46:40 UTC
This patch has been pushed to 3.12.x, will be in 3.12.2.

Thanks Kyle!
Comment 24 Bernardo Gonzalez Kriegel 2013-08-03 20:10:05 UTC
Pushed to 3.10.x, will be in 3.10.10
Comment 25 Chris Hall 2013-08-18 04:28:20 UTC
Pushed to 3.8.x, will be in 3.8.17