log user creation timestamp #3416
Conversation
There was a problem hiding this comment.
@sjanssen2, this is great; really appreciate the contribution.
| @@ -1891,7 +1891,8 @@ CREATE TABLE qiita.qiita_user ( | |||
| receive_processing_job_emails boolean DEFAULT false, | |||
| social_orcid character varying DEFAULT NULL, | |||
| social_researchgate character varying DEFAULT NULL, | |||
| social_googlescholar character varying DEFAULT NULL | |||
| social_googlescholar character varying DEFAULT NULL, | |||
There was a problem hiding this comment.
A few requests:
- Could you undo the changes on this file and add them as a SQL patch? Do not worry about changing the DBSchema, I'll do that with this and other minor changes it has happened before the release. As a reminder, the reason those changes were made in this file was because I merged all changes into a single file a few PRs ago.
- Could you make the default of creation_timestamp NOW()? That way it will be controlled by the database during creation.
Thank you.
There was a problem hiding this comment.
- in fact, I think you should use 92.sql which is the patch for this release; fine to do that on the other PR if you want me to merge this one first
- 👍🏽
There was a problem hiding this comment.
@1: I moved the alterations to 92.sql. I didn't know that you collect changes for a release. However, tests fail. I might overlook something, but I have the impression that no patch is currently applied in the github actions?!
There was a problem hiding this comment.
2. Could you make the default of creation_timestamp NOW()? That way it will be controlled by the database during creation.
As a follow up: I've just made the DB changes in my "productive" instance and found that all existing users (which did not have the creation_timestamp column of cause) now have the timestamp of DB change as their creation_timestamp information, even though they existed for many month in the past.
Is that really the behaviour we want to see in a productive Qiita, @antgonza ?
qiita_db/user.py
Outdated
|
|
||
| # log timestamp of user creation | ||
| sql = """UPDATE qiita.{0} | ||
| SET creation_timestamp = NOW() | ||
| WHERE email = %s""".format(cls._table) | ||
| qdb.sql_connection.perform_as_transaction(sql, [email]) | ||
|
|
||
| # Add system messages to user |
There was a problem hiding this comment.
If the default in the database is now(), then this is not required.
There was a problem hiding this comment.
I like removing this code a lot :-) I was not sure how SQL will deal with old entries that have None in this column.
| @@ -75,7 +75,8 @@ def setUp(self): | |||
| 'receive_processing_job_emails': True, | |||
There was a problem hiding this comment.
The issue with this test and my suggestion about using now() in the database is that there is going to be a small difference on the tests so they will need to be adjusted.
There was a problem hiding this comment.
I tried to account for these unpredictable time differences with your "before < obs < after" assertions, e.g. here https://github.com/jlab/qiita/blob/1488be79e9468e240ea318ae8920fd6559030307/qiita_db/test/test_user.py#L170-L198
| @@ -50,7 +50,7 @@ INSERT INTO qiita.user_level VALUES (7, 'wet-lab admin', 'Can access the private | |||
| -- Data for Name: qiita_user; Type: TABLE DATA; Schema: qiita; Owner: antoniog | |||
| -- | |||
|
|
|||
| INSERT INTO qiita.qiita_user VALUES ('[email protected]', 4, '$2a$12$gnUi8Qg.0tvW243v889BhOBhWLIHyIJjjgaG6dxuRJkUM8nXG9Efe', 'Dude', 'Nowhere University', '123 fake st, Apt 0, Faketown, CO 80302', '111-222-3344', NULL, NULL, NULL, false, '0000-0002-0975-9019', 'Rob-Knight', '_e3QL94AAAAJ'); | |||
| INSERT INTO qiita.qiita_user VALUES ('[email protected]', 4, '$2a$12$gnUi8Qg.0tvW243v889BhOBhWLIHyIJjjgaG6dxuRJkUM8nXG9Efe', 'Dude', 'Nowhere University', '123 fake st, Apt 0, Faketown, CO 80302', '111-222-3344', NULL, NULL, NULL, false, '0000-0002-0975-9019', 'Rob-Knight', '_e3QL94AAAAJ', '2015-12-03 13:52:42.751331-07'); | |||
There was a problem hiding this comment.
Based on the failure, you need to remove this change too.
There was a problem hiding this comment.
hm, I thought that the new patches should be free of test data. I've moved this addition of the creation_timestamp info into the 92.sql patch. Let's wait for the tests.
Addressing #3378 this PR will use SQL's
NOW()function to log creation of a new user account (validated or not). This is handy for:a) reconstructing user growth
b) pruning accounts that have been created but not validated for X days
I've also added this information to the user's profile page, if the value is not None:
