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

Purge non-verified user #3417

Merged
merged 13 commits into from
Jun 20, 2024
Merged

Purge non-verified user #3417

merged 13 commits into from
Jun 20, 2024

Conversation

sjanssen2
Copy link
Contributor

@sjanssen2 sjanssen2 commented Jun 19, 2024

Handle #3416 first!

Addressing #3378 as well.
This PR is based on user creation_timestamp logging and offers a new Admin only page that lists those users that
a) are non yet verifier (i.e. user_status_id==5) and
b) where registers more than 30 days ago (give a new user sufficient time to react)
Individual or all users can be selected in the website and removed by one click.

image

@antgonza
Copy link
Member

@sjanssen2, could you pull from dev and update this branch? Thank you.

Copy link
Member

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

One comment that I completely missed in my original review. Thanks.

Comment on lines 44 to 52

UPDATE qiita.qiita_user SET creation_timestamp = '2015-12-03 13:52:42.751331-07' WHERE email = '[email protected]';

-- Jun 20, 2024
-- Add some non-verified users to the test DB to test new admin page: /admin/purge_users/

INSERT INTO qiita.qiita_user VALUES ('[email protected]', 5, '$2a$12$gnUi8Qg.0tvW243v889BhOBhWLIHyIJjjgaG6dxuRJkUM8nXG9Efe', 'JustNow', 'NonVeriUser', '1634 Edgemont Avenue', '303-492-1984', NULL, NULL, NULL, false, NULL, NULL, NULL, NOW());
INSERT INTO qiita.qiita_user VALUES ('[email protected]', 5, '$2a$12$gnUi8Qg.0tvW243v889BhOBhWLIHyIJjjgaG6dxuRJkUM8nXG9Efe', 'Oldie', 'NonVeriUser', '172 New Lane', '102-111-1984', NULL, NULL, NULL, false, NULL, NULL, NULL, NOW() - INTERVAL '1 YEAR');
INSERT INTO qiita.qiita_user VALUES ('[email protected]', 5, '$2a$12$gnUi8Qg.0tvW243v889BhOBhWLIHyIJjjgaG6dxuRJkUM8nXG9Efe', 'TooLate', 'NonVeriUser', '564 C Street', '508-492-222', NULL, NULL, NULL, false, NULL, NULL, NULL, NOW() - INTERVAL '30 DAY');
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for not catching this before but could you move these lines to patches/test_db_sql/92.sql. The issue is that if we leave them here they will also be inserted in the main deployment but in patches/test_db_sql they will only be applied to the test environment.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I think this is the source of the error.

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 changed that, makes total sense to me. I also know a guy who complained about test data leftovers in a fresh production DB ;-)

@sjanssen2
Copy link
Contributor Author

I am a bit puzzled about the current DB error raised in the tests. It looks to me like the portals test does not clean up properly for tearDown. In my attempt to reproduce this issue locally, all tests passed when executed once, but re-running those caused the same errors as in github. However, I wonder why this becomes an issue now and not already earlier. Any ideas @antgonza ?

@ackermag
Copy link

ackermag commented Jun 20, 2024 via email

@sjanssen2
Copy link
Contributor Author

I am a bit puzzled about the current DB error raised in the tests. It looks to me like the portals test does not clean up properly for tearDown. In my attempt to reproduce this issue locally, all tests passed when executed once, but re-running those caused the same errors as in github. However, I wonder why this becomes an issue now and not already earlier. Any ideas @antgonza ?

I might got the problem. Adding three new test users caused more iterations of this loop

qiita/qiita_db/portal.py

Lines 89 to 94 in 8b3f717

FOR eml IN
SELECT email FROM qiita.qiita_user
LOOP
INSERT INTO qiita.analysis
(email, name, description, dflt)
VALUES (eml, eml || '-dflt', 'dflt', true)
when creating a portal. Thus the assertion
self.assertCountEqual(obs, exp)
failed and subsequent code to clean up the new portal was never executed, leading to follow up fails.

@sjanssen2 sjanssen2 requested a review from antgonza June 20, 2024 14:51
@coveralls
Copy link

Coverage Status

coverage: 92.814% (-0.004%) from 92.818%
when pulling 71b97f3 on jlab:purge_nonverified_user
into 8b3f717 on qiita-spots:dev.

@antgonza antgonza merged commit fd2d9b7 into qiita-spots:dev Jun 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants