#2591: Remove profile feature flag - [MEOWARD] #2738

asaki222 · 2024-09-10T15:28:51Z

Ticket

Resolves #2591

Changes

Removed the profile feature flag.
Modified code, and tests

Context for reviewers

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Met the acceptance criteria, or will meet them in a subsequent PR
Created/modified automated tests
Added at least 2 developers as PR reviewers (only 1 will need to approve)
Messaged on Slack or in standup to notify the team that a PR is ready for review
Changes to “how we do things” are documented in READMEs and or onboarding guide
If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited.

Ensured code standards are met (Original Developer)

[] All new functions and methods are commented using plain language
Did dependency updates in Pipfile also get changed in requirements.txt?
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Add at least 1 designer as PR reviewer

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Pulled this branch locally and tested it
Reviewed this code and left comments
Checked that all code is adequately covered by tests
Made it clear which comments need to be addressed before this work is merged
If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited.

Ensured code standards are met (Code reviewer)

All new functions and methods are commented using plain language
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values
(Rarely needed) Did dependency updates in Pipfile also get changed in requirements.txt?

Validated user-facing changes as a developer

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

Checked that the design translated visually
Checked behavior
Checked different states (empty, one, some, error)
Checked for landmarks, page heading structure, and links
Tried to break the intended flow

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)
- Chrome
- Microsoft Edge
- FireFox
- Safari
(Rarely needed) Tested as both an analyst and applicant user

Screenshots

github-actions · 2024-09-10T15:33:30Z

🥳 Successfully deployed to developer sandbox meoward.

github-actions · 2024-09-10T15:52:33Z

🥳 Successfully deployed to developer sandbox meoward.

github-actions · 2024-09-10T15:57:01Z

🥳 Successfully deployed to developer sandbox meoward.

github-actions · 2024-09-10T16:10:04Z

🥳 Successfully deployed to developer sandbox meoward.

github-actions · 2024-09-10T21:41:49Z

🥳 Successfully deployed to developer sandbox meoward.

CocoByte

Hi! Maybe I jumped the gun on reviewing this, but it looks like there are a number of failing tests still. If this is still under construction, mind adding "[DRAFT]" to front of the PR title?

… meoward/2591-Remove-Profile-feature-flag

github-actions · 2024-09-12T18:01:01Z

🥳 Successfully deployed to developer sandbox meoward.

changes addresses, tests passing

zandercymatics

Looks like there are still some references to profile_feature!

zandercymatics · 2024-09-17T21:05:26Z

src/registrar/admin.py

-            recipient = obj.creator
-        else:
-            recipient = None
+        # TODO 2574: remove lines 1977-1978 (refactor as needed)


Still need this todo?

I can remove!

zandercymatics · 2024-09-17T21:07:42Z

src/registrar/templates/domain_request_sidebar.html

@@ -15,7 +15,7 @@
              </svg>
            {% endif %}
          {% endif %}
-        <a href="{% namespaced_url 'domain-request' this_step %}" 
+        <a href="{% namespaced_url 'domain-request' this_step %}"


seems like this file was accidentally included? I'm in favor of keeping this though as its some cleanup stuff

zandercymatics · 2024-09-17T21:07:57Z

src/registrar/templates/domain_request_sidebar.html

@@ -39,4 +39,4 @@
    {% endfor %}
    </ul>
  </nav>
-</div>
+</div>


I'd say though just remember to end the file with a newline. I think thats our convention typically

src/registrar/templates/domain_request_status.html

src/registrar/tests/test_views.py

zandercymatics · 2024-09-17T21:14:57Z

src/registrar/tests/test_views.py

-            senior_official=contact_user,
-        )
+    def test_domain_your_contact_information_when_profile_feature_on(self):
+        """test that Your contact information is not accessible when profile feature is on"""
        with override_flag("profile_feature", active=True):


looks like you missed a reference to profile_feature! I think for this test though you can actually delete it - we won't need it anymore

