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

Change display project info depending on the user role #1440

Merged
merged 18 commits into from
Jun 27, 2023

Conversation

valyo
Copy link
Member

@valyo valyo commented Jun 20, 2023

Before submitting this PR

  1. Description: Researcher users should get the Unit name instead of person's name in the "Created by" column returned by dds project info display
  2. Jira task / GitHub issue: DDS-1595
  3. How to test: Login with different roles and check the "Created by" column
  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 Jun 20, 2023

Codecov Report

Merging #1440 (3c5de7c) into dev (4fbe8a3) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1440      +/-   ##
==========================================
+ Coverage   89.40%   89.42%   +0.02%     
==========================================
  Files          29       29              
  Lines        4305     4314       +9     
==========================================
+ Hits         3849     3858       +9     
  Misses        456      456              
Impacted Files Coverage Δ
dds_web/api/project.py 90.10% <100.00%> (+0.19%) ⬆️

@valyo valyo marked this pull request as ready for review June 21, 2023 10:11
@valyo valyo requested a review from i-oden June 21, 2023 10:11
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.

There are no changes in the code, only in the tests. Maybe there's a commit that hasn't been pushed?
We also need to have a new test to check that the unit is displayed and not the name, atm it seems like we only have a check for the name if it's unit users etc.

@valyo valyo requested a review from i-oden June 22, 2023 09:32
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 but have a few suggested changes that may require test changes too.

Also add a sprintlog row

dds_web/api/project.py Outdated Show resolved Hide resolved
dds_web/api/project.py Outdated Show resolved Hide resolved
dds_web/api/project.py Outdated Show resolved Hide resolved
@valyo valyo requested a review from i-oden June 26, 2023 09:01
@i-oden
Copy link
Member

i-oden commented Jun 26, 2023

@valyo Can you rebase / update branch as well first?

tests/test_project_info.py Outdated Show resolved Hide resolved
@valyo valyo requested a review from i-oden June 26, 2023 11:11
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.

We need to change the creator information in UserProjects endpoint as well, line 448 in the same file you've changed in.

The dds project info display command works now though 👍🏻
But dds ls and dds project ls displays the name of the creator

@valyo valyo requested a review from i-oden June 26, 2023 12:47
tests/test_project_listing.py Outdated Show resolved Hide resolved
tests/test_project_listing.py Show resolved Hide resolved
@valyo valyo requested a review from i-oden June 27, 2023 08:25
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.

Found a few new things during testing. Could you also add tests for this so we can make sure that it works for deleted users as well?

dds_web/api/project.py Outdated Show resolved Hide resolved
dds_web/api/project.py Outdated Show resolved Hide resolved
Co-authored-by: Ina Odén Österbo <[email protected]>
@valyo valyo requested a review from i-oden June 27, 2023 09:26
@i-oden
Copy link
Member

i-oden commented Jun 27, 2023

@valyo Could you add the tests so that we can catch issues with deleted users?

@i-oden
Copy link
Member

i-oden commented Jun 27, 2023

@valyo Could you add the tests so that we can catch issues with deleted users?

Or would you like me to create a new task for adding tests for this?

@valyo
Copy link
Member Author

valyo commented Jun 27, 2023

it doesn't matter for me if we have a new task or not

@i-oden
Copy link
Member

i-oden commented Jun 27, 2023

it doesn't matter for me if we have a new task or not

Pro for adding tests now: we have tests to catch potential issues, we're more thorough
Pro for creating a new task to add later: we can merge, prep the new release and then you can start focusing on your CKAD exam instead

@i-oden
Copy link
Member

i-oden commented Jun 27, 2023

it doesn't matter for me if we have a new task or not

Pro for adding tests now: we have tests to catch potential issues, we're more thorough Pro for creating a new task to add later: we can merge, prep the new release and then you can start focusing on your CKAD exam instead

@valyo Lets skip the tests now. When the tests have passed the current run I can merge and we can start prepping a release.

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!

@i-oden
Copy link
Member

i-oden commented Jun 27, 2023

@valyo If and when these checks pass, you can merge the PR.

@valyo valyo merged commit ffabef9 into dev Jun 27, 2023
@i-oden i-oden deleted the DDS-1595-change-display-project-info branch June 28, 2023 06:02
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.

2 participants