Bug 4354 - Cannot edit existing issuing rules
Summary: Cannot edit existing issuing rules
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
: 8363 (view as bug list)
Depends on:
Blocks: 17808
  Show dependency treegraph
 
Reported: 2010-03-30 04:57 UTC by Cindy Murdock Ames
Modified: 2016-12-23 09:23 UTC (History)
9 users (show)

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


Attachments
Proposed fix (6.67 KB, patch)
2010-05-27 17:47 UTC, Owen Leonard
Details | Diff | Splinter Review
Revised for current HEAD (6.67 KB, patch)
2010-12-31 13:54 UTC, Owen Leonard
Details | Diff | Splinter Review
Revised for current HEAD (6.66 KB, patch)
2011-01-05 15:16 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 4354: Cannot edit existing issuing rules (9.92 KB, patch)
2013-01-16 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 4354: Cannot edit existing issuing rules (10.24 KB, patch)
2013-01-24 13:52 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 4354: Cannot edit existing issuing rules (10.28 KB, patch)
2013-01-24 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4354 - Followup - Add row highlighting (1.66 KB, patch)
2013-01-24 14:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4354 - Followup - Add row highlighting (1.72 KB, patch)
2013-01-24 15:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 4354 - Followup - Add row highlighting (1.88 KB, patch)
2013-01-24 17:30 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 4354: Cannot edit existing issuing rules (10.38 KB, patch)
2013-03-17 15:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 4354 - Followup - Add row highlighting (1.97 KB, patch)
2013-03-17 15:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 4354: Followup Add a Cancel button (8.93 KB, patch)
2013-03-18 11:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 4354: Followup: FIX the hardduedate copy (4.03 KB, patch)
2013-03-21 11:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 4354: Followup: FIX the hardduedate copy (4.09 KB, patch)
2013-03-21 12:16 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 4354: Followup: FIX the hardduedate copy (4.09 KB, patch)
2013-03-21 12:53 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 4354: Followup: FIX the hardduedate copy (4.09 KB, patch)
2013-03-21 12:53 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 4354: Followup: FIX the hardduedate copy (4.09 KB, patch)
2013-03-21 12:56 UTC, Paul Poulain
Details | Diff | Splinter Review
Add robots txt file to deny all bots (653 bytes, patch)
2013-03-21 13:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 4354: Manage the specific case "Unlimited" value (1.53 KB, patch)
2013-03-21 14:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 4354: Cannot edit existing issuing rules (10.42 KB, patch)
2013-03-21 14:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 4354 - Followup - Add row highlighting (1.98 KB, patch)
2013-03-21 14:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 4354: Followup Add a Cancel button (9.00 KB, patch)
2013-03-21 14:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 4354: Followup: FIX the hardduedate copy (4.15 KB, patch)
2013-03-21 14:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 4354: Manage the specific case "Unlimited" value (1.60 KB, patch)
2013-03-21 14:52 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 Chris Cormack 2010-05-21 01:26:11 UTC


---- Reported by cmurdock@ccfls.org 2010-03-30 16:57:46 ----

The only way to change an existing issuing rule is to re-enter all the values for it as if you were adding a new rule.  There should probably be a simpler way to edit an existing issuing rule, so you can more easily change a single value if you want to.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:26 UTC  ---

This bug was previously known as _bug_ 4354 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4354

Actual time not defined. Setting to 0.0
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.

Comment 1 Owen Leonard 2010-05-27 17:47:01 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2010-06-24 19:41:37 UTC
+1

