Bug 12365

Same would go for preference settings.

Created attachment 61865 [details] [review]
Bug 12365: Adding a note column to circ rules

This patch adds a note column to the circ and fine rules to act as a
description line for the rule.

To test:
1) Apply patch, update database (may have to also update schema)
2) Go to Admin -> Circ and fine rules
3) Scroll to the right, notice new Note column
4) Confirm it works to create a brand new rule with a Note
5) Confirm it works to modify an existing rule with a Note

Sponsored-by: Catalyst IT

Found a minor issue while testing: If you have text in the note field for 'All libraries' + 'All item types', the text 'sticks' in the input field. This is due to setting the value in smart-rules.tt line 384:

<td><input type="text" name="note" id="note" size="15" value="[% rule.note %]"></td>

Works well if I change the line to:
<td><input type="text" name="note" id="note" size="15"></td>

61865 - Bug 12365: Adding a note column to circ rules

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12365: Adding a note column to circ rules
Using index info to reconstruct a base tree...
M	Koha/Schema/Result/Issuingrule.pm
M	admin/smart-rules.pl
M	installer/data/mysql/kohastructure.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging admin/smart-rules.pl
Auto-merging Koha/Schema/Result/Issuingrule.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Issuingrule.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 12365: Adding a note column to circ rules
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-12365-Adding-a-note-column-to-circ-rules-pR_pTz.patch

Unfortunately, still having merge conflicts with the exact conflict message Katrin indicated previously.

Note: I ran reset_all in kohadevbox to guarantee a clean devbox to apply it to.

Academy keyword, please do not sign it off. Let it to KohaCon17 hackfesters.

(In reply to Jonathan Druart from comment #9)
> Academy keyword, please do not sign it off. Let it to KohaCon17 hackfesters.

Thanks for the information...wasn't aware of the significance of the academy keyword!

Patch tested with a sandbox, by macon  lauren KohaCon17 <caballeromaricon@gmail.com>

Created attachment 76609 [details] [review]
Bug 12365 - Notes for rules

Lee Jamison <ldjamison@marywood.edu> rescued this patch.
It is a clean re-build of attachments 61865 and 63902
to fix the merge conflict introduced during application of both
attachments.

To test:
Follow test plan in Comment 2

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: macon  lauren KohaCon17 <caballeromaricon@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 76637 [details] [review]
Bug 12365: Notes for rules

This patch adds a note column to the circ and fine rules to act as a
description line for the rule.

To test:
1) Apply patch, update database (may have to also update schema)
2) Go to Admin -> Circ and fine rules
3) Scroll to the right, notice new Note column
4) Confirm it works to create a brand new rule with a Note
5) Confirm it works to modify an existing rule with a Note

Sponsored-by: Catalyst IT

Lee Jamison <ldjamison@marywood.edu> rescued this patch.
It is a clean re-build of attachments 61865 and 63902
to fix the merge conflict introduced during application of both
attachments.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: macon  lauren KohaCon17 <caballeromaricon@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

I amended the commit message to include the original test plan.

A couple of follow-ups I'd like to see:

1. Put a maxlength attribute on the form field, since the database column limits to 100 characters.
2. Add some way to show/hide the notes to conserve space, perhaps a popover? https://getbootstrap.com/docs/3.3/javascript/#popovers

Created attachment 76658 [details] [review]
Bug 12365: [FOLLOW-UP] Adding maxlength attribute and popover

Test that you can't add a note of more than 100 characters.
Test notes with rules, notes are hidden by 'Click to view note' and the
popover correctly displays note.

Hi Aleisha, thx for the follow-up!

Some notes:

1) I'd like to see "click to view note" changed to just "view note". As the text is formatted as a link, there should be no need to add the additional instruction.

2) When there are multiple lines with notes and you click on one of them, they will just overlay each other, with the first note being behind the second and so on, There seems to be no way to make them disappear again. I think my expectation was that the note would disappear clicking on the next note or just clicking outside the note field again. A longer note can also overlay the buttons, so I think putting some more thought into this would be worthwhile.

3) Tiny: Can you make your patch subject: Bug XXXX: (follow-up)? I would have fixed it as part of QA, but then stumbled on 2). (see: https://wiki.koha-community.org/wiki/Commit_messages#Follow-ups)

Thx!

4) Editing a note no longer works!

Created attachment 77147 [details] [review]
Bug 12365: (follow-up) Adding maxlength attribute and popover

Test that you can't add a note of more than 100 characters.
Test notes with rules, notes are hidden by 'View note' and the
popover correctly displays note.
View the note by hovering over the link
I've moved the Note column so the popover displays nicely and doesn't
cover the buttons. I've also changed the placement so that it doesn't
cover information in the row.

Aleisha, did you see my comment about not being able to edit the note because it pulls the pop-over note into the input field?

Created attachment 77275 [details] [review]
Bug 12365: (follow-up) Adding maxlength attribute and popover

Test that you can't add a note of more than 100 characters.
Test notes with rules, notes are hidden by 'View note' and the
popover correctly displays note.
View the note by hovering over the link
I've moved the Note column so the popover displays nicely and doesn't
cover the buttons. I've also changed the placement so that it doesn't
cover information in the row.

