Bug 18125 - Edit button can duplicate some circulation rule
Summary: Edit button can duplicate some circulation rule
Status: RESOLVED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-15 16:17 UTC by Baptiste
Modified: 2023-06-10 20:18 UTC (History)
10 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
Bug 17927: (QA followup) Fix timestamp nullable in hold.json (1019 bytes, patch)
2017-02-16 11:01 UTC, Baptiste
Details | Diff | Splinter Review
Disabled the select for patron Category and Item Type (1.34 KB, patch)
2017-02-16 11:41 UTC, Baptiste
Details | Diff | Splinter Review
[SIGNED-OFF] Disabled the select for patron Category and Item Type (1.40 KB, patch)
2017-05-09 13:31 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Baptiste 2017-02-15 16:17:44 UTC
If you modify the patron category and the item type when you edit a circulation rule, this will create a new circulation rule
Comment 1 Baptiste 2017-02-16 11:01:34 UTC Comment hidden (obsolete)
Comment 2 Baptiste 2017-02-16 11:41:48 UTC
Created attachment 60329 [details] [review]
Disabled the select for patron Category and Item Type

Test plan
1 - Edit a circulation rule and check you can modify the patron Category and item type
2 - Apply patch
3 - Edit a circulation rule : you can't change anymore
Comment 3 Andreas Hedström Mace 2017-02-28 05:58:38 UTC
Patch works as intended, but I'm not sure if this is the best solution? Allowing for quick copying of a circulation rule (simply, change patron category and/or item type) seems to me to be useful - although a bit illogical (why create a new rule, and not just change the edited one?). Is it a bug or a feature?

Even if this is the preferred method, I'm not to keen on the fact that patron category and itemtype still look like dropdowns, but can't be clicked. Had i not known about the issue and stumbled upon it, I would have thought that to be a bug!

Either they should not look like dropdowns and/or be grayed out, or similar.
Comment 4 Baptiste 2017-02-28 08:43:15 UTC
Hi !

I don't think there is a quite better solution without changing everything in the code, but if you have any better idea I can try it ;)

I mean, this situation is better than the old one, because there is no unexpected behaviour of the application, in the old one, you can click on edit, save, see the new rule as expected, not notice there is a new one and work with a new circulation rule.

The application works so :
- when you click on edit, every fields of the current row get copied into the last row.
- when you submit the edited lines (which has no other status than the other lines), it checks if there is a row in the base with the same key (library + patron category + Item type). Case not it creates a new one, otherwise it modifies the old one : this means if you accidentally create a new rule, you will destroy the existing one.

I think to solve completely the problem, the interface should be transformed at all, because with this working, we can't :
- write a function which copies without editing if the keys are the same
- write a function which edits without creating a new one if the keys are differents.

Moreover the comments in the code were expecting this behaviour, but I think somewhere in the versions the behaviour got lost.

I don't think an alert would be a good idea because :
- if you put it on submit button, you will get it whenever you create a new rule
- if you put it on the edit button, you will get it whenever you edit a rule and  it is quite annoying

A half solution could be to alert if any line is about to get replaced, but it is the same problem : the user will be alerted only if the button does his job.
Comment 5 Andreas Hedström Mace 2017-03-05 18:44:06 UTC
I see your point Baptiste! This could be an interim solution, rather than rewriting the whole circulation rules.

