It looks like changes to a patron's "extended patron attributes" are not logged, when BorrowersLog is on. They should probably be.
If we want to be really flexible, we could make logging a per-attribute setting.
Logging would be a good thing.
+1
Still relevant in 21.11 and still would be useful!
+1, this would be helpful
Created attachment 163957 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library
Created attachment 163958 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library
Created attachment 163959 [details] [review] Bug 26744: Only log if BorrowersLog syspref is enabled
This works as described so I will sign-off. Why not log repeatable attributes as well?
Created attachment 164034 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 164035 [details] [review] Bug 26744: Only log if BorrowersLog syspref is enabled Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
I actually don't know since I'm not the original author of this patch, but I guess there is sole technical reasons. My guess would be it's harder to figure out a hood way of representing the change, and especially tricky if you just want one log message when adding multiple instances of a repeatable field since the stores method would be called repeatedly for each existing and added change. If the logging was performed in the extended_attributes method it could be doable, but that might not catch all modifications and think there might be more patrons performing modification that would have to be handled as well. Can ask my collegue tomorrow about this.
(In reply to Lucas Gass from comment #9) > This works as described so I will sign-off. Why not log repeatable > attributes as well? I'm the one who wrote the original patch here. The way editing of extended attributes work is that they're all first deleted, then they're recreated from the form. Since all of the previous attributes are just deleted, I have no way that I can see to know which of the repeatable attributes that was changed into what. It there had been a more conventional way to edit the existing attributes this would've been easy.
Created attachment 164098 [details] [review] Bug 26744: Add support for repeatable and deleted attributes
I realised it actually was not too much trouble also logging changes to repeatable attributes (and deletions) if instead performing the logging in Koha::Patron->extended_attributes, so I revised the patch to do that instead.
I tested this in sandbox with three patron attributes: SSN (not repetable), HOLDID (not repetable) and AUTOTYPE (repetable). When I made a change to attribute the after value is recorded to the action logs but not the before value. The repetable AUTOTYPE attribute recorded changes only to one of the values. Deletion of SSN-value was not recorderd. It seems to me that this does not work as intended. Here are explained the actions that I took and how they showed in the action logs. Changed AUTOTYPE values from Palautusautomaatti -> Palautusautomaatti 2 AND Lainausautomaatti -> Lainausautomaatti2. The latter was not recorded to the action log. 04/19/2024 06:50 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti2" ], "before" : [] } Intranet Changed HOLDID value from 12345678 to 123456789 04/19/2024 06:50 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute HOLDID: { "after" : "123456789", "before" : "" } Intranet Changed SSN value from sotu123456 to sotu1234567 04/19/2024 06:50 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute SSN: { "after" : "sotu1234567", "before" : "" } Intranet Changed HOLDID value from 123456789 to 1234567890 04/19/2024 06:51 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute HOLDID: { "after" : "1234567890", "before" : "" } Intranet Changed SSN value from sotu1234567 to sotu123456 04/19/2024 06:51 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute SSN: { "after" : "sotu123456", "before" : "" } Intranet Changed AUTOTYPE values from Palautusautomaatti2 -> Palautusautomaatti AND Lainausautomaatti2 -> Lainausautomaatti. The latter was not recorded to the action log. 04/19/2024 06:51 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] } Intranet Deleted value from SSN and saved. The deletion was not recorded but there were actions logged for AUTOTYPE and HOLDID which were not changed any way before saving the patron. 04/19/2024 06:52 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] } Intranet 04/19/2024 06:52 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute HOLDID: { "after" : "1234567890", "before" : "" } Intranet Added value for SSN, did not change other attributes. Still there we lines for them. 04/19/2024 07:04 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] } Intranet 04/19/2024 07:04 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute SSN: { "after" : "sotu234567", "before" : "" } Intranet 04/19/2024 07:04 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute HOLDID: { "after" : "1234567890", "before" : "" } Intranet Cleared repeting AUTOTYPE Lainausautomaatti. No record of it but there were record of others which were not changed. 04/19/2024 07:04 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] } Intranet 04/19/2024 07:04 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute SSN: { "after" : "sotu234567", "before" : "" } Intranet 04/19/2024 07:04 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute HOLDID: { "after" : "1234567890", "before" : "" } Intranet Added back the cleared AUTOTYPE Lainausautomaatti. No record of it. 04/19/2024 07:08 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute SSN: { "after" : "sotu234567", "before" : "" } Intranet 04/19/2024 07:08 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute HOLDID: { "after" : "1234567890", "before" : "" } Intranet 04/19/2024 07:08 koha (51) Patrons Modify Joyce Gaines (22) Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] } Intranet
Created attachment 165212 [details] [review] Bug 26744: Don't delete extended attributes outside of method And fix bug where $attribute_changes was overwritten instead of assigned default value.
Created attachment 165213 [details] [review] Bug 26744: Add test for multiple values of repeatable fields
@Anneli Österman Thanks for the extensive testing, I should have added tests also for updates with multiple values. There was a line outside of the extended_attributes method that deleted the attributes in memberentry.pl that was not caught by the tests as the method is tested in isolation. But there was also a bug where the stashed repeatable attributes was overwritten where a default value should be assigned, both are now fixed and tests have been added to cover the latter case.
Created attachment 165301 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Created attachment 165302 [details] [review] Bug 26744: Only log if BorrowersLog syspref is enabled Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Created attachment 165303 [details] [review] Bug 26744: Add support for repeatable and deleted attributes Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Created attachment 165304 [details] [review] Bug 26744: Don't delete extended attributes outside of method And fix bug where $attribute_changes was overwritten instead of assigned default value. Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Created attachment 165305 [details] [review] Bug 26744: Add test for multiple values of repeatable fields Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Created attachment 177605 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Squashed (to simplify rabase and review) and rebasted against master. The changes in bug 35508 required some refactoring, and the changes in made there could be unified with the code here since both bugs deal with comparing changes for attributes.The comparison is now more granular and instead of replacing all attributes if anything changes the replacement is now on an attribute type level.
Will probably have to perform som more manual testing, and perhaps add some more tests, before totally confident everything works as it should. But feel free to review the code meanwhile.
Another improvement is that the comparison (that decides if attributes has changed) now resides in the extended_attributes method, and not outside in memberentry.pl, so the updated column will not be set if patron attributes are updated some where else using this method, when no value has changed.
Created attachment 177652 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Created attachment 178728 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Rebased against main.
Created attachment 178771 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Since it was just a rebase we can keep Clemens SO line here.
The only problem I now see here is that changes are not logged when I update attributes with the Batch patron modification tool.
Created attachment 178787 [details] [review] Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute
Created attachment 178788 [details] [review] Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute
(In reply to Lucas Gass (lukeg) from comment #34) > The only problem I now see here is that changes are not logged when I update > attributes with the Batch patron modification tool. Ok! This has now been fixed so that modborrowers.pl use the extended_attributes method instead and changes are logged. There is also a case in Koha::Patron->merge_with where patron attributes are modified by the add_extended_attribute method. The way it's used is not possible to replace it with the extended_attributes method, but I added logging also for add_extended_attribute, which is better really as it possibly cover other cases.
Created attachment 178789 [details] [review] Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute
Created attachment 178801 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Created attachment 178802 [details] [review] Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Small note, the string this puts into the info field isn't formatted exactly like edits of other parts of the patron record. For example, here's an edit to a name field: { "preferred_name" : { "after" : "Huey", "before" : "Hank" } } And an edit to an attribute: Patron attribute SHOW_BCODE: { "after" : "1", "before" : "" } Not a blocker, but worth noting for its impact on reports.
Created attachment 178803 [details] [review] Bug 26744: (QA follow up) Tidy
David, this is looking very nice. I tidied some things but you might want to look into pulling fresh ktd images and use the new pre-commit tidy stuff. See Bug 38664. You might face more QA challenges in the future without. I have one question here: Is List::Util's zip6 really necessary for comparison?
Created attachment 178817 [details] [review] Bug 26744: Cosolidate changes and change log format
(In reply to Andrew Fuerste-Henry from comment #41) > Small note, the string this puts into the info field isn't formatted exactly > like edits of other parts of the patron record. For example, here's an edit > to a name field: > { "preferred_name" : { "after" : "Huey", "before" : "Hank" } } > > And an edit to an attribute: > Patron attribute SHOW_BCODE: { "after" : "1", "before" : "" } > > Not a blocker, but worth noting for its impact on reports. Ok! I changed the log format to pure json, prepending "attribute." to the "field" name. Also saw that the normal field logging writes one log entry for all field changes, so fixed so this is done also for attribute changes.
(In reply to Lucas Gass (lukeg) from comment #43) > David, this is looking very nice. I tidied some things but you might want to > look into pulling fresh ktd images and use the new pre-commit tidy stuff. > See Bug 38664. You might face more QA challenges in the future without. > > I have one question here: > > Is List::Util's zip6 really necessary for comparison? It could of course be done in other ways, but I don't see why using zip6 would be a problem as it makes the code compact and concise.
(In reply to David Gustafsson from comment #45) > (In reply to Andrew Fuerste-Henry from comment #41) > > Small note, the string this puts into the info field isn't formatted exactly > > like edits of other parts of the patron record. For example, here's an edit > > to a name field: > > { "preferred_name" : { "after" : "Huey", "before" : "Hank" } } > > > > And an edit to an attribute: > > Patron attribute SHOW_BCODE: { "after" : "1", "before" : "" } > > > > Not a blocker, but worth noting for its impact on reports. > > Ok! I changed the log format to pure json, prepending "attribute." to the > "field" name. Also saw that the normal field logging writes one log entry > for all field changes, so fixed so this is done also for attribute changes. Thanks, David! I can confirm the syntax change looks good and multiple attribute changes all log as one entry. Is there some inherent reason attribute changes and edits to the regular borrower fields couldn't all go in one log line when performed together? If that's feasible, I may file a followup bug for it.
Created attachment 178839 [details] [review] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 178840 [details] [review] Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 178841 [details] [review] Bug 26744: (QA follow up) Tidy Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 178842 [details] [review] Bug 26744: Cosolidate changes and change log format Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Pushed for 25.05! Well done everyone, thank you!
(In reply to Andrew Fuerste-Henry from comment #47) > (In reply to David Gustafsson from comment #45) > > (In reply to Andrew Fuerste-Henry from comment #41) > > > Small note, the string this puts into the info field isn't formatted exactly > > > like edits of other parts of the patron record. For example, here's an edit > > > to a name field: > > > { "preferred_name" : { "after" : "Huey", "before" : "Hank" } } > > > > > > And an edit to an attribute: > > > Patron attribute SHOW_BCODE: { "after" : "1", "before" : "" } > > > > > > Not a blocker, but worth noting for its impact on reports. > > > > Ok! I changed the log format to pure json, prepending "attribute." to the > > "field" name. Also saw that the normal field logging writes one log entry > > for all field changes, so fixed so this is done also for attribute changes. > > Thanks, David! I can confirm the syntax change looks good and multiple > attribute changes all log as one entry. > Is there some inherent reason attribute changes and edits to the regular > borrower fields couldn't all go in one log line when performed together? If > that's feasible, I may file a followup bug for it. That's not practically possible I'm afraid, even though I can se how it makes sense from a user perspective. as code wise fields and attributes are very different things, with no common methods for updates/changes.
(In reply to David Gustafsson from comment #53) > That's not practically possible I'm afraid, even though I can se how it > makes sense from a user perspective. as code wise fields and attributes are > very different things, with no common methods for updates/changes. Thanks for confirming!
I suspect this change to be the cause of the following 2 failures: $ prove t/db_dependent/api/v1/patrons.t t/db_dependent/api/v1/patrons.t .. 3/7 # Failed test at t/db_dependent/api/v1/patrons.t line 1042. # got: 'zmEXaS_1HNKHibXWfmUaGo2s8GVHLodtQe5ulfn2oLO' # expected: 'pD7HHacPDh6DExQ7wl2XBBZNj2cmz3_F0_iSUhNAFEVY4Wv16OS55oN9aVWVjB' # Failed test at t/db_dependent/api/v1/patrons.t line 1042. # got: 'zmEXaS_1HNKHibXWfmUaGo2s8GVHLodtQe5ulfn2oLO' # expected: 'Z1dK8ALzVvAz3pjOgpVOzM9cVizAnWw7Fy8rB9eTfuLvCRCdhzqpgFcMN3' # Failed test at t/db_dependent/api/v1/patrons.t line 1042. # got: 'Z1dK8ALzVvAz3pjOgpVOzM9cVizAnWw7Fy8rB9eTfuLvCRCdhzqpgFcMN3' # expected: 'zmEXaS_1HNKHibXWfmUaGo2s8GVHLodtQe5ulfn2oLO' # Failed test at t/db_dependent/api/v1/patrons.t line 1042. # got: 'pD7HHacPDh6DExQ7wl2XBBZNj2cmz3_F0_iSUhNAFEVY4Wv16OS55oN9aVWVjB' # expected: 'zmEXaS_1HNKHibXWfmUaGo2s8GVHLodtQe5ulfn2oLO' # Looks like you failed 4 tests of 26. # Failed test 'extended_attributes tests' # at t/db_dependent/api/v1/patrons.t line 1045. # Looks like you failed 1 test of 44. # Failed test 'librarian access tests' # at t/db_dependent/api/v1/patrons.t line 1048. # Looks like you failed 1 test of 2. t/db_dependent/api/v1/patrons.t .. 4/7 # Failed test 'update() tests' # at t/db_dependent/api/v1/patrons.t line 1049. t/db_dependent/api/v1/patrons.t .. 7/7 # Looks like you failed 1 test of 7. t/db_dependent/api/v1/patrons.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/7 subtests $ prove t/db_dependent/api/v1/patrons_extended_attributes.t t/db_dependent/api/v1/patrons_extended_attributes.t .. 2/5 # Failed test 'exact match for JSON Pointer "/0/type"' # at t/db_dependent/api/v1/patrons_extended_attributes.t line 358. # got: 'KDtH_WsRVKD5KY5R9N_VTJTJkZ6xfGKtqmD' # expected: 'DDqUW_ML8Mz3FfSWsx2kOUoywRGNwi2UuGvNjhVKyqkoKexxB_0aidSXowl6' # Failed test 'exact match for JSON Pointer "/0/value"' # at t/db_dependent/api/v1/patrons_extended_attributes.t line 358. # got: 'updated_mandatory' # expected: 'updated_repeatable_1' # Failed test 'exact match for JSON Pointer "/1/value"' # at t/db_dependent/api/v1/patrons_extended_attributes.t line 358. # got: 'updated_repeatable_1' # expected: 'updated_repeatable_2' # Failed test 'exact match for JSON Pointer "/2/value"' # at t/db_dependent/api/v1/patrons_extended_attributes.t line 358. # got: 'updated_repeatable_2' # expected: 'updated_repeatable_3' # Failed test 'exact match for JSON Pointer "/3/type"' # at t/db_dependent/api/v1/patrons_extended_attributes.t line 358. # got: 'DDqUW_ML8Mz3FfSWsx2kOUoywRGNwi2UuGvNjhVKyqkoKexxB_0aidSXowl6' # expected: 'KDtH_WsRVKD5KY5R9N_VTJTJkZ6xfGKtqmD' # Failed test 'exact match for JSON Pointer "/3/value"' # at t/db_dependent/api/v1/patrons_extended_attributes.t line 358. # got: 'updated_repeatable_3' # expected: 'updated_mandatory' # Looks like you failed 6 tests of 29. t/db_dependent/api/v1/patrons_extended_attributes.t .. 3/5 # Failed test 'overwrite() tests' # at t/db_dependent/api/v1/patrons_extended_attributes.t line 361. They could eventually pass as it's a random failure, but it's failing almost every run locally. The problem is the following: We send some attributes to the PUT route, but return the attributes in a different order. My guess is that the culprit is this line: Koha/Patron.pm 2308 # Sort new attributes by code 2309 @new_attributes = sort { $a->attribute cmp $b->attribute } @new_attributes; Please fix ASAP.
I'm looking into it. I made sure the attributes is now saved in the order they where passed in, but also encountering some other failed tests. Not sure what is happening, but will sort it out.
Created attachment 179284 [details] [review] Bug 26744i: (follow up) Preserve order when adding patron attributes
Created attachment 179285 [details] [review] Bug 26744: (follow up) Preserve order when adding patron attributes
I'm not sure about now patches for bugs that has already been push are handled, should I have created a new bug for this? Posting the patch her meanwhile. It was the insertion order that was the issue, but I initially put the code that checks for required types before storing the attributes, while it previously was the other way around, so tests failed since some errors that exceptions where thrown in a different order, so when there where many validation errors a different one showed up in the tests. When preserving the same order as before all tests pass.
Created attachment 179286 [details] [review] Bug 26744: (follow up) Preserve order when adding patron attributes
Pretty sure should be handled as a new bug, so created Bug 39334.
(In reply to David Gustafsson from comment #61) > Pretty sure should be handled as a new bug, so created Bug 39334. Hi David, sorry for being a little late :( Follow-up patches on bugs not yet in a released version are fine. If I set "additional_work_needed" and don't aks for a new bug it's usually fine.
(In reply to David Gustafsson from comment #46) > (In reply to Lucas Gass (lukeg) from comment #43) > > Is List::Util's zip6 really necessary for comparison? > > It could of course be done in other ways, but I don't see why using zip6 > would be a problem as it makes the code compact and concise. It looks like zip6 became available in List::MoreUtils 0.423 (from August 2017). Older versions of Debian/Ubuntu used version 0.416 so they wouldn't have been able to support this change. Fortunately, all the currently supported versions of Debian/Ubuntu have version 0.430 so it's all good. That said, I'll open a follow-up report to bump up the required version of List::MoreUtils in the cpanfile.