#3363: UAT Quick Bug Fixes - [AD] #3372
Conversation
|
🥳 Successfully deployed to developer sandbox rh. |
|
🥳 Successfully deployed to developer sandbox rh. |
|
🥳 Successfully deployed to developer sandbox rh. |
|
🥳 Successfully deployed to developer sandbox rh. |
… rh/3363-uat-1-bug-fixes
|
🥳 Successfully deployed to developer sandbox rh. |
therealslimhsiehdy
requested review from
lizpearl,
Matt-Spence,
CocoByte,
rachidatecs,
dave-kennedy-ecs,
erinysong and
zandercymatics
therealslimhsiehdy
changed the title
|
🥳 Successfully deployed to developer sandbox rh. |
… rh/3363-uat-1-bug-fixes
|
🥳 Successfully deployed to developer sandbox rh. |
| @@ -443,6 +443,12 @@ def setUp(self): | |||
| name="domainrenewal.gov", | |||
| ) | |||
|
|
|||
| self.domainnotexpiring, _ = Domain.objects.get_or_create( | |||
There was a problem hiding this comment.
nitpck please use camel case or snake case but not all lowercase for your variables, all lowercase makes it harder to read. Same comment for the variable below
| name="domainnotexpiring.gov", expiration_date=timezone.now().date() + timedelta(days=65) | ||
| ) | ||
|
|
||
| self.domainnodomainmanager, _ = Domain.objects.get_or_create(name="domainnodomainmanager") |
There was a problem hiding this comment.
nitpick In general don't use invalid domain names in test unless you want to test for an invalid case. We really should, down the road, make it so a domain can never be created that is invalid, but in the meantime you can not have a domain that doesn't have .gov so change any of your tests that are missing the .gov so that they have the correct ending.
| @@ -657,6 +662,21 @@ def test_domain_renewal_form_domain_manager_edit(self): | |||
| self.assertEqual(edit_page.status_code, 200) | |||
| self.assertContains(edit_page, "Domain managers can update all information related to a domain") | |||
|
|
|||
| @override_flag("domain_renewal", active=True) | |||
| def test_domain_renewal_form_not_expired_or_expiring(self): | |||
There was a problem hiding this comment.
nitpick All new functions should have docstring comment of some kind, especially a test.
| """Domain detail overview page.""" | ||
|
|
||
| template_name = "domain_renewal.html" | ||
|
|
||
| def get_context_data(self, **kwargs): | ||
| """Grabbing security email information for the renewal form""" |
There was a problem hiding this comment.
| """Grabbing security email information for the renewal form""" | |
| """Grabs the security email information and adds security_email to the renewal form context | |
| sets it to None if it uses a default email""" |
| context["security_email"] = security_email | ||
| return context | ||
|
|
||
| def in_editable_state(self, pk): |
There was a problem hiding this comment.
comment 100 percent I am reading this as inedible
maybe you could just say is_editable? usually with boolean returns using is_ or has_ to indicate a yes no (true/false) state helps with readability. not blocking on it though
| if security_email is None or security_email in default_emails: | ||
| context["security_email"] = None | ||
| return context | ||
| context["security_email"] = security_email |
There was a problem hiding this comment.
suggestion if you move this line before the if statement you can delete the return currently on 331 and just have one return in your function
| if Domain.objects.filter(id=pk).exists(): | ||
| requested_domain = Domain.objects.get(id=pk) | ||
|
|
||
| if requested_domain and (requested_domain.is_expiring() or requested_domain.is_expired()): |
There was a problem hiding this comment.
suggestion: you can just return this
| if requested_domain and (requested_domain.is_expiring() or requested_domain.is_expired()): | |
| # return if domain is expired or close to expiring | |
| return requested_domain and (requested_domain.is_expiring() or requested_domain.is_expired()): |
you can then remove the return true/false lines
|
|
||
| def in_editable_state(self, pk): | ||
| """Override in_editable_state from DomainPermission | ||
| Allow renewal form to be accessed""" |
There was a problem hiding this comment.
| Allow renewal form to be accessed""" | |
| Allow renewal form to be accessed | |
| returns boolean""" |
suggestion
| Allow renewal form to be accessed""" | ||
|
|
||
| requested_domain = None | ||
| if Domain.objects.filter(id=pk).exists(): |
There was a problem hiding this comment.
comment: I am just skimming but, I do recall that the one you are overriding has checks for if the domain is editbable that then include a check on state (so that you cannot change/edit an on hold or deleted domain. Reading this code, it looks like an onhold or deleted domain could still have and editable renewal form. Instead, you may want to call the super in_editable_state and add that to your return value.
There was a problem hiding this comment.
I did a drive-by code read through, and see my comment regarding a corner case you may be missing with on hold and deleted domains. I didn't go indepth into the code, but another suggestion( besides the one I made already) is that you can make sure is expired and is expiring never show on deleted and on hold domains by adding a check on state to you is_expiring and is_expired functions so that if the domain is in either of these states it returns false. If you don't do this it looks like your state_display function on models domain will result in deleted and on hold domains being flagged as expired/expiring... We would definitely not want that.
Ticket
Resolves #3363
Changes
usa-site-alert--slimThis domain will expire soon. Go to “Manage” to renew the domain./renewalat the end of the URL and we are not a domain manager, we should get a 403 and NOT be able to see the form UNLESS it's expired or expiring/renewalat the end of the URL we should get a 403 and NOT be able to see the form UNLESS it's expired or expiringContext for reviewers
While we are doing UAT testing, we found some bugs that we were decently "quick" fixes to resolve
Setup
Please look at Google Sheet, specifically the bug/questions section on how to reproduce/directions - if confused reach out to Rebecca or Aditi.
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