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

Expired link is deleted from database when expired #1466

Conversation

rv0lt
Copy link
Member

@rv0lt rv0lt commented Sep 14, 2023

Before submitting this PR

  1. Description: When an invite is sent, the expiry time is set to exactly 7 days from now (incl the time). At midnight there is a cronjob which finds the expired invites in the Invites table and removes them. This means that there is a gap between the actual expiry and when the invite is deleted from the database. This PR fixes that. As well as adding some tests

  2. Jira task / GitHub issue: https://scilifelab.atlassian.net/jira/software/projects/DDS/boards/13?isInsightsOpen=true&selectedIssue=DDS-1642

  3. How to test: Add information on how someone could manually test this functionality. As detailed as possible.

  4. Type of change: Check the relevant boxes in the section below

  5. Add docstrings and comments to code, even if you personally think it's obvious.

What type of change(s) does the PR contain?

  • New feature
    • Breaking: Please describe the reason for the break and how we can fix it.
    • Non-breaking
  • Database change
    • Migration included in PR
    • Migration not needed
  • Bug fix
    • Breaking: Please describe the reason for the break and how we can fix it.
    • Non-breaking
  • Security Alert fix
  • Documentation
  • Tests (only)
  • Workflow

Checklist

  • Sprintlog
    • Added
    • Not needed (E.g. PR contains only tests)
  • Rebase / Update / Merge from base branch (the branch from which the current is forked)
    • Done
    • Not needed
  • Blocking PRs
    • Merged
    • No blocking PRs
  • PR to master branch

Actions / Scans

  • Black: Python code formatter. Does not execute. Only tests.
    Run black . locally to execute formatting.
    • Passed
  • Prettier: General code formatter. Our use case: MD and yaml mainly.
    Run npx prettier --write . locally to execute formatting.
    • Passed
  • Yamllint: Linting of yaml files.
    • Passed
  • Tests: Pytest to verify that functionality works as expected.
    • New tests added
    • No new tests
    • Passed
  • CodeQL: Scan for security vulnerabilities, bugs, errors
    • New alerts: Go through them and either fix, dismiss och ignore. Add reasoning in items below.
    • Alerts fixed: What?
    • Alerts ignored / dismissed: Why?
    • Passed
  • Trivy: Security scanner
    • New alerts: Go through them and either fix, dismiss och ignore. Add reasoning in items below.
    • Alerts fixed: What?
    • Alerts ignored / dismissed: Why?
    • Passed
  • Snyk: Security scanner
    • New alerts: Go through them and either fix, dismiss och ignore. Add reasoning in items below.
    • Alerts fixed: What?
    • Alerts ignored / dismissed: Why?
    • Passed

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #1466 (753ead3) into dev (ca45daf) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1466      +/-   ##
==========================================
+ Coverage   90.11%   90.15%   +0.03%     
==========================================
  Files          29       29              
  Lines        4401     4407       +6     
==========================================
+ Hits         3966     3973       +7     
+ Misses        435      434       -1     
Files Changed Coverage Δ
dds_web/api/schemas/user_schemas.py 88.88% <100.00%> (+0.65%) ⬆️
dds_web/api/user.py 90.63% <100.00%> (+0.20%) ⬆️

@rv0lt rv0lt requested a review from i-oden September 14, 2023 12:16
@rv0lt rv0lt marked this pull request as ready for review September 14, 2023 12:27
@rv0lt rv0lt changed the title added test and functionality Verify that expired invite is deleted from database when link is clicked Sep 14, 2023
dds_web/api/user.py Outdated Show resolved Hide resolved
@i-oden i-oden requested a review from valyo September 15, 2023 07:32
dds_web/api/user.py Outdated Show resolved Hide resolved
dds_web/api/user.py Outdated Show resolved Hide resolved
@rv0lt
Copy link
Member Author

rv0lt commented Sep 15, 2023

@i-oden You mean adding tests that check that the ProjectInviteKeys are removed right?

@i-oden
Copy link
Member

i-oden commented Sep 15, 2023

@i-oden You mean adding tests that check that the ProjectInviteKeys are removed right?

@rv0lt Correct!

@rv0lt rv0lt requested a review from i-oden September 18, 2023 11:42
Copy link
Member

@i-oden i-oden left a comment

Choose a reason for hiding this comment

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

Looks good! Few comments / suggestions

  • Change the title of the PR so that it reflects the changes (expired invite deleted when new invite attempted or something like that)
  • Fix / clarify a few things in the tests
  • Add a test for the SQLAlchemyError. You don't need to add for the OperationalError, it's more complicated and it will anyway produce the same message. Tip: Check the codecov report further up in this PR (see image below). The percentage within the <> brackets should be 100% or as close to it as possible (if not 100 then explain why / discuss with us for example) for the files that are listed in the table. That means that 100% of your code changes are covered by tests.

image

dds_web/api/schemas/user_schemas.py Outdated Show resolved Hide resolved
dds_web/api/user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
@rv0lt rv0lt changed the title Verify that expired invite is deleted from database when link is clicked Expired link is deleted from database when expired Sep 18, 2023
@rv0lt
Copy link
Member Author

rv0lt commented Sep 18, 2023

@i-oden like this changes I push you mean? I dont know How to check for example for the ddserr.databaseerror as that was already part of the code I just move it around.

@i-oden
Copy link
Member

i-oden commented Sep 18, 2023

@i-oden like this changes I push you mean? I dont know How to check for example for the ddserr.databaseerror as that was already part of the code I just move it around.

What you've added now is a good start, but you'll need to change a bit and add a bit of code too. I can have a look tomorrow again and come with a suggestion!

Copy link
Member

@i-oden i-oden left a comment

Choose a reason for hiding this comment

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

Suggestions regarding the tests

dds_web/api/user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Show resolved Hide resolved
SPRINTLOG.md Outdated Show resolved Hide resolved
rv0lt and others added 7 commits September 19, 2023 09:56
Co-authored-by: Ina Odén Österbo <[email protected]>
Co-authored-by: Ina Odén Österbo <[email protected]>
Co-authored-by: Ina Odén Österbo <[email protected]>
Co-authored-by: Ina Odén Österbo <[email protected]>
Co-authored-by: Ina Odén Österbo <[email protected]>
@rv0lt rv0lt requested a review from i-oden September 19, 2023 09:07
Copy link
Member

@i-oden i-oden left a comment

Choose a reason for hiding this comment

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

Looks great and now the test coverage of your changes is 100% 👏🏻

Just a couple of very minor changes, just for consistency, then I'm ready to approve.

tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
rv0lt and others added 2 commits September 19, 2023 11:46
Co-authored-by: Ina Odén Österbo <[email protected]>
Co-authored-by: Ina Odén Österbo <[email protected]>
Copy link
Member

@i-oden i-oden left a comment

Choose a reason for hiding this comment

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

Good job!

Once @valyo has also approved, you can merge it.

Copy link
Member

@valyo valyo left a comment

Choose a reason for hiding this comment

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

Looks good and works as expected. Just small corrections in the tests docstrings

tests/api/test_user.py Outdated Show resolved Hide resolved
tests/api/test_user.py Outdated Show resolved Hide resolved
rv0lt and others added 2 commits September 20, 2023 11:27
Co-authored-by: Valentin Georgiev <[email protected]>
Co-authored-by: Valentin Georgiev <[email protected]>
@rv0lt rv0lt merged commit d10dcf7 into dev Sep 20, 2023
15 checks passed
@rv0lt rv0lt deleted the dds-1642-verify-that-expired-invite-is-deleted-from-database-when-link-is-clicked branch September 20, 2023 09:55
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.

3 participants