Update: Test that you can edit the note when clicking 'Edit' and that
behaviour is the same for editing all other fields

I got the following error when applying the notes for rules patch. 
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging admin/smart-rules.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 12365: Notes for rules
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-12365-Notes-for-rules-osNzXn.patch

Lisette

Created attachment 78685 [details] [review]
Bug 12365: Notes for rules

This patch adds a note column to the circ and fine rules to act as a
description line for the rule.

To test:
1) Apply patch, update database (may have to also update schema)
2) Go to Admin -> Circ and fine rules
3) Scroll to the right, notice new Note column
4) Confirm it works to create a brand new rule with a Note
5) Confirm it works to modify an existing rule with a Note

Sponsored-by: Catalyst IT

Lee Jamison <ldjamison@marywood.edu> rescued this patch.
It is a clean re-build of attachments 61865 and 63902
to fix the merge conflict introduced during application of both
attachments.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: macon  lauren KohaCon17 <caballeromaricon@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 78686 [details] [review]
Bug 12365: (follow-up) Adding maxlength attribute and popover

Test that you can't add a note of more than 100 characters.
Test notes with rules, notes are hidden by 'View note' and the
popover correctly displays note.
View the note by hovering over the link
I've moved the Note column so the popover displays nicely and doesn't
cover the buttons. I've also changed the placement so that it doesn't
cover information in the row.

Update: Test that you can edit the note when clicking 'Edit' and that
behaviour is the same for editing all other fields

Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
        installer/data/mysql/atomicupdate/bug_12365_-_add_note_column_to_issuingrules.perl
Please move or remove them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0001 Bug 12365: Notes for rules
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-12365-Notes-for-rules-Ei3222.patch

Lisette

Ahhh that file is left over from the last time you tested, can you checkout master again, then check that file is gone before applying the patch please

Thanks
Chris

Was able to apply properly but when trying to add a note both with a new rule and when editing an existing rule, I got the following error: 
No property note for Koha::IssuingRule at /usr/share/perl5/Exception/Class/Base.pm line 73

Lisette

Note for QA: This will conflict with bug 15522.

Comment on attachment 78685 [details] [review]
Bug 12365: Notes for rules

Review of attachment 78685 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/bug_12365_-_add_note_column_to_issuingrules.perl
@@ +1,4 @@
> +$DBversion = 'XXX';
> +if( CheckVersion( $DBversion ) ) {
> +    unless( column_exists( 'issuingrules', 'note' ) ) {
> +        $dbh->do(q|ALTER TABLE issuingrules ADD note varchar(100) default NULL AFTER article_requests|);

backticks may be in order.

can you explain?

I am catching up on bugs, and noticed that it was noted that this is actually modifying the rules table with a new column.  I would like to reiterate, if it isn't too late, that it might be preferable to have a separate notes table.  One that could be used for more than just this one table.  I don't see it hindering performance as a notes table would not be accessed when the rules are used in circulation processes.  If there is a reason it would be better to have the notes in the table, great.  I am learning here, so would be interested in knowing if one method is better than another.

Hi Christopher, can you explain what you think the advantage of another table would be? Storing the notes with the data it belongs to makes sense to me on first glance.

(In reply to Katrin Fischer from comment #36)
> Hi Christopher, can you explain what you think the advantage of another
> table would be? Storing the notes with the data it belongs to makes sense to
> me on first glance.

The biggest benefit right off the bat is that you wouldn't have to add new columns here and there for the same thing.  If we think we'll need notes for preferences or other settings throughout, we'd end up adding new columns to all of these tables.  Why not create a table that could serve them all?  Create a table once.

If you apply the rules of database normalization just a notes column would not need a new table as it's a 1:1 relationship. It would be interesting to see if there are differences in performance or similar, but I think for a single column with little data there might not be much of a difference. I would not block this patch here as it follows an existing pattern.

+1 for DB normalization.

(In reply to Christopher Brannon from comment #37)
> The biggest benefit right off the bat is that you wouldn't have to add new
> columns here and there for the same thing.  If we think we'll need notes for
> preferences or other settings throughout, we'd end up adding new columns to
> all of these tables.  Why not create a table that could serve them all? 
> Create a table once.

We will not have foreign keys, it's better to have 1 column per table IMO.

Just wanted to jump in with my input: it makes sense to me that if I'm looking for notes associated to circ rules, I'm going to look at the circ rules table. 

Working on getting this ready for testing.

Created attachment 80790 [details] [review]
Bug 12365: (follow-up) Putting backticks in atomicupdate

Created attachment 80956 [details] [review]
Bug 12365: Notes for rules

This patch adds a note column to the circ and fine rules to act as a
description line for the rule.

To test:
1) Apply patch, update database (may have to also update schema)
2) Go to Admin -> Circ and fine rules
3) Scroll to the right, notice new Note column
4) Confirm it works to create a brand new rule with a Note
5) Confirm it works to modify an existing rule with a Note