zandercymatics · 2024-09-17T21:16:58Z

src/registrar/tests/test_views_domain.py

+        home_page = self.app.get(reverse("user-profile"))
        self.assertContains(home_page, self.domain.name)


(Blocking) why was this changed? We shouldn't see the domain name on user-profile

When a new user first logs in, they are taken to the user-profile page, instead of the home.

@asaki222 I see your point regarding testing that! However, this test is supposed to test on if a domain invite adds that domain to the domain table after the user logs in. They only see this profile page when they are missing information on the user object - which would be why you are seeing that behavior

Since the purpose of this test is different, can you revert this? You will need to just add more data to the user object defined on 755. I think title and phone, but maybe one additional field. This will cause the user to redirect to home

The only weird thing about this is why the test is passing at all - basically I don't think the domain name should exist on the user profile page. Looking at it now, this is actually a bug caused by line 739 - the email address has the name of the domain!

Regarding that error with the test - do you mind adding a line like self.assertNotContains(home_page, email_address) as well?

@zandercymatics if I revert this test, and added the
self.assertNotContains(home_page, email_address), it would fail since its technically on the home page. Is that what you meant?

github-actions · 2024-09-17T21:46:25Z

🥳 Successfully deployed to developer sandbox meoward.

asaki222 · 2024-09-17T21:52:51Z

Looks like there are still some references to profile_feature!

Removed @zandercymatics , I must have missed these when I was trying to debug

github-actions · 2024-09-17T21:55:02Z

🥳 Successfully deployed to developer sandbox meoward.

github-actions · 2024-09-17T21:56:18Z

🥳 Successfully deployed to developer sandbox meoward.

github-actions · 2024-09-18T13:47:48Z

🥳 Successfully deployed to developer sandbox meoward.

zandercymatics

Your changes look great! I ran it locally and it works on meoward!
Just two more things:

There are some old documentation artifacts for profile_feature in adding-feature-flags.md and on the function description for waffle_flag.py. Can you remove those?
There is a blocking change on that test_domain_invitation flow one! I left some more details as to why - its a pretty subtle one

github-actions · 2024-09-18T16:06:58Z

🥳 Successfully deployed to developer sandbox meoward.

github-actions · 2024-09-18T18:35:21Z

🥳 Successfully deployed to developer sandbox meoward.

zandercymatics

your changes look good but I think you need to lint. The jobs haven't passed yet - but if all of them do then all is good. You just need to also remove the references in our documentation, then I will approve after both of those are true!

github-actions · 2024-09-18T19:41:38Z

🥳 Successfully deployed to developer sandbox meoward.

github-actions · 2024-09-18T20:10:28Z

🥳 Successfully deployed to developer sandbox meoward.

github-actions · 2024-09-18T20:18:43Z

🥳 Successfully deployed to developer sandbox meoward.

asaki222 added 8 commits September 9, 2024 11:32

removed method to add has_profile_feature_flag to context, and reference

44c911e

removed profile feature flag reference from process view method

2e3f4bd

removed the profile_feature_flag from send_status_update_emailmethod

32e3d2f

final changes

c2bdfaa

remvoed all reference to profile feature flag in code

8954efb

adding back some conditionals for feature flag

db46ca9

removed submitter

f7c3579

ran formatting command

1203e54

asaki222 requested a review from abroddrick as a code owner September 10, 2024 15:28

asaki222 closed this Sep 10, 2024

asaki222 deleted the meoward/2591-Remove-Profile-feature-flag branch September 10, 2024 15:37

asaki222 restored the meoward/2591-Remove-Profile-feature-flag branch September 10, 2024 15:48

asaki222 reopened this Sep 10, 2024

removed unused reference

acfaaa0

fixed linting issues

6d57f51

asaki222 added 2 commits September 10, 2024 13:29

reverted tests

b17ef34

made changes to the tests

519c942

CocoByte previously requested changes Sep 11, 2024

View reviewed changes

