Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

log user creation timestamp #3416

Merged
merged 12 commits into from
Jun 20, 2024
2 changes: 1 addition & 1 deletion qiita_db/support_files/populate_test_db.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the failure, you need to remove this change too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

INSERT INTO qiita.qiita_user VALUES ('[email protected]', 4, '$2a$12$gnUi8Qg.0tvW243v889BhOBhWLIHyIJjjgaG6dxuRJkUM8nXG9Efe', 'Shared', 'Nowhere University', '123 fake st, Apt 0, Faketown, CO 80302', '111-222-3344', NULL, NULL, NULL, false);
INSERT INTO qiita.qiita_user VALUES ('[email protected]', 1, '$2a$12$gnUi8Qg.0tvW243v889BhOBhWLIHyIJjjgaG6dxuRJkUM8nXG9Efe', 'Admin', 'Owner University', '312 noname st, Apt K, Nonexistantown, CO 80302', '222-444-6789', NULL, NULL, NULL, false);
INSERT INTO qiita.qiita_user VALUES ('[email protected]', 4, '$2a$12$gnUi8Qg.0tvW243v889BhOBhWLIHyIJjjgaG6dxuRJkUM8nXG9Efe', 'Demo', 'Qiita Dev', '1345 Colorado Avenue', '303-492-1984', NULL, NULL, NULL, false);
Expand Down
10 changes: 9 additions & 1 deletion qiita_db/support_files/qiita-db-unpatched.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few requests:

  1. 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.
  2. Could you make the default of creation_timestamp NOW()? That way it will be controlled by the database during creation.

Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1 oh, I thought the patching mechanism is dead as I didn't see patch files anymore :-) I've moved the change into 93.sql. Changes are much cleaner this way.
@2 love to :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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
  2. 👍🏽

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine.

creation_timestamp timestamp without time zone DEFAULT NULL
);


Expand Down Expand Up @@ -1931,6 +1932,13 @@ COMMENT ON COLUMN qiita.qiita_user.pass_reset_code IS 'Randomly generated code f
COMMENT ON COLUMN qiita.qiita_user.pass_reset_timestamp IS 'Time the reset code was generated';


--
-- Name: COLUMN qiita_user.creation_timestamp; Type: COMMENT; Schema: qiita
--

COMMENT ON COLUMN qiita.qiita_user.creation_timestamp IS 'The date the user account was created';


--
-- Name: reference; Type: TABLE; Schema: qiita
--
Expand Down
38 changes: 31 additions & 7 deletions qiita_db/test/test_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ def setUp(self):
'receive_processing_job_emails': True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'social_orcid': None,
'social_researchgate': None,
'social_googlescholar': None
'social_googlescholar': None,
'creation_timestamp': None
}

def tearDown(self):
Expand All @@ -88,7 +89,22 @@ def test_instantiate_unknown_user(self):
with self.assertRaises(qdb.exceptions.QiitaDBUnknownIDError):
qdb.user.User('[email protected]')

def _check_correct_info(self, obs, exp):
def _check_correct_info(self, obs, exp, ts_before=None):
"""Compares info dict of user with special handling of specific keys.

Parameters
----------
obs : dict
Observed user info dictionary.
exp : dict
Expected user info dictionary.
ts_before : datetime.datetime or None
User.create records the creation timestamp through SQL's NOW().
Since it is set by the database to the microsecond, we can't
predict it a priori and therefore simply record timestamp before
execution of user.create() and compare the relation.
The DB creation_timestamp column is optional, i.e. can be None.
"""
self.assertEqual(set(exp.keys()), set(obs.keys()))
for key in exp:
# user_verify_code and password seed randomly generated so just
Expand All @@ -97,10 +113,14 @@ def _check_correct_info(self, obs, exp):
self.assertEqual(len(obs[key]), 20)
elif key == "password":
self.assertEqual(len(obs[key]), 60)
elif key == "creation_timestamp":
self.assertTrue(((exp[key] is None) and (obs[key] is None))
or (ts_before <= exp[key]))
else:
self.assertEqual(obs[key], exp[key])

