Bug 23486 - Date last seen should be set upon registration
Summary: Date last seen should be set upon registration
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-21 18:50 UTC by Nick Clemens
Modified: 2024-02-13 14:05 UTC (History)
12 users (show)

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


Attachments
Bug 23486: Set lastseen to default to current timestamp (2.98 KB, patch)
2022-03-14 14:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: DO NOT PUSH Schema update (1.46 KB, patch)
2022-03-14 14:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: DO NOT PUSH Schema update (1.46 KB, patch)
2022-03-25 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: DO NOT PUSH Schema update (1.46 KB, patch)
2022-05-03 13:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: (follow-up) Correct Schema (1.15 KB, patch)
2022-05-03 15:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: Set lastseen to default to current timestamp (3.12 KB, patch)
2022-05-03 15:40 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23486: DO NOT PUSH Schema update (1.61 KB, patch)
2022-05-03 15:40 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23486: (follow-up) Correct Schema (1.29 KB, patch)
2022-05-03 15:40 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23486: Set lastseen to default to current timestamp (3.12 KB, patch)
2022-05-13 10:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: DO NOT PUSH Schema update (1.50 KB, patch)
2022-05-13 10:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: Set lastseen to default to current timestamp (3.44 KB, patch)
2023-03-27 14:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: DO NOT PUSH - Schema Updates (1.51 KB, patch)
2023-03-27 14:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: Set lastseen to default to current timestamp (3.44 KB, patch)
2023-04-07 13:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: DO NOT PUSH - Schema updates (1.74 KB, patch)
2023-04-07 13:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23486: Set lastseen to default to current timestamp (3.49 KB, patch)
2023-04-23 10:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 23486: DO NOT PUSH - Schema updates (1.79 KB, patch)
2023-04-23 10:15 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-08-21 18:50:26 UTC
When a patron is registered, via staff or self registration we should fill in the 'lastseen'  field
Comment 1 Nick Clemens 2019-08-21 18:53:32 UTC
Also on issuing an item?
Comment 2 Christofer Zorn 2022-01-12 13:45:43 UTC
I'd like to bump this change. We were under reporting our active users because our report was counting when the lastseen date was between two dates, however we had 3000+ active users in the system missed because of the null.

If the lastseen date is updated on a SIP connection or CKO, it certainly should be updated when the patron registers.

Thanks!
Comment 3 Nick Clemens 2022-03-14 14:25:33 UTC
Created attachment 131657 [details] [review]
Bug 23486: Set lastseen to default to current timestamp

Generally, a patron is not registered without some action from the patron
Defaulting the lastseen upon creation seems a reasonable way to set the initial value

TO test:
Apply patches
Update database
Create a patron
Check their lastseen date
Comment 4 Nick Clemens 2022-03-14 14:25:37 UTC
Created attachment 131658 [details] [review]
Bug 23486: DO NOT PUSH Schema update
Comment 5 Séverine Queune 2022-03-23 15:09:01 UTC
Hi Nick !

I get this error message when I apply the patch in a devbox :

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23486: Set lastseen to default to current timestamp
Applying: Bug 23486: DO NOT PUSH Schema update
Using index info to reconstruct a base tree...
M	Koha/Schema/Result/Borrower.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/Schema/Result/Borrower.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Borrower.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 23486: DO NOT PUSH Schema update
hint: Use 'git am --show-current-patch=diff' to see the failed 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-23486-DO-NOT-PUSH-Schema-update-1WltcW.patch

I'll be happy to test it when it's fixed, thanks !
Séverine
Comment 6 Nick Clemens 2022-03-25 11:44:07 UTC
Created attachment 132222 [details] [review]
Bug 23486: DO NOT PUSH Schema update
Comment 7 Séverine Queune 2022-05-03 13:36:54 UTC
Sorry Nick, I wasn't quick enough to test the first rebase, I have the same error in my DevBox :/

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23486: Set lastseen to default to current timestamp
Applying: Bug 23486: DO NOT PUSH Schema update
Using index info to reconstruct a base tree...
M	Koha/Schema/Result/Borrower.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/Schema/Result/Borrower.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Borrower.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 23486: DO NOT PUSH Schema update
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Comment 8 Nick Clemens 2022-05-03 13:49:23 UTC
Created attachment 134540 [details] [review]
Bug 23486: DO NOT PUSH Schema update
Comment 9 Nick Clemens 2022-05-03 15:20:00 UTC
Created attachment 134547 [details] [review]
Bug 23486: (follow-up) Correct Schema

It seems the generated schema handles timestamp incorrectly - this will allow for testing
Comment 10 Biblibre Sandboxes 2022-05-03 15:40:16 UTC
Created attachment 134549 [details] [review]
Bug 23486: Set lastseen to default to current timestamp

Generally, a patron is not registered without some action from the patron
Defaulting the lastseen upon creation seems a reasonable way to set the initial value

TO test:
Apply patches
Update database
Create a patron
Check their lastseen date

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 11 Biblibre Sandboxes 2022-05-03 15:40:22 UTC
Created attachment 134550 [details] [review]
Bug 23486: DO NOT PUSH Schema update

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 12 Biblibre Sandboxes 2022-05-03 15:40:27 UTC
Created attachment 134551 [details] [review]
Bug 23486: (follow-up) Correct Schema

It seems the generated schema handles timestamp incorrectly - this will allow for testing

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 13 Séverine Queune 2022-05-03 15:41:20 UTC
Works nice, well done Nick !!
Comment 14 Marcel de Rooy 2022-05-13 09:41:24 UTC
Please fix the DBIx stuff and submit it in one patch
Comment 15 Nick Clemens 2022-05-13 10:25:19 UTC
Created attachment 134963 [details] [review]
Bug 23486: Set lastseen to default to current timestamp

