Created attachment 159838 [details] [review]
Bug 35508: Update borrowers.updated_on when modifying a patron's attribute

Currently, if you have patron attributes and modify the values in a patron's account, the patron's updated_on date is not updated. This patch makes the "Updated on" change when a patron attribute is updated.

To test:

1. Create a patron attribute type
   1.1. Go to Administration > Patron attribute types
   1.2. Click New patron attribute
   1.3. Fill out the code and description
   1.4. Click Save

2. Edit a patron (normal)
   2.1. Go to Patrons and find a patron account
   2.2. Click Edit
   2.3. Change a regular field (e.g. Middle name)
   2.4. Click Save

--> Notice the "Updated on" date in the left column has been updated to now

3. Edit a patron attribute
   3.1. In another patron account*, click Edit
   3.2. Change the value of an attribute
   3.3. Click Save

--> Notice the "Updated on" date did not change

4. Apply the patch
   4.1 Repeat step 3.1, 3.2, 3.3

--> Notice the "Updated on" date has now changed

Created attachment 159895 [details] [review]
Bug 35508: Update borrowers.updated_on when modifying a patron's attribute

Currently, if you have patron attributes and modify the values in a patron's account, the patron's updated_on date is not updated. This patch makes the "Updated on" change when a patron attribute is updated.

To test:

1. Create a patron attribute type
   1.1. Go to Administration > Patron attribute types
   1.2. Click New patron attribute
   1.3. Fill out the code and description
   1.4. Click Save

2. Edit a patron (normal)
   2.1. Go to Patrons and find a patron account
   2.2. Click Edit
   2.3. Change a regular field (e.g. Middle name)
   2.4. Click Save

--> Notice the "Updated on" date in the left column has been updated to now

3. Edit a patron attribute
   3.1. In another patron account*, click Edit
   3.2. Change the value of an attribute
   3.3. Click Save

--> Notice the "Updated on" date did not change

4. Apply the patch
   4.1 Repeat step 3.1, 3.2, 3.3

--> Notice the "Updated on" date has now changed

Signed-off-by: Esther <esther@bywatersolutions.com>

I'm not sure about this. That columns is really meant to indicate the last time this particular row of this particular table was altered. What do you think of adding an "attributes_updated_on" column to the borrowers table for this?

Interesting.  But after thinking about it, I do not see it really as a problem.  updated_on is meant to tell when a patron was modified, imo, not tell us that a physical alignment on the hard drive was modified.  Attributes are just extensions of the borrower table. Adding a field would make some reports really convoluted, and unsortable.

It IS a solution, but doesn't seem optimal to me.  Adding unnecessary complexity (imo).

I understand the proposal and the motivation. I've added this topic to next dev meeting's agenda [1] for broader discussion.


[1] https://wiki.koha-community.org/wiki/Development_IRC_meeting_17_April_2024

Just pasting here what I said in the chat, to provide context

The need behind this is that we have a client who is linking their patron account to an external system. We added a lot of patron attributes for them because it's a special library and they need specific information. They need their other system to be "triggered" when a patron is modified in Koha, either normal patron fields or patron attributes

Also I think that end users don't see a difference between normal patron fields and patron attributes. We show the updated_on date in the left-hand column, so I think it would be logical (again, from end user pov) that this date be updated when we update a patron attribute.

The general consensus at the developers meeting was to move ahead with this!

Created attachment 165012 [details] [review]
Bug 35508: Update borrowers.updated_on when modifying a patron's attribute

Currently, if you have patron attributes and modify the values in a patron's account, the patron's updated_on date is not updated. This patch makes the "Updated on" change when a patron attribute is updated.

To test:

1. Create a patron attribute type
   1.1. Go to Administration > Patron attribute types
   1.2. Click New patron attribute
   1.3. Fill out the code and description
   1.4. Click Save

2. Edit a patron (normal)
   2.1. Go to Patrons and find a patron account
   2.2. Click Edit
   2.3. Change a regular field (e.g. Middle name)
   2.4. Click Save

--> Notice the "Updated on" date in the left column has been updated to now

3. Edit a patron attribute
   3.1. In another patron account*, click Edit
   3.2. Change the value of an attribute
   3.3. Click Save

--> Notice the "Updated on" date did not change

4. Apply the patch
   4.1 Repeat step 3.1, 3.2, 3.3

--> Notice the "Updated on" date has now changed

Signed-off-by: Esther <esther@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 165013 [details] [review]
Bug 35508: Move logic to Koha::Patron::Attribute::store()

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

(In reply to Kyle M Hall from comment #8)
> The general consensus at the developers meeting was to move ahead with this!

Great example for how we can use the meetings/Mattermost to resolve questions like this. :) I agree that we should log changes to patron attributes as changes to the patron, but I believe this patches currently have an unintended side-effect:

Pre-patch:
* Edit a patron record, save without changes: updated_on is unchanged.

With these patches:
* Edit a patron record, save without any changed: updated_on is changed.

I know we just had some similar issues on bug 26744. Maybe some of that could be useful here.

Created attachment 166640 [details] [review]
Bug 35508: Update borrowers.updated_on when modifying a patron's attribute

Currently, if you have patron attributes and modify the values in a patron's account, the patron's updated_on date is not updated. This patch makes the "Updated on" change when a patron attribute is updated.

To test:

1. Create a patron attribute type
   1.1. Go to Administration > Patron attribute types
   1.2. Click New patron attribute
   1.3. Fill out the code and description
   1.4. Click Save

2. Edit a patron (normal)
   2.1. Go to Patrons and find a patron account
   2.2. Click Edit
   2.3. Change a regular field (e.g. Middle name)
   2.4. Click Save

--> Notice the "Updated on" date in the left column has been updated to now

3. Edit a patron attribute
   3.1. In another patron account*, click Edit
   3.2. Change the value of an attribute
   3.3. Click Save

--> Notice the "Updated on" date did not change

4. Apply the patch
   4.1 Repeat step 3.1, 3.2, 3.3

--> Notice the "Updated on" date has now changed

Signed-off-by: Esther <esther@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 166641 [details] [review]
Bug 35508: Move logic to Koha::Patron::Attribute::store()

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 167229 [details] [review]
Bug 35508: Don't update patron attributes if new list of attributes matches list of existing attributes

I can no longer qa this. I think this latest followup is a bridge too far for that!

Created attachment 172946 [details] [review]
Bug 35508: Update borrowers.updated_on when modifying a patron's attribute

Currently, if you have patron attributes and modify the values in a patron's account, the patron's updated_on date is not updated. This patch makes the "Updated on" change when a patron attribute is updated.

To test:

1. Create a patron attribute type
   1.1. Go to Administration > Patron attribute types
   1.2. Click New patron attribute
   1.3. Fill out the code and description
   1.4. Click Save

2. Edit a patron (normal)
   2.1. Go to Patrons and find a patron account
   2.2. Click Edit
   2.3. Change a regular field (e.g. Middle name)
   2.4. Click Save

--> Notice the "Updated on" date in the left column has been updated to now

3. Edit a patron attribute
   3.1. In another patron account*, click Edit
   3.2. Change the value of an attribute
   3.3. Click Save

--> Notice the "Updated on" date did not change

4. Apply the patch
   4.1 Repeat step 3.1, 3.2, 3.3

--> Notice the "Updated on" date has now changed

Signed-off-by: Esther <esther@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 172947 [details] [review]
Bug 35508: Move logic to Koha::Patron::Attribute::store()

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 172948 [details] [review]
Bug 35508: Don't update patron attributes if new list of attributes matches list of existing attributes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 172949 [details] [review]
Bug 35508: (QA follow-up) Fix qa script issues

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Wondering:

Koha/Patron/Attribute.pm

+    C4::Context->dbh->do(
+        "UPDATE borrowers SET updated_on = NOW() WHERE borrowernumber = ?", undef,
+        $self->borrowernumber
+    );
+

Should this really be SQL here?

Pushed for 24.11!

Well done everyone, thank you!

(In reply to Katrin Fischer from comment #20)
> Wondering:
> 
> Koha/Patron/Attribute.pm
> 
> +    C4::Context->dbh->do(
> +        "UPDATE borrowers SET updated_on = NOW() WHERE borrowernumber = ?",
> undef,
> +        $self->borrowernumber
> +    );
> +
> 
> Should this really be SQL here?

This is one of those cases where direct SQL is definitely the best solution. If we were to use DBIC we would have to perform at least one lookup, if not two.

We could in theory pass the calling object ( i.e. the borrower ) into the accessor method on construction to reduce the needed lookups ( basically pass the borrower object into new and have it cached in $self for a 'patron' method to return without a lookup ). This is more of an architectural issue and is a bit outside the scope but I've be happy to work on it if you file a followup bug and assign it to me!

I am happy with the explanation. If you think it's worth keeping in mind/fixing later, please file the bug.

Release notes added

Backported to 24.05.06x for upcoming 24.05.06