def test_create_user(self):
before = datetime.now()
user = qdb.user.User.create('[email protected]', 'password')

# adding a couple of messages
Expand Down Expand Up @@ -131,8 +151,9 @@ def test_create_user(self):
'email': '[email protected]',
'social_orcid': None,
'social_researchgate': None,
'social_googlescholar': None}
self._check_correct_info(obs, exp)
'social_googlescholar': None,
'creation_timestamp': datetime.now()}
self._check_correct_info(obs, exp, before)

# Make sure new system messages are linked to user
sql = """SELECT message_id FROM qiita.message_user
Expand All @@ -146,6 +167,7 @@ def test_create_user(self):
qdb.util.clear_system_messages()

def test_create_user_info(self):
before = datetime.now()
user = qdb.user.User.create('[email protected]', 'password',
self.userinfo)
self.assertEqual(user.id, '[email protected]')
Expand All @@ -171,8 +193,9 @@ def test_create_user_info(self):
'email': '[email protected]',
'social_orcid': None,
'social_researchgate': None,
'social_googlescholar': None}
self._check_correct_info(obs, exp)
'social_googlescholar': None,
'creation_timestamp': datetime.now()}
self._check_correct_info(obs, exp, before)

def test_create_user_column_not_allowed(self):
self.userinfo["email"] = "FAIL"
Expand Down Expand Up @@ -241,7 +264,8 @@ def test_get_info(self):
'phone': '222-444-6789',
'social_orcid': None,
'social_researchgate': None,
'social_googlescholar': None
'social_googlescholar': None,
'creation_timestamp': None
}
self.assertEqual(self.user.info, expinfo)

Expand Down
3 changes: 2 additions & 1 deletion qiita_db/test/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ def test_get_table_cols(self):
exp = {"email", "user_level_id", "password", "name", "affiliation",
"address", "phone", "user_verify_code", "pass_reset_code",
"pass_reset_timestamp", "receive_processing_job_emails",
"social_orcid", "social_researchgate", "social_googlescholar"}
"social_orcid", "social_researchgate", "social_googlescholar",
"creation_timestamp"}
self.assertEqual(set(obs), exp)

def test_exists_table(self):
Expand Down
6 changes: 6 additions & 0 deletions qiita_db/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,12 @@ def create(cls, email, password, info=None):
cls._table, ','.join(columns), ','.join(['%s'] * len(values)))
qdb.sql_connection.TRN.add(sql, values)

# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the default in the database is now(), then this is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like removing this code a lot :-) I was not sure how SQL will deal with old entries that have None in this column.

sql = """INSERT INTO qiita.message_user (email, message_id)
SELECT %s, message_id FROM qiita.message
Expand Down
8 changes: 6 additions & 2 deletions qiita_pet/handlers/user_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,9 @@ class UserProfileHandler(BaseHandler):
def get(self):
profile = UserProfile()
profile.process(data=self.current_user.info)
self.render("user_profile.html", profile=profile, msg="", passmsg="")
self.render("user_profile.html", profile=profile, msg="", passmsg="",
creation_timestamp=self.current_user.info[
'creation_timestamp'])

@authenticated
@execute_as_transaction
Expand Down Expand Up @@ -248,7 +250,9 @@ def post(self):
else:
passmsg = "Incorrect old password"
self.render("user_profile.html", user=user.id, profile=form_data,
msg=msg, passmsg=passmsg)
msg=msg, passmsg=passmsg,
creation_timestamp=self.current_user.info[
'creation_timestamp'])


class ForgotPasswordHandler(BaseHandler):
Expand Down
3 changes: 3 additions & 0 deletions qiita_pet/templates/user_profile.html
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ <h3>User Information</h3>
{% end %}
</div>
{% end %}
{%if creation_timestamp is not None %}
<div style="padding-left: 1em; padding-bottom: 1em; color: grey;">account created on {{creation_timestamp}}</div>
{% end %}
<div style="color:{% if msg.startswith('ERROR:') %}red{% else %}darkgreen{% end %};">{{msg}}</div>
<button type="submit" class="btn btn-success">Save Edits</button>
</form>
Expand Down
Loading