search

ScilifelabDataCentre / dds_web

A cloud-based system for the delivery of data from SciLifeLab Facilities to their users (e.g. research group).
Other
8 stars 8 forks source link

Flask command to change unit quotas #1551

Closed rv0lt closed 1 month ago

rv0lt commented 1 month ago

Read this before submitting the PR

  1. Always create a Draft PR first
  2. Go through sections 1-5 below, fill them in and check all the boxes
  3. Make sure that the branch is updated; if there's an "Update branch" button at the bottom of the PR, rebase or update branch.
  4. When all boxes are checked, information is filled in, and the branch is updated: mark as Ready For Review and tag reviewers (top right)
  5. Once there is a submitted review, implement the suggestions (if reasonable, otherwise discuss) and request an new review.

If there is a field which you are unsure about, enter the edit mode of this description or go to the PR template; There are invisible comments providing descriptions which may be of help.

1. Description / Summary

Added a new command to update the quotas of a unit

2. Jira task / GitHub issue

Link to the github issue or add the Jira task ID here.

3. Type of change

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

Check the relevant boxes below. For an explanation of the different sections, enter edit mode of this PR description template.

4. Additional information

5. Actions / Scans

Check the boxes when the specified checks have passed.

For information on what the different checks do and how to fix it if they're failing, enter edit mode of this description or go to the PR template.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.49%. Comparing base (14069c9) to head (19c2521). Report is 18 commits behind head on dev.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #1551 +/- ## ========================================== + Coverage 92.46% 92.49% +0.03% ========================================== Files 29 29 Lines 4830 4851 +21 ========================================== + Hits 4466 4487 +21 Misses 364 364 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rv0lt commented 1 month ago

I find both commands difficult to use if I don't see the current info/quota values (they can be seen by querying the DB, but that's obviously not optimal). I suggest that the commands can at least print the current values before prompting for new ones.

That's a valid point! I can add another line to the confirmation prompt for the new command. However, for the old command I am not sure if it should belong here or to a different PR

valyo commented 1 month ago

However, for the old command I am not sure if it should belong here or to a different PR

Yes, maybe the old command should be considered in a different PR

i-oden commented 1 month ago

I find both commands difficult to use if I don't see the current info/quota values (they can be seen by querying the DB, but that's obviously not optimal). I suggest that the commands can at least print the current values before prompting for new ones.

That's a valid point! I can add another line to the confirmation prompt for the new command. However, for the old command I am not sure if it should belong here or to a different PR

@rv0lt for the update_unit_sto4 command, do not change it to display the info first. Sounds like a good idea for this new command though.

i-oden commented 1 month ago

Why don't we add possibility for change of "External Display Name", DIA and DIE, while we are dealing with this. Is there a reason that all options of update-unit-sto4 are required, maybe @i-oden knows?

@valyo We shouldn't mix tasks, there's always a risk of complication and also we need to add more tests, plus it's more difficult to review. We should keep the PRs as simple as possible and also finish the tasks planned for the sprint before adding new ones from the backlog.

valyo commented 1 month ago

@rv0lt for the update_unit_sto4 command, do not change it to display the info first.

@i-oden do you mean in this PR, or you think it should not display the info at all?

i-oden commented 1 month ago

@rv0lt for the update_unit_sto4 command, do not change it to display the info first.

@i-oden do you mean in this PR, or you think it should not display the info at all?

We shouldn't display them at all, at least not the keys. We can display the endpoint, like "sto2" or "sto4" or something like that but other than that, no.