Bug 34585 - "When to charge" columns value not copied when editing circulation rule
Summary: "When to charge" columns value not copied when editing circulation rule
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-08-22 10:38 UTC by Emmi Takkinen
Modified: 2024-04-03 07:31 UTC (History)
3 users (show)

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


Attachments
Bug 34585: Add data-code attribute to chargeperiod_charge_at selector (2.43 KB, patch)
2023-08-22 10:46 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34585: Fix behaviour of select dropdowns when editing circulation rules (4.47 KB, patch)
2023-12-29 12:32 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34585: Fix behaviour of select dropdowns when editing circulation rules (4.52 KB, patch)
2024-01-03 16:32 UTC, Lucas Gass
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2023-08-22 10:38:38 UTC
When one tries to edit circulation rule "When to charge" columns value is always set as "End of interval". This happens because column doesn't have data-code attribute which defines existing rules value.

To reproduce:
1. Navigate to Circulation and fine rules page.
2. Find or create a rule with a "When to charge" value set as "Start of interval".
3. Edit rule.
=> Note that "When to charge" value is now "End of interval", while as the other values should be copied as they were.
Comment 1 Emmi Takkinen 2023-08-22 10:46:31 UTC
Created attachment 154688 [details] [review]
Bug 34585: Add data-code attribute to chargeperiod_charge_at selector

When one tries to edit circulation rule "When to charge"
columns value is always set as "End of interval". This patch adds
data-code attribute to chargeperiod_charge_at selector
to define existing rules value.

To reproduce:
1. Navigate to Circulation and fine rules page.
2. Find or create a rule with a "When to charge" value set as
"Start of interval".
3. Edit rule.
=> Note that "When to charge" value is now "End of interval",
while as the other values should be copied as they were.
4. Apply this patch.
5. Edit rule again.
=> "When to charge" should now be copied correctly.

Sponsored-by: Koha-Suomi Oy
Comment 2 Owen Leonard 2023-08-24 16:54:20 UTC
This isn't working for me. When I edit a rule with the setting "Start of interval," the "edit" button correctly updates the dropdown to that value, but when I click the "Clear" button and then edit a rule with the setting "End of interval," the value doesn't get set correctly.
Comment 3 Johanna Miettunen 2023-08-25 14:26:08 UTC
After applying the patch, value on "When to charge" column is copied as it is in the rule before editing. In my tests the patch works. I can't repeat the situation that Owen is describing. 

But, there are couple things I noticed. 
1) On our current production Koha (23.05.01.003, custom build: KK-Koha 23.05.848) value on "When to charge" column is copied as it is when editing the rule. So, our library doesn't seem to have this problems Emmi is reporting in the first place. Maybe we already have some fix for this...

2) I think the one problem is with the Clear button and how it behaves. This happens in our production Koha and in sandbox tests before and after applying the patch. 
1. Edit rule A, values are copied, but then decide to cancel, click Clear
2. Edit rule B, values might be copied, but then decide to cancel, click Clear
3. Edit rule C, not all values are not copied

All in all, it seems that even after the first time, when one cancels by clicking Clear, it is most reliable to first refresh the page and and then start editing the same rule or the next rule.

I feel silly writing this, but I thing Emmi's patch is partially fixing the error at least on test sandbox. But if one cancels the editing process, the Koha/page can't keep up and editing mode starts behaving weirdly, and can't copy data on rule row as it maybe should.
Comment 4 Emmi Takkinen 2023-09-26 10:45:36 UTC
I was able to reproduce Owens problem on our 22.xx version. This also happens on current master, so it might be that National library indeed has some sort of fix for this. 

I also noticed that Owens problem happens also with rules "OPAC item level holds", "On shelf holds allowed", "Automatic renewal", "Hard due date", "Unit", "Days mode" and "Patron category". Basically with every dropdown selector except "Item type". This could also have something to do with problem Johanna described. But this should probably have its own bug.
Comment 5 Emmi Takkinen 2023-12-01 07:43:41 UTC
Pressing "Cancel" button should set first select option as selected and it does. However previous selection is never cleared. So every time rule row is selected to be edited it's select options receive "selected" attribute which persists. When new value is selected, it's select options still have previous rules options as selected.
Comment 6 Emmi Takkinen 2023-12-29 12:32:47 UTC
Created attachment 160370 [details] [review]
Bug 34585: Fix behaviour of select dropdowns when editing circulation rules

When chancing rule to edit via "Edit" button in circulation rules table,
dropdown values aren't always changed correctly. Pressing "Edit" first time
works, but after that previous rules values persist. Same happens when one
tries to clear edit row, values aren't set to default values. This happens
with every input with a dropdown in it. This mostly happanes because we set
attribute "selected" to selected options, but never clear them which leads
to values persisting. In columns "Patron category" and "Itemtype" value "All",
or "*" as it is in code, doesn't match to any existing values from database
and can't be changed via pressing "Edit" button.

To reproduce:
1. Select a rule to edit and then select another rule to edit.
2. Repeat this couple of times and observe what values dropdowns fields display
in edit row.
=> Selected values in dropdowns aren't always changed based on rule that you are
currently editing. Instead after changing rule couple of times value doesn't change at all.
3. Press "Clear" button to return edit rows values back to default values.
=> Values in dropdowns aren't set back as default, instead they have same values as rule
you edited before pressing "Clear".

After reproducing problem, apply this patch and proceed to test "Clear" and "Edit" buttons.

To test "Edit" button:
1. Again, select a rule to edit and then select a another rule to edit.
2. Observe what values dropdowns fields display in edit row.
=> Confirm that values are changed based on rule that you are currently editing
and this behaviour doesn't change after selecting new rule to edit couple of times.

To test "Clear" button:
1. Press "Clear" button.
=> Values in dropdowns should now be set as default values.

Sponsored-by: Koha-Suomi Oy
Comment 7 Lucas Gass 2024-01-03 16:32:06 UTC
Created attachment 160488 [details] [review]
Bug 34585: Fix behaviour of select dropdowns when editing circulation rules

When chancing rule to edit via "Edit" button in circulation rules table,
dropdown values aren't always changed correctly. Pressing "Edit" first time
works, but after that previous rules values persist. Same happens when one
tries to clear edit row, values aren't set to default values. This happens
with every input with a dropdown in it. This mostly happanes because we set
attribute "selected" to selected options, but never clear them which leads
to values persisting. In columns "Patron category" and "Itemtype" value "All",
or "*" as it is in code, doesn't match to any existing values from database
and can't be changed via pressing "Edit" button.

To reproduce:
1. Select a rule to edit and then select another rule to edit.
2. Repeat this couple of times and observe what values dropdowns fields display
in edit row.
=> Selected values in dropdowns aren't always changed based on rule that you are
currently editing. Instead after changing rule couple of times value doesn't change at all.
3. Press "Clear" button to return edit rows values back to default values.
=> Values in dropdowns aren't set back as default, instead they have same values as rule
you edited before pressing "Clear".

After reproducing problem, apply this patch and proceed to test "Clear" and "Edit" buttons.

To test "Edit" button:
1. Again, select a rule to edit and then select a another rule to edit.
2. Observe what values dropdowns fields display in edit row.
=> Confirm that values are changed based on rule that you are currently editing
and this behaviour doesn't change after selecting new rule to edit couple of times.

To test "Clear" button:
1. Press "Clear" button.
=> Values in dropdowns should now be set as default values.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 8 Victor Grousset/tuxayo 2024-01-16 20:36:20 UTC
What's the status of 1st attachment?
"Bug 34585: Add data-code attribute to chargeperiod_charge_at selector"

Is it valid?
Was is applied when signing off?
It doesn't change the same area of the code so it's not sure it's an old version. And it has a different test plan.
Comment 9 Victor Grousset/tuxayo 2024-01-16 20:44:51 UTC
CCing Lucas to have confirmation about what they signed off.
Comment 10 Emmi Takkinen 2024-01-26 11:07:11 UTC
(In reply to Victor Grousset/tuxayo from comment #8)
> What's the status of 1st attachment?
> "Bug 34585: Add data-code attribute to chargeperiod_charge_at selector"
> 
> Is it valid?
> Was is applied when signing off?
> It doesn't change the same area of the code so it's not sure it's an old
> version. And it has a different test plan.

Yes it's still valid. These are two separate patches fixing two different problems. First just adds data-code attribute to chargeperiod_charge_at selector so that its value is copied correctly. Second one fixes input fields with dropdowns which currently aren't always cleared as they should be.