CocoByte self-assigned this Sep 11, 2024

asaki222 changed the title ~~Meoward/2591 remove profile feature flag~~ DRAFT - Meoward/2591 remove profile feature flag Sep 11, 2024

asaki222 added 3 commits September 11, 2024 16:36

Merge branch 'main' of https://github.com/cisagov/manage.get.gov into…

e7936ce

… meoward/2591-Remove-Profile-feature-flag

updated to main

91532ad

closed a tag

d386e69

put the end tag back in its correct spot

a55c019

asaki222 changed the title ~~(Draft) #2591: Remove profile feature flag - [MEOWARD]~~ #2591: Remove profile feature flag - [MEOWARD] Sep 17, 2024

abroddrick requested a review from CocoByte September 17, 2024 19:22

abroddrick requested review from zandercymatics and dave-kennedy-ecs September 17, 2024 19:23

zandercymatics requested changes Sep 17, 2024

View reviewed changes

removed the remaining profile feature"

e8d36b9

asaki222 added 2 commits September 17, 2024 17:50

fixing format

441095f

added new line

270094e

removed unused reference

0d69558

zandercymatics requested changes Sep 18, 2024

View reviewed changes

added more detail for test based on suggestion:

294fbee

refactored test

f3159b5

reformatted test file

6be34e0

zandercymatics reviewed Sep 18, 2024

View reviewed changes

asaki222 added 2 commits September 18, 2024 15:47

removed profile feature from docs

7a81210

removed profile feature from documentation

d8d3605

zandercymatics approved these changes Sep 18, 2024

View reviewed changes

asaki222 merged commit 9f300ef into main Sep 19, 2024
10 checks passed

asaki222 deleted the meoward/2591-Remove-Profile-feature-flag branch September 19, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2591: Remove profile feature flag - [MEOWARD] #2738

#2591: Remove profile feature flag - [MEOWARD] #2738

asaki222 commented Sep 10, 2024 •

edited

Loading

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

CocoByte left a comment

github-actions bot commented Sep 12, 2024

zandercymatics left a comment

zandercymatics Sep 17, 2024

asaki222 Sep 17, 2024

zandercymatics Sep 17, 2024

zandercymatics Sep 17, 2024

asaki222 Sep 18, 2024

zandercymatics Sep 17, 2024

zandercymatics Sep 17, 2024

asaki222 Sep 17, 2024

zandercymatics Sep 18, 2024 •

edited

Loading

zandercymatics Sep 18, 2024 •

edited

Loading

asaki222 Sep 18, 2024

github-actions bot commented Sep 17, 2024

asaki222 commented Sep 17, 2024

github-actions bot commented Sep 17, 2024

github-actions bot commented Sep 17, 2024

github-actions bot commented Sep 18, 2024

zandercymatics left a comment •

edited

Loading

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

zandercymatics left a comment •

edited

Loading

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

		home_page = self.app.get(reverse("user-profile"))
		self.assertContains(home_page, self.domain.name)

#2591: Remove profile feature flag - [MEOWARD] #2738

#2591: Remove profile feature flag - [MEOWARD] #2738

Conversation

asaki222 commented Sep 10, 2024 • edited Loading

Ticket

Changes

Context for reviewers

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

As a designer reviewer, I have

Verified that the changes match the design intention

Validated user-facing changes as a designer

Screenshots

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

CocoByte left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 12, 2024

zandercymatics left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zandercymatics Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

zandercymatics Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 17, 2024

asaki222 commented Sep 17, 2024

github-actions bot commented Sep 17, 2024

github-actions bot commented Sep 17, 2024

github-actions bot commented Sep 18, 2024

zandercymatics left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

zandercymatics left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

asaki222 commented Sep 10, 2024 •

edited

Loading

zandercymatics Sep 18, 2024 •

edited

Loading

zandercymatics Sep 18, 2024 •

edited

Loading

zandercymatics left a comment •

edited

Loading

zandercymatics left a comment •

edited

Loading