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

Catch KeyNotFoundError #1045

Merged
merged 31 commits into from
Mar 14, 2022
Merged

Catch KeyNotFoundError #1045

merged 31 commits into from
Mar 14, 2022

Conversation

i-oden
Copy link
Member

@i-oden i-oden commented Mar 10, 2022

The CLI displays INTERNAL SERVER ERROR if a unit user does not have access to a specific project and tries to give another user access. This PR returns the specific errors (should only be one atm) and which projects were affected.

To test:

  1. Use the Super Admin account to add a new Unit Personnel
  2. Use the new Unit Personnel account to give another user access to a project

Needs to be used with ScilifelabDataCentre/dds_cli#379, otherwise it will just say that all is well every time.

  • Tests passing
  • Black formatting
  • [ - ] Migrations for any changes to the database schema
  • [ - ] Rebase/merge the dev branch
  • Note in the CHANGELOG

@i-oden i-oden self-assigned this Mar 10, 2022
@i-oden i-oden added the must have Cannot deliver on target date without this label Mar 10, 2022
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #1045 (ba731fd) into dev (83b71e1) will increase coverage by 0.24%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1045      +/-   ##
==========================================
+ Coverage   87.50%   87.75%   +0.24%     
==========================================
  Files          27       27              
  Lines        2961     2964       +3     
==========================================
+ Hits         2591     2601      +10     
+ Misses        370      363       -7     
Impacted Files Coverage Δ
dds_web/api/project.py 88.43% <66.66%> (-0.70%) ⬇️
dds_web/api/user.py 88.49% <73.07%> (+1.49%) ⬆️
dds_web/api/api_s3_connector.py 89.18% <0.00%> (-1.72%) ⬇️
dds_web/api/s3.py 66.66% <0.00%> (-1.52%) ⬇️
dds_web/utils.py 75.28% <0.00%> (-0.68%) ⬇️
dds_web/api/dds_decorators.py 84.09% <0.00%> (-0.36%) ⬇️
dds_web/web/user.py 82.14% <0.00%> (-0.08%) ⬇️
dds_web/database/models.py 93.92% <0.00%> (-0.02%) ⬇️
dds_web/run_app.py 0.00% <0.00%> (ø)
dds_web/security/auth.py 96.95% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83b71e1...ba731fd. Read the comment docs.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MatthiasZepper MatthiasZepper left a comment

Choose a reason for hiding this comment

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

Not entirely sure, but I think the logic with the try statements, database actions and mail sending should be refactored slightly.

dds_web/api/user.py Outdated Show resolved Hide resolved
dds_web/api/user.py Outdated Show resolved Hide resolved
dds_web/api/user.py Outdated Show resolved Hide resolved
dds_web/api/user.py Outdated Show resolved Hide resolved
dds_web/api/user.py Outdated Show resolved Hide resolved
@i-oden
Copy link
Member Author

i-oden commented Mar 11, 2022

I need to change the tests. The unit user that's being used does not have a key in the table - that's why the tests are failing.

@i-oden i-oden marked this pull request as draft March 11, 2022 07:57
@i-oden i-oden marked this pull request as ready for review March 11, 2022 13:28
@i-oden i-oden requested a review from talavis March 11, 2022 13:31
Copy link
Contributor

@MatthiasZepper MatthiasZepper left a comment

Choose a reason for hiding this comment

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

Very nice. I like the solution with the goahead variable!

@i-oden
Copy link
Member Author

i-oden commented Mar 11, 2022

I don't understand this https://github.com/ScilifelabDataCentre/dds_web/runs/5512442090?check_suite_focus=true#step:3:667

When I run it myself it works. I'm missing something and I'm not going to be able to figure it out today. If anyone else does, let me know. 🙏🏻

@MatthiasZepper
Copy link
Contributor

I don't understand this https://github.com/ScilifelabDataCentre/dds_web/runs/5512442090?check_suite_focus=true#step:3:667

When I run it myself it works. I'm missing something and I'm not going to be able to figure it out today. If anyone else does, let me know. 🙏🏻

The unit admin you are trying to invite has the wrong role configured in the JSON: new_unit_admin = {"email": "[email protected]", "role": "Super Admin"}. Therefore the if new_invite.role in ["Unit Admin", "Unit Personnel"]: fails and the DDSArgumentError is never raised.

However, I don't understand why the test passed locally then?

@i-oden
Copy link
Member Author

i-oden commented Mar 13, 2022

I've added some more tests so if you could have a look again @MatthiasZepper.
If I could get a review from @talavis too that would be great.

dds_web/api/user.py Outdated Show resolved Hide resolved
dds_web/api/user.py Outdated Show resolved Hide resolved
dds_web/api/user.py Outdated Show resolved Hide resolved
dds_web/api/user.py Outdated Show resolved Hide resolved
@i-oden i-oden merged commit d3fb38d into dev Mar 14, 2022
@i-oden i-oden deleted the fix-internal-server-error branch March 14, 2022 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend must have Cannot deliver on target date without this tests pytest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants