#3316: automatically add domain managers as portfolio members - [MS] #3421
Conversation
|
🥳 Successfully deployed to developer sandbox ms. |
Matt-Spence
changed the title
|
🥳 Successfully deployed to developer sandbox ms. |
There was a problem hiding this comment.
Would you be able to pull stagings DB into getgov-ms so I can test this script there?
As for the code:
Great unit tests. You are just missing a documentation update in data_migration.md, but looks good. Runs well locally on about 25 records.
Since this script will be dealing with ~200 or so records (and maybe more if we loop through more portfolios), I've included some optional optimization things. Its obviously not as important for scripts to be super performant, but it would be a nice-to-have as speaking from experience with a lot of records it can cut down runtime from 20-30 minutes down to a few seconds with large enough datasets (like our transition domains).
I did not note it, but you could also use bulk_create for all three of your steps - though you may lose some logging detail under try/except. If that is something you want to do, what I've personally done in that case is to make the query more specific such that it never enters a error state and instead just lumps it under skipped instead
| for email in invited_managers: | ||
| self.create_portfolio_invitation(portfolio, email) | ||
|
|
||
| def create_portfolio_invitation(self, portfolio: Portfolio, email: str): |
There was a problem hiding this comment.
(Q) Do we auto-send emails now when they are created now? We added some logic to these recently, I just don't entirely recall the rules to it
There was a problem hiding this comment.
I don't think we automatically send them when a portfolio invitation object is created, I think we send emails at the same time but as it's own separate command. That said I also don't entirely understand these rules. I didn't receive any emails when I tested it but it's always possible that there was an email whitelist issue or something.
There was a problem hiding this comment.
I'm pretty sure that I would need to be calling the send_portfolio_invitation_email function to send an email.
There was a problem hiding this comment.
Thank you for checking in on this! I think this is definitely a non-issue
| agency_name = options.get("agency_name") | ||
| branch = options.get("branch") | ||
| parse_requests = options.get("parse_requests") | ||
| parse_domains = options.get("parse_domains") | ||
| both = options.get("both") | ||
| add_managers = options.get("add_managers") |
There was a problem hiding this comment.
On line 105, can you change that line to be if parse_requests or parse_domains or add_managers and change the CommandError? This would add capability to script such that you can just run it with just create_federal_portfolio --add_managers, which would be useful in situations where we don't also want to add new domain requests or domains
(sorry for the image, doesn't let me select it directly)
There was a problem hiding this comment.
That's a good idea, will do.
Co-authored-by: zandercymatics <[email protected]>
Co-authored-by: zandercymatics <[email protected]>
…/github.com/cisagov/manage.get.gov into ms/3316-automatically-add-portfolio-members
| skip_existing_portfolios = options.get("skip_existing_portfolios") | ||
|
|
||
| if not both: | ||
| if not parse_requests and not parse_domains: | ||
| raise CommandError("You must specify at least one of --parse_requests or --parse_domains.") | ||
| if not (parse_requests or parse_domains or add_managers): |
There was a problem hiding this comment.
Nice refactor here, thanks
|
|
||
| # Fetch all domains associated with the portfolio | ||
| domains = Domain.objects.filter(domain_info__portfolio=portfolio) | ||
| domain_managers: set[int] = set() |
There was a problem hiding this comment.
| domain_managers: set[int] = set() | |
| domain_managers: set[UserDomainRole] = set() |
| for id in domain_managers: | ||
| try: | ||
| # manager is a user id | ||
| user = User.objects.get(id=id) | ||
| _, created = UserPortfolioPermission.objects.get_or_create( | ||
| portfolio=portfolio, | ||
| user=user, | ||
| defaults={"roles": [UserPortfolioRoleChoices.ORGANIZATION_MEMBER]}, | ||
| ) | ||
| self.added_managers.add(user) | ||
| if created: | ||
| logger.info(f"Added manager '{user}' to portfolio '{portfolio}'") | ||
| else: | ||
| logger.info(f"Manager '{user}' already exists in portfolio '{portfolio}'") | ||
| except User.DoesNotExist: | ||
| self.failed_managers.add(user) | ||
| logger.debug(f"User '{user}' does not exist") |
There was a problem hiding this comment.
Did you mean to change this as well, given the changes to domain_managers?
|
🥳 Successfully deployed to developer sandbox ms. |
2 similar comments
|
🥳 Successfully deployed to developer sandbox ms. |
|
🥳 Successfully deployed to developer sandbox ms. |
|
🥳 Successfully deployed to developer sandbox ms. |
|
🥳 Successfully deployed to developer sandbox ms. |
1 similar comment
|
🥳 Successfully deployed to developer sandbox ms. |
|
🥳 Successfully deployed to developer sandbox ms. |
Ticket
Resolves #3316
Changes
Context for reviewers
Setup
Identify or create several domains that are part of the same organization and have a few managers, at least one of which should be you (for checking emails). Run the script for that organization with the --add-managers flag set to True, and ensure that:
Code Review Verification Steps
As the original developer, I have
Satisfied acceptance criteria and met development standards
Ensured code standards are met (Original Developer)
Validated user-facing changes (if applicable)
As a code reviewer, I have
Reviewed, tested, and left feedback about the changes
Validated user-facing changes as a developer
Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.
As a designer reviewer, I have
Verified that the changes match the design intention
Validated user-facing changes as a designer
References
Screenshots