#2564: Revise fixtures to include portfolios - [RJM] #2847
Conversation
rachidatecs
requested review from
therealslimhsiehdy,
Matt-Spence,
CocoByte,
abroddrick,
dave-kennedy-ecs,
erinysong,
asaki222 and
zandercymatics
|
🥳 Successfully deployed to developer sandbox rjm. |
|
🥳 Successfully deployed to developer sandbox rjm. |
|
🥳 Successfully deployed to developer sandbox rjm. |
|
🥳 Successfully deployed to developer sandbox rjm. |
|
🥳 Successfully deployed to developer sandbox rjm. |
Co-authored-by: Alysia Broddrick <[email protected]>
|
🥳 Successfully deployed to developer sandbox rjm. |
|
🥳 Successfully deployed to developer sandbox rjm. |
| @staticmethod | ||
| def _generate_fake_expiration_date(days_in_future=365): | ||
| """Generates a fake expiration date between 1 and 365 days in the future.""" | ||
| current_date = timezone.now().date() # nosec | ||
| return current_date + timedelta(days=random.randint(1, days_in_future)) # nosec | ||
|
|
||
| @staticmethod | ||
| def _generate_fake_expiration_date_in_past(): | ||
| """Generates a fake expiration date up to 365 days in the past.""" | ||
| current_date = timezone.now().date() # nosec | ||
| return current_date + timedelta(days=random.randint(-365, -1)) # nosec |
| # Approve the current and expired domain requests | ||
| approved_domain = cls._approve_request(domain_request, users) | ||
| expired_domain = cls._approve_request(domain_request_expired, users, is_expired=True) | ||
|
|
||
| # Collect objects to update | ||
| if domain_request: | ||
| domain_requests_to_update.append(domain_request) | ||
| if domain_request_expired: | ||
| domain_requests_to_update.append(domain_request_expired) | ||
| if approved_domain: | ||
| domains_to_update.append(approved_domain) | ||
| if expired_domain: | ||
| domains_to_update.append(expired_domain) |
There was a problem hiding this comment.
(Blocking) Having to collect the domains here by using a DB call (get_or_create) is killing the performance benefits of using bulk_update. There is a much faster way to add an expiration date by using two for loops.
The faster way to do this would be to:
- Remove your domain logic in
_approve_request - After the first loop, create a variable that stores
domains_to_update = Domain.objects.filter(domain_info__domain_request__in= domain_requests_to_update) - Loop through that variable doing your .expired logic (as you probably inferred, just the
domain.expiration_date = (bit) - Do a bulk_update on domains as your already doing
This reduces the number of db calls from being equal to the number of users, down to two.
There may be another way to do this, too. Doesn't have to look any sort of way
| def _set_foreign_key_fields(cls, portfolio: Portfolio, portfolio_dict: dict, user: User): | ||
| """Helper method used by `load`.""" | ||
| if not portfolio.senior_official: | ||
| if "senior_official" in portfolio_dict and portfolio_dict["senior_official"] is not None: |
There was a problem hiding this comment.
| if "senior_official" in portfolio_dict and portfolio_dict["senior_official"] is not None: | |
| if portfolio_dict.get("senior_official") is not None: |
(nitpick/optional)
| to run this code. | ||
| """ | ||
|
|
||
| SUBORGS = [ |
There was a problem hiding this comment.
(optional) Imo I think its okay to add a few suborgs with random names, as was done with the random domain names.
Its okay if not though - but we it would be a nice to have to have two suborgs per org (some features rely on that)
| cls._bulk_create_suborgs(suborgs_to_create) | ||
|
|
||
| @classmethod | ||
| def _get_portfolios(cls): |
There was a problem hiding this comment.
(blocking) this function should get all existing portfolios, rather than using a predefined set of portfolios. This won't scale too well if we add more in fixtures_portfolios
There was a problem hiding this comment.
I disagree. There's no benefit in adding multiple portfolios WITH multiple nested suborgs each, because unlike requests, we do not have multiple variations (such as statuses) to test. If we need to add more portfolios, we can easily do so and they would have no suborgs, which is fine for fixtures.
If we need to add multiple suborgs to a portfolio, we can easily do so on portfolio1 or 2 and that would be sufficient.
There was a problem hiding this comment.
No need to add multiple suborgs to each - just want to reduce the need to change this list across all files when adding another portfolio
There was a problem hiding this comment.
You can add more portfolios, the suborg fixtures will ignore them
| return None | ||
|
|
||
| @classmethod | ||
| def _prepare_suborgs_to_create(cls, portfolios): |
There was a problem hiding this comment.
Similar thing here. We should avoid hard-coding this sort of data.
I think it'd be ok if you did a for loop that checks on each portfolio, if thats easier. Its not that many records.
The more scalable approach would be:
Checking for existence in that for loop you can define the following right before it: Suborganization.objects.filter(name__in=[suborg.get("name") for suborg in cls.SUBORGS]).in_bulk(field_name="name")
Then just do (in the for loop):
if not existing_suborgs.get(suborg.get("name")):
suborgs_to_create.append(Suborganization(portfolio=portfolio_item, name=suborg_item.get("name"))
There was a problem hiding this comment.
Same comment as above
| portfolios = list( | ||
| Portfolio.objects.filter(organization_name__in=["Hotel California", "Wish You Were Here"]) | ||
| ) |
There was a problem hiding this comment.
Same thing here with linking it to the fixtures_portfolios fixture
| }, | ||
| { | ||
| "username": "30001ee7-0467-4df2-8db2-786e79606060", | ||
| "first_name": "Zander", | ||
| "last_name": "Adkinson", | ||
| "title": "ACME specialist", |
|
🥳 Successfully deployed to developer sandbox rjm. |
1 similar comment
|
🥳 Successfully deployed to developer sandbox rjm. |
|
🥳 Successfully deployed to developer sandbox rjm. |
Co-authored-by: zandercymatics <[email protected]>
|
🥳 Successfully deployed to developer sandbox rjm. |
Ticket
Resolves #2564
Changes
Context for reviewers
The portfolios created are
Hotel CaliforniaandWish You Were HereOn your sandbox, you may have permissions on an existing portfolio and that would take precedence in org view. Just remove your old permissions.
Setup
Locally: It's straightforward, compose up and then run
docker-compose exec app python manage.py loada few timesSandboxes: login
cf login -a api.fr.cloud.gov --ssoand run fixtures a few timescf run-task getgov-<sandboxname> --command "./manage.py load" --name fixtures. Keep an eye out for breaking errors (you should not see any) and the data (only issue is you may already have a 'primary' portfolio, see above.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
Ensured code standards are met (Code reviewer)
Validated user-facing changes as a developer
New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Meets all designs and user flows provided by design/product
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Tested with multiple browsers, the suggestion is to use ones that the developer didn't (check off which ones were used)
(Rarely needed) Tested as both an analyst and applicant user
Note: Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist
As a designer reviewer, I have
Verified that the changes match the design intention
Validated user-facing changes as a designer
Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Tested with multiple browsers (check off which ones were used)
(Rarely needed) Tested as both an analyst and applicant user
Screenshots