Generally, a patron is not registered without some action from the patron
Defaulting the lastseen upon creation seems a reasonable way to set the initial value

TO test:
Apply patches
Update database
Create a patron
Check their lastseen date

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 16 Nick Clemens 2022-05-13 10:25:23 UTC
Created attachment 134964 [details] [review]
Bug 23486: DO NOT PUSH Schema update

RM note - manual changes needed to generate
Comment 17 Martin Renvoize 2022-05-14 08:15:47 UTC
There's something really odd going on here.. I don't understand why current_timestamp is coming out wrong.. though I do have vague recollections of such an issue in the past.

However.. I also query.. should the column be updated to 'NOT NULL' too if we're giving it a DEFAULT?

Also.. sha1 issues trying to apply the follow-up..
Comment 18 Nick Clemens 2022-05-16 10:44:48 UTC
I think if we are going to make it 'NOT NULL' we are going to need to add a DB update, and set any currently null to the dateenrolled. That was we don't 'see' a number of patrons at upgrade
Comment 19 Nick Clemens 2023-03-27 14:27:09 UTC
Created attachment 148780 [details] [review]
Bug 23486: Set lastseen to default to current timestamp

Generally, a patron is not registered without some action from the patron
Defaulting the lastseen upon creation seems a reasonable way to set the initial value.

This patch also makes the column NOT NULL to avoid any future data issues. To handle any
currently NULL, we either set them to dateenrolled, or updated_on - the latter is already
NOT NULL so is the bast fallback we have (assuming patron was also seen when they were updated)

TO test:
Apply patches
Update database
Create a patron
Check their lastseen date
Comment 20 Nick Clemens 2023-03-27 14:27:12 UTC
Created attachment 148781 [details] [review]
Bug 23486: DO NOT PUSH - Schema Updates
Comment 21 Laura Escamilla 2023-04-07 12:39:44 UTC
After applying the patch I was unable to create a new patron. I received the message, "The following fields are wrong. Please fix them." even though all the required fields were completed and no fields were being marked as incorrect. Even after filling in all the optional fields the patron could still not be created.
Comment 22 Nick Clemens 2023-04-07 13:09:16 UTC
Created attachment 149273 [details] [review]
Bug 23486: Set lastseen to default to current timestamp

Generally, a patron is not registered without some action from the patron
Defaulting the lastseen upon creation seems a reasonable way to set the initial value.

This patch also makes the column NOT NULL to avoid any future data issues. To handle any
currently NULL, we either set them to dateenrolled, or updated_on - the latter is already
NOT NULL so is the bast fallback we have (assuming patron was also seen when they were updated)

TO test:
Apply patches
Update database
Create a patron
Check their lastseen date
Comment 23 Nick Clemens 2023-04-07 13:09:19 UTC
Created attachment 149274 [details] [review]
Bug 23486: DO NOT PUSH - Schema updates

Note that the fields must be manually updated after generation
as generated syntax us invalid:
default_value: 'current_timestamp()'

First occurence:
default_value: 'current_timestamp'

Second occurence:
default_value => \"current_timestamp",
Comment 24 David Nind 2023-04-23 10:15:20 UTC
Created attachment 150119 [details] [review]
Bug 23486: Set lastseen to default to current timestamp

Generally, a patron is not registered without some action from the patron
Defaulting the lastseen upon creation seems a reasonable way to set the initial value.

This patch also makes the column NOT NULL to avoid any future data issues. To handle any
currently NULL, we either set them to dateenrolled, or updated_on - the latter is already
NOT NULL so is the bast fallback we have (assuming patron was also seen when they were updated)

TO test:
Apply patches
Update database
Create a patron
Check their lastseen date

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2023-04-23 10:15:24 UTC
Created attachment 150120 [details] [review]
Bug 23486: DO NOT PUSH - Schema updates

Note that the fields must be manually updated after generation
as generated syntax us invalid:
default_value: 'current_timestamp()'

First occurence:
default_value: 'current_timestamp'

Second occurence:
default_value => \"current_timestamp",

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2023-04-23 10:18:28 UTC
Testing notes (using KTD):

1. To check the lastseen field in the database:
   - koha-mysql kohadev
   - SQL query (where XXX = borrower number for newly created patron): select * from borrowers where borrowernumber = XXX;
Comment 27 Katrin Fischer 2023-04-23 10:36:35 UTC
"Generally, a patron is not registered without some action from the patron
Defaulting the lastseen upon creation seems a reasonable way to set the initial value"

I believe this is a wrong assumption. It works for self registrations and manual registration, but not for patron imports.

Often an institution will import all their users so they can log into the OPAC when they need to, but a lot of them never will. For this use case setting the date and assuming them as active won't be correct. This is also true when migrating data using the patron import tool, also common.

I think we cannot handle that on database level for this reason, we need to handle the different types of registration somehow.

Also, right now documentation would be wrong:
last time a patron has been seen (connected at the OPAC or staff interface)
Comment 28 Martin Renvoize 2023-04-24 10:08:05 UTC
I agree with Katrin here I'm afraid.
Comment 29 Katrin Fischer 2023-12-08 21:17:43 UTC
We could add patron creation to the list of actions in  TrackLastPatronActivityTriggers to resolve this.
Comment 30 Inkeri Hakulinen 2024-02-13 14:05:22 UTC
I think that Katrin's suggestion is good. It lets every library then decide, do they want adding a patron to set a date to "lastseen" or not.