But is it perhaps possible to gray out the two dropdowns (patron category and item type) or similar, so that it will be apparent to the users that these are actually not clickable when editing a rule?
Comment 6 Baptiste 2017-03-17 16:22:58 UTC
Hi ! 
You mean gray out the field or the menus ? 
Because graying out the scrollable menus is the expected behavior of this patch..
Comment 7 Andreas Hedström Mace 2017-03-22 16:40:04 UTC
Yes, I mean graying out the scrollable menus. I can't click them, but I don't see anything visually that these are not editable. Or am I missing something?
Comment 8 Baptiste 2017-03-23 09:34:22 UTC
Hi,
What browser do you use ? On firefox and chromium it works as expected on my computer (I can clearly see a difference with other buttons).
Comment 9 Andreas Hedström Mace 2017-03-23 14:47:05 UTC
Hi Baptiste! I've been testing with Google Chrome. I just double-checked with Firefox, and there it looks good (the blue turns grey on the scrolldown menu)! In Chrome it's gray to start with though.
Comment 10 Baptiste 2017-04-25 08:32:38 UTC
May I change the status for needs signoff ?
Comment 11 Andreas Hedström Mace 2017-05-03 13:34:56 UTC
I'm changing the status to Needs signoff, since my previous issue was browser-related.
Comment 12 Owen Leonard 2017-05-09 13:31:05 UTC
Created attachment 63288 [details] [review]
[SIGNED-OFF] Disabled the select for patron Category and Item Type

Test plan
1 - Edit a circulation rule and check you can modify the patron Category 
    and item type
2 - Apply patch
3 - Edit a circulation rule : you can't change anymore

https://bugs.koha-community.org/show_bug.cgi?id=18125

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Jonathan Druart 2017-05-09 17:39:02 UTC
I am not sure this is acceptable, it will become hard to copy rules.
Maybe we need an additional "Copy" action button?
Comment 14 Marcel de Rooy 2017-05-19 08:56:02 UTC
(In reply to Jonathan Druart from comment #13)
> I am not sure this is acceptable, it will become hard to copy rules.
> Maybe we need an additional "Copy" action button?

Indeed. I am not so sure if the advantage of this oneliner outweights the disadvantage of no longer being able to copy a rule.
It should not be that hard to add a copy button?
Comment 15 Baptiste 2017-05-19 09:05:51 UTC
(In reply to Marcel de Rooy from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > I am not sure this is acceptable, it will become hard to copy rules.
> > Maybe we need an additional "Copy" action button?
> 
> Indeed. I am not so sure if the advantage of this oneliner outweights the
> disadvantage of no longer being able to copy a rule.
> It should not be that hard to add a copy button?

Hum.. the problem with a copy button it that the working of this button wouldn't be intuitive.

I mean if we create an copy button (like the old edit), this will not copy a rule if you don't modify the 2 first fields.
Comment 16 Marcel de Rooy 2017-05-19 09:10:21 UTC
(In reply to Baptiste from comment #15)
> (In reply to Marcel de Rooy from comment #14)
> > (In reply to Jonathan Druart from comment #13)
> > > I am not sure this is acceptable, it will become hard to copy rules.
> > > Maybe we need an additional "Copy" action button?
> > 
> > Indeed. I am not so sure if the advantage of this oneliner outweights the
> > disadvantage of no longer being able to copy a rule.
> > It should not be that hard to add a copy button?
> 
> Hum.. the problem with a copy button it that the working of this button
> wouldn't be intuitive.
> 
> I mean if we create an copy button (like the old edit), this will not copy a
> rule if you don't modify the 2 first fields.

Yeah, you may need to lift some check first and make sure it is applied later..
Comment 17 Baptiste 2017-07-07 15:41:31 UTC
The problem with a copy button is following: if you don't modify the key, it will edit an existing one (which is the worst behaviour I think)
Comment 18 Victor Grousset/tuxayo 2018-02-08 13:37:54 UTC
Baptiste doesn't work anymore at BibLibre.
Unassigning to avoid signaling that this bug is still being supervized. Which could wrongly discourage someone to continue the work.
Comment 19 Katrin Fischer 2023-06-10 20:18:53 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> Baptiste doesn't work anymore at BibLibre.
> Unassigning to avoid signaling that this bug is still being supervized.
> Which could wrongly discourage someone to continue the work.

The behaviour of editing also acting as an option to copy rules is described in the manual and really very useful. I think we cannot remove it without introducing another option for copying that works better than what we have now. 

Closing WONTFIX.