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

List possible values when inviting a team within an environment #158

Conversation

LEUNGUU
Copy link
Contributor

@LEUNGUU LEUNGUU commented Oct 7, 2022

Feature or Bugfix

  • Feature

Detail

  • Provide a drop down list of Cognito Groups for user to choose when inviting a team to an organization or an environment

Relates

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@LEUNGUU LEUNGUU requested a review from dlpzx October 7, 2022 10:44
@dlpzx dlpzx requested a review from louishourcade October 10, 2022 07:29
@dlpzx dlpzx changed the title [WIP] List possible values when inviting a team within an environment List possible values when inviting a team within an environment Oct 10, 2022
@dlpzx
Copy link
Contributor

dlpzx commented Oct 10, 2022

As an additional step, we could filter the groups in the dropdown to list all cognito groups, minus the groups that have already been invited @LEUNGUU

Copy link
Contributor

@louishourcade louishourcade left a comment

Choose a reason for hiding this comment

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

Thanks for your PR @LEUNGUU,
Can you remove the console logs in EnvironmentTeamInviteForm.js ?
Also as @dlpzx mentioned, I think it would be nice to show only groups that do have access to he environment yet. Could you add this to the PR ?

@LEUNGUU
Copy link
Contributor Author

LEUNGUU commented Oct 11, 2022

Thanks for your PR @LEUNGUU, Can you remove the console logs in EnvironmentTeamInviteForm.js ? Also as @dlpzx mentioned, I think it would be nice to show only groups that do have access to he environment yet. Could you add this to the PR ?

Hi Louis. Sure. I will look into it.

Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

Final remarks:

  1. Environment and organization permissions. In the original implementation we used calls to the RDS DB that had been decorated and checked the permissions on the orga or environment. Now we do not check the permissions when listing the groups, but we still check the permissions when we invite the team. Therefore, we are not loosing control over the "invite" action.
  2. We removed the integration tests on the list environment groups. This one I think it is important (and I will add a test in the PR). This api call is essential for the use of data.all. Without it teams cannot be added and work on the environment/orga

@dlpzx
Copy link
Contributor

dlpzx commented Oct 17, 2022

Hi @LEUNGUU, I have added the integration test for the api. For the integration test it was easier to move the cognito calls to the handlers (for mocking). I also like having all AWS boto3 calls in the handlers, even if they are calling services in the central infra account

@LEUNGUU
Copy link
Contributor Author

LEUNGUU commented Oct 17, 2022

@dlpzx Even though this 'list_cognito_groups' function is not so consistent with others in handlers folder, well, I agree with you. Let's put it there.

@dlpzx dlpzx self-requested a review October 18, 2022 15:21
Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

I tested the coverage integration tests and they succeded

@dlpzx dlpzx merged commit de6ec86 into v1m2m0 Oct 20, 2022
@dlpzx dlpzx deleted the list-and-search-for-possible-values-when-inviting-a-team-within-an-environment branch November 9, 2022 09:16
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.

List and search for possible values when inviting a team within an environment
3 participants