Sponsored-by: Catalyst IT

Lee Jamison <ldjamison@marywood.edu> rescued this patch.
It is a clean re-build of attachments 61865 and 63902
to fix the merge conflict introduced during application of both
attachments.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: macon  lauren KohaCon17 <caballeromaricon@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 80957 [details] [review]
Bug 12365: (follow-up) Adding maxlength attribute and popover

Test that you can't add a note of more than 100 characters.
Test notes with rules, notes are hidden by 'View note' and the
popover correctly displays note.
View the note by hovering over the link
I've moved the Note column so the popover displays nicely and doesn't
cover the buttons. I've also changed the placement so that it doesn't
cover information in the row.

Update: Test that you can edit the note when clicking 'Edit' and that
behaviour is the same for editing all other fields

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 80958 [details] [review]
Bug 12365: (follow-up) Putting backticks in atomicupdate

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Hi Aleisha,

please make sure you always run the latest version of the QA tools from the gitlab repository on your patches:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
   FAIL	  filters
		missing_filter at line 125 (                                                                <a name="viewnote" data-toggle="popover" title="Note" data-content="[% rule.note %]" data-placement="top" data-trigger="hover">View note</a>)

I'd have fixed that, but there is another issue: The layout of the issuing rule table is broken with this patch. The buttons at the end of the table are misaligned. 
My entered note shows up in the last column labelled "Actions" as well, pushing the buttons into another non-existing column... (looking at it this will hopefully make more sense)

Created attachment 80997 [details] [review]
Bug 12365: (follow-up) Fixing broken circ rules table

Created attachment 80998 [details] [review]
Bug 12365: (follow-up) Fixing broken circ rules table

Adding filters to note to satisfy qa test tools

Follow-up is small enough - setting back to signed off.

We got a new column in the table now (daily holds limit) - please rebase on latest master!

Created attachment 81603 [details] [review]
Bug 12365: Notes for rules

This patch adds a note column to the circ and fine rules to act as a
description line for the rule.

To test:
1) Apply patch, update database (may have to also update schema)
2) Go to Admin -> Circ and fine rules
3) Scroll to the right, notice new Note column
4) Confirm it works to create a brand new rule with a Note
5) Confirm it works to modify an existing rule with a Note

Sponsored-by: Catalyst IT

Lee Jamison <ldjamison@marywood.edu> rescued this patch.
It is a clean re-build of attachments 61865 and 63902
to fix the merge conflict introduced during application of both
attachments.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: macon  lauren KohaCon17 <caballeromaricon@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 81604 [details] [review]
Bug 12365: (follow-up) Adding maxlength attribute and popover

Test that you can't add a note of more than 100 characters.
Test notes with rules, notes are hidden by 'View note' and the
popover correctly displays note.
View the note by hovering over the link
I've moved the Note column so the popover displays nicely and doesn't
cover the buttons. I've also changed the placement so that it doesn't
cover information in the row.

Update: Test that you can edit the note when clicking 'Edit' and that
behaviour is the same for editing all other fields

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 81605 [details] [review]
Bug 12365: (follow-up) Putting backticks in atomicupdate

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 81606 [details] [review]
Bug 12365: (follow-up) Fixing broken circ rules table

Adding filters to note to satisfy qa test tools

Rebased. Setting back to signed off

Created attachment 81647 [details] [review]
Bug 12365: Notes for rules

This patch adds a note column to the circ and fine rules to act as a
description line for the rule.

To test:
1) Apply patch, update database (may have to also update schema)
2) Go to Admin -> Circ and fine rules
3) Scroll to the right, notice new Note column
4) Confirm it works to create a brand new rule with a Note
5) Confirm it works to modify an existing rule with a Note

Sponsored-by: Catalyst IT

Lee Jamison <ldjamison@marywood.edu> rescued this patch.
It is a clean re-build of attachments 61865 and 63902
to fix the merge conflict introduced during application of both
attachments.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: macon  lauren KohaCon17 <caballeromaricon@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 81648 [details] [review]
Bug 12365: (follow-up) Adding maxlength attribute and popover

Test that you can't add a note of more than 100 characters.
Test notes with rules, notes are hidden by 'View note' and the
popover correctly displays note.
View the note by hovering over the link
I've moved the Note column so the popover displays nicely and doesn't
cover the buttons. I've also changed the placement so that it doesn't
cover information in the row.

Update: Test that you can edit the note when clicking 'Edit' and that
behaviour is the same for editing all other fields

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 81649 [details] [review]
Bug 12365: (follow-up) Putting backticks in atomicupdate

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 81650 [details] [review]
Bug 12365: (follow-up) Fixing broken circ rules table

Adding filters to note to satisfy qa test tools

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Thx for your patience and the quick rebase!

Awesome work all!

Pushed to master for 18.11

Created attachment 82081 [details] [review]
Bug 12365: Fix GetHardDueDate.t test

Enhancement, will not be backported to 18.05.x series.

Can we look to finishing off the removal of the almost exactly the same use case field mentioned in bug 21753 now we have this replacement?