I've tested it. It works and is useful.
Comment 3 Owen Leonard 2010-12-31 13:54:32 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2011-01-05 15:16:34 UTC Comment hidden (obsolete)
Comment 5 Fridolin Somers 2011-01-06 13:36:08 UTC
(In reply to comment #4)
> Created attachment 2946 [details] [review]
> Revised for current HEAD

Hie,

Nice patch, it saves a lot of time.

I just had a problem after merging : 
When clicking on "edit", the submit button value changed to "Edit".
It javascript, you should modify to apply edition only on text input : 
L16 : 
$("#default-circulation-rules tr:last td:eq("+i+") input[type=text]").val(itm);

Regards,
Comment 6 dev_patches 2011-01-06 14:53:09 UTC
This has already been done in one of our patches waiting to be pushed. via jedit
Comment 7 Paul Poulain 2011-01-06 15:05:38 UTC
i thas also be done in our BibLibre-cat-circ branch. in smart-rules, each cell of the table can be edited and saved independantly. Very convenient, libraries loves it !
Comment 8 MJ Ray (software.coop) 2011-02-05 19:01:02 UTC
This bug is mentioned in:
Proposed fix for Bug 4354,	Cannot edit existing issuing rules http://lists.koha-community.org/pipermail/koha-patches/2011-January/013519.html
Proposed fix for Bug 4354,	Cannot edit existing issuing rules http://lists.koha-community.org/pipermail/koha-patches/2011-January/013512.html
Comment 9 Paul Poulain 2011-08-02 14:49:13 UTC

*** This bug has been marked as a duplicate of bug 5872 ***
Comment 10 Jonathan Druart 2013-01-16 15:53:37 UTC
*** Bug 8363 has been marked as a duplicate of this bug. ***
Comment 11 Jonathan Druart 2013-01-16 15:54:14 UTC Comment hidden (obsolete)
Comment 12 Nicole C. Engard 2013-01-24 13:52:04 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-01-24 13:56:13 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-01-24 14:22:32 UTC Comment hidden (obsolete)
Comment 15 Nicole C. Engard 2013-01-24 14:31:18 UTC
(In reply to comment #14)
> Created attachment 14821 [details] [review]
> Bug 4354 - Followup - Add row highlighting

Kyle,

I don't see any different in FF on Mac.  What am I looking for in the follow up?

Nicole
Comment 16 Jonathan Druart 2013-01-24 15:57:37 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-01-24 15:58:38 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > Created attachment 14821 [details] [review]
> > Bug 4354 - Followup - Add row highlighting
> 
> Kyle,
> 
> I don't see any different in FF on Mac.  What am I looking for in the follow
> up?
> 
> Nicole

Hi Nicole,
It works here with FF, try to reload your page with ctrl-r, clean your cache, etc.
Comment 18 Nicole C. Engard 2013-01-24 17:30:34 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2013-03-17 15:16:52 UTC
Before passing qa, this works and adds some usability but it still needs some attention IMO.

If I click Edit, I cannot revert my action. There is no Cancel button. If I have clicked Edit and I want to add another rule (old school), it is not possible without clicking save. Could we add a Cancel for that, or have another way out of edit mode?

If I edit and enter some values, and click on another Edit, I just lose all my values. I should not have clicked Edit, but could we prevent it? (Not a blocker)

I am not a design expert, but cannot resist to say that the orange line is helpful, but it may not be fully clear to every user how this works. (Not a blocker)

Please send a followup and switch back to Signed off, or send another awesome response :)
Comment 20 Marcel de Rooy 2013-03-17 15:17:27 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2013-03-17 15:17:41 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2013-03-18 11:52:23 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2013-03-21 10:46:50 UTC
Marcel - taking this bug from you as we are so close to feature freeze. Hope it's ok!
Comment 24 Katrin Fischer 2013-03-21 11:02:55 UTC
I would really love to see this going in, but I have found a problem with editing the hardduedate setting.

When you try to modify a rule that has no hard due date set (shows as: none defined) the field is filled with "defined" on modifying the rule. Also I think this part won't work correctly with translated templates as it seems to rely on the English strings in the template:

// specific processing for the Hard due date column
var s = itm.split(' ');
var select_value;
if ( s[0] == "before" ) {
  select_value = -1
} else if ( s[0] == "on" ) {
  select_value = 0
} else if ( s[0] == "after" ) {
  select_value = 1
}

Jonathan - could you take a look at that? If it's not too hard to fix switch back to signed off.
Comment 25 Jonathan Druart 2013-03-21 11:34:55 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2013-03-21 11:37:03 UTC
(In reply to comment #24)
> Jonathan - could you take a look at that? If it's not too hard to fix switch
> back to signed off.

Fixed! I switch back to Needs Signoff, I have found a volunteer for testing :)
Comment 27 Paul Poulain 2013-03-21 12:16:33 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 28 Paul Poulain 2013-03-21 12:16:54 UTC Comment hidden (obsolete)
Comment 29 Paul Poulain 2013-03-21 12:52:33 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 30 Paul Poulain 2013-03-21 12:53:00 UTC Comment hidden (obsolete)
Comment 31 Paul Poulain 2013-03-21 12:53:31 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 32 Paul Poulain 2013-03-21 12:53:57 UTC Comment hidden (obsolete)
Comment 33 Paul Poulain 2013-03-21 12:55:31 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 34 Paul Poulain 2013-03-21 12:56:00 UTC Comment hidden (obsolete)
Comment 35 Paul Poulain 2013-03-21 13:03:34 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 36 Paul Poulain 2013-03-21 13:05:37 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 37 Paul Poulain 2013-03-21 13:06:13 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2013-03-21 13:10:52 UTC
Comment on attachment 16618 [details] [review]
Add robots txt file to deny all bots

Wrong file, Paul?
Comment 39 Paul Poulain 2013-03-21 13:28:36 UTC
(In reply to comment #38)
> Comment on attachment 16618 [details] [review] [review]
> Add robots txt file to deny all bots
> 
> Wrong file, Paul?

It's not really me in fact, it's a sandbox. I'll ask Cedric Vita, that is the real signoffer, and is in BibLibre office for hackfest ;-)
Comment 40 Katrin Fischer 2013-03-21 14:27:29 UTC
We are just about there - when you have Current checkouts allowed empty the field should be empty if it says "Unlimited".
Comment 41 Jonathan Druart 2013-03-21 14:44:35 UTC Comment hidden (obsolete)
Comment 42 Katrin Fischer 2013-03-21 14:51:10 UTC
Created attachment 16654 [details] [review]
[PASSED QA] Bug 4354: Cannot edit existing issuing rules

This patch adds javascript which pulls the values from the
chosen row of data and populates the last row's form with
that data. Some changes have been made to the markup to
better accommodate this.

Possibly missing is some kind of visual feedback to the user
that the edit action is taking place in a different row from
where they clicked. Comments are especially welcomed on
this aspect of the interaction.

The original author of this patch is Owen Leonard <oleonard@myacpl.org>

Test Plan:
* Edit an existing circulation rule
* Delete a circulation rule
* Create a new circulation rule
* Create a new circulation rule that is a dup of an old one
and make sure that it still overwrites the old rule like before

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>

All tests passed.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comments on last patch.
Comment 43 Katrin Fischer 2013-03-21 14:51:34 UTC
Created attachment 16655 [details] [review]
[PASSED QA] Bug 4354 - Followup - Add row highlighting

Test plan:
* Click on a row to edit
* Note that the row is highlighted in orange

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
All tests pass.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 44 Katrin Fischer 2013-03-21 14:51:58 UTC
Created attachment 16656 [details] [review]
[PASSED QA] Bug 4354: Followup Add a Cancel button

New cancel button on the edit row.
Add a prevent message when editing a new row.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 45 Katrin Fischer 2013-03-21 14:52:18 UTC
Created attachment 16657 [details] [review]
[PASSED QA] Bug 4354: Followup: FIX the hardduedate copy

Before this patch, the copy was based on the string (could be
translated!). Now is base on the -1, 0, 1 values.
The trick is to backup the hardduedatecompare value for each line of the
table.

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 46 Katrin Fischer 2013-03-21 14:52:40 UTC
Created attachment 16658 [details] [review]
[PASSED QA] Bug 4354: Manage the specific case "Unlimited" value

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All good :)
Comment 47 Jared Camins-Esakov 2013-03-21 23:56:15 UTC
Katrin,

Did you have any comments? The first patch says "Comments on last patch," but the last patch only says "All good," which makes me think I may be missing something somewhere.
Comment 48 Jared Camins-Esakov 2013-03-22 00:09:07 UTC
This patch has been pushed to master. I made the decision that since all seemed to work in my testing to just push this. If there's a problem I will revert it.
Comment 49 Katrin Fischer 2013-03-22 05:50:20 UTC
Sorry, I rebased and did my usual pattern - no real comments.