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

permissions: Add PODReadAccess permission #1213

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

cdamian
Copy link
Contributor

@cdamian cdamian commented Feb 24, 2023

Description

Added PODReadAccess permission.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Permissions unit tests

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I rebased on the latest main branch

@cdamian cdamian force-pushed the add-pod-read-access-permission branch from 4b52f1f to a12ac30 Compare February 24, 2023 11:26
@@ -289,6 +295,7 @@ where
}
PoolRole::LoanAdmin => Ok(self.pool_admin.insert(PoolAdminRoles::RISK_ADMIN)),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unrelated to this PR, but the naming seems a bit off here, not all pool roles are pool admin roles. I wonder if we should change self.pool_admin to self.pool and PoolAdminRoles to just PoolRoles. What do you think?

cc @mustermeiszer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds sane to me. I would be more pro having an additional struct like PoolAdminRoles and PoolRoles but this would mean we need to write a migration. So lets keep it simple.

@cdamian cdamian force-pushed the add-pod-read-access-permission branch from a12ac30 to 032ffb2 Compare February 24, 2023 11:34
@cdamian cdamian force-pushed the add-pod-read-access-permission branch from 032ffb2 to 2521570 Compare February 24, 2023 13:02
Copy link
Collaborator

@mustermeiszer mustermeiszer 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 to me!

@@ -289,6 +295,7 @@ where
}
PoolRole::LoanAdmin => Ok(self.pool_admin.insert(PoolAdminRoles::RISK_ADMIN)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds sane to me. I would be more pro having an additional struct like PoolAdminRoles and PoolRoles but this would mean we need to write a migration. So lets keep it simple.

@cdamian cdamian merged commit 9148f05 into main Feb 24, 2023
@cdamian cdamian deleted the add-pod-read-access-permission branch February 24, 2023 20:07
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