Remove unnecessary fields from Register an App form BB2-2660 #1203

dull7295 · 2024-06-05T06:15:46Z

JIRA Ticket:
BB2-2660

User Story or Bug Summary:

As a Builder, I want to register an app quickly without seeing unnecessary fields, so that I can have a simplified developer experience

Implementation Details

Currently on the "register a new application" form there are two fields that only have one option.

OAuth - Client Type
Authorization Grant Type
There's no need to show these fields anymore since the options can't be changed, and we display the info for them on the success screen after registering an app

What Does This PR Do?

What Should Reviewers Watch For?

If you're reviewing this PR, please check these things, in particular:

TODO

What Security Implications Does This PR Have?

Submitters should complete the following questionnaire:

If the answer to any of the questions below is Yes, then here's a link to the associated Security Impact Assessment (SIA), security checklist, or other similar document in Confluence: N/A.
- Does this PR add any new software dependencies? Yes or No.
- Does this PR modify or invalidate any of our security controls? Yes or No.
- Does this PR store or transmit data that was not stored or transmitted before? Yes or No.
If the answer to any of the questions below is Yes, then please add a Security Engineer and ISSO as a reviewer, and note that this PR should not be merged unless/until he also approves it.
- Do you think this PR requires additional review of its security implications for other reasons? Yes or No.

What Needs to Be Merged and Deployed Before this PR?

This PR cannot be either merged or deployed until the following pre-requisite changes have been fully deployed:

CMSgov/some_repo#42

Any Migrations?

Yes, there are migrations
- The migrations should be run PRIOR to the code being deployed
- The migrations should be run AFTER the code is deployed
- There is a more complicated migration plan (downtime, etc)
No migrations

Submitter Checklist

I have gone through and verified that...:

This PR is reasonably limited in scope, to help ensure that:
1. It doesn't unnecessarily tie a bunch of disparate features, fixes, refactorings, etc. together.
2. There isn't too much of a burden on reviewers.
3. Any problems it causes have a small "blast radius".
4. It'll be easier to rollback if that becomes necessary.
I have named this PR and its branch such that they'll be automatically be linked to the (most) relevant Jira issue, per: https://confluence.atlassian.com/adminjiracloud/integrating-with-development-tools-776636216.html.
This PR includes any required documentation changes, including README updates and changelog / release notes entries.
All new and modified code is appropriately commented, such that the what and why of its design would be reasonably clear to engineers, preferably ones unfamiliar with the project.
All tech debt and/or shortcomings introduced by this PR are detailed in TODO and/or FIXME comments, which include a JIRA ticket ID for any items that require urgent attention.
Reviews are requested from both:
- At least two other engineers on this project, at least one of whom is a senior engineer or owns the relevant component(s) here.
- Any relevant engineers on other projects (e.g. BFD, SLS, etc.).
Any deviations from the other policies in the DASG Engineering Standards are specifically called out in this PR, above.
- Please review the standards every few months to ensure you're familiar with them.

loganbertram · 2024-06-11T14:43:21Z

apps/dot_ext/forms.py

+        #Validate choices
        if not (
            client_type == "confidential"
            and authorization_grant_type == "authorization-code"
        ):
-            validate_error = True
+            #validate_error = True
            msg += (
                "Only a confidential client and "
                "authorization-code grant type are allowed at this time."


I think you can cut this down through line 113, since you're hard-coding these values.

actually, 95-113 can go, really.

I cleaned up and pushed the changes.

loganbertram · 2024-06-11T14:45:07Z

apps/dot_ext/forms.py

+        authorization_grant_type = self.fields["authorization_grant_type"]
+        authorization_grant_type.widget = authorization_grant_type.hidden_widget()


I think you only need to make the fields optional and cut lines 65 and 66

Yes, I cleaned up and pushed the code.

jimmyfagan

Looks good, and looks like it works well from testing locally, just one more cleanup comment.

jimmyfagan · 2024-06-11T16:54:37Z

apps/dot_ext/forms.py

        client_type = self.cleaned_data.get("client_type")
        authorization_grant_type = self.cleaned_data.get("authorization_grant_type")


It doesn't seem like we need these vars to be set either now that the rest of the logic here is removed.

Suggested change

client_type = self.cleaned_data.get("client_type")

authorization_grant_type = self.cleaned_data.get("authorization_grant_type")

Yep! I'd second this. I think this was meant to get ripped out before. I'd say it's good to go otherwise.

Yes, I lately realized and cleaned up this and committed.

jimmyfagan

Looks good to me!

…s-from-Register-anAppform

jimmyfagan · 2024-06-13T21:03:52Z

apps/dot_ext/tests/test_form_application.py

@@ -258,33 +258,6 @@ def test_update_form_edit(self):
        form = CustomRegisterApplicationForm(user, passing_app_fields)
        self.assertTrue(form.is_valid())

-        # Test client_type = 'confidential' and authorization_grant_type = 'implicit' not allowed.


Wondering if we should be removing these tests or just reworking what goes on here. It could be a good test to still verify that regardless of what the form data says, we will save confidential and authorization-code. Would it make sense for the clean function to save those values to these fields in the cleaned_data and then to adjust these tests to instead of expecting an error, to expect that the cleaned_data shows what we want it to regardless of what was passed in? Part of that question is: should the values be hardcoded in clean, save, or both? @loganbertram hoping you can weigh in here too.

Yeah, I'd leave lines 89-90 in clean() and drop setting the values in save(). I feel like the "cleaning" being done is setting legacy values for fields that don't actually accept other inputs.

I think pulling the tests out makes sense. No way to change these values now and, in the future, if we accept other values, we would need to change this test anyway. I'd be ok with minimal changes too, but this whole testing mechanism doesn't need to exist.

@loganbertram I tried to move this logic to the clean method but it did not save the fields back to the database.
@jimmyfagan @loganbertram I think we can leave as is for now and do share your thoughts on it?

…s-from-Register-anAppform

jimmyfagan

This works for now, I might try to look more into why we couldn't make the change, but it's not worth delaying the functional changes you've made here. Approved!

Remove unnecessary fields from Register an App form BB2-2660

d008a0c

dull7295 marked this pull request as draft June 7, 2024 15:36

Assign the default values for hidden fields BB2-2660

e3ec59c

loganbertram reviewed Jun 11, 2024

View reviewed changes

Cleaned up the code BB2-2660

7213274

dull7295 assigned dull7295 and unassigned dull7295 Jun 11, 2024

dull7295 marked this pull request as ready for review June 11, 2024 15:18

dull7295 requested a review from loganbertram June 11, 2024 15:18

jimmyfagan reviewed Jun 11, 2024

View reviewed changes

Cleaned up the code BB2-2660

c26b992

jimmyfagan approved these changes Jun 12, 2024

View reviewed changes

loganbertram approved these changes Jun 12, 2024

View reviewed changes

dull7295 and others added 4 commits June 12, 2024 15:55

Merge branch 'master' into dull7295/BB2-2660-Remove-unnecessary-field…

5b8cffa

…s-from-Register-anAppform

Fixed Unit test cases and cleaned BB2-2660

48b4aa7

Fixed Unit test cases and cleaned BB2-2660

b807c60

Removed Blank Lines BB2-2660

8430ca7

jimmyfagan reviewed Jun 13, 2024

View reviewed changes

jimmyfagan requested review from jimmyfagan and loganbertram June 13, 2024 21:04

Merge branch 'master' into dull7295/BB2-2660-Remove-unnecessary-field…

702689f

…s-from-Register-anAppform

jimmyfagan approved these changes Jun 20, 2024

View reviewed changes

dull7295 merged commit 2b86b4f into master Jun 20, 2024
6 checks passed

dull7295 deleted the dull7295/BB2-2660-Remove-unnecessary-fields-from-Register-anAppform branch June 20, 2024 15:55

jimmyfagan mentioned this pull request Jun 22, 2024

BB2-2660: Move logic from save to clean for oauth client and grant type #1210

Draft

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary fields from Register an App form BB2-2660 #1203

Remove unnecessary fields from Register an App form BB2-2660 #1203

dull7295 commented Jun 5, 2024

loganbertram Jun 11, 2024

loganbertram Jun 11, 2024

dull7295 Jun 11, 2024

loganbertram Jun 11, 2024

dull7295 Jun 11, 2024

jimmyfagan left a comment

jimmyfagan Jun 11, 2024

loganbertram Jun 11, 2024

dull7295 Jun 12, 2024

jimmyfagan left a comment

jimmyfagan Jun 13, 2024

loganbertram Jun 14, 2024

loganbertram Jun 14, 2024

dull7295 Jun 18, 2024

jimmyfagan left a comment

		authorization_grant_type = self.fields["authorization_grant_type"]
		authorization_grant_type.widget = authorization_grant_type.hidden_widget()

		client_type = self.cleaned_data.get("client_type")
		authorization_grant_type = self.cleaned_data.get("authorization_grant_type")

Remove unnecessary fields from Register an App form BB2-2660 #1203

Remove unnecessary fields from Register an App form BB2-2660 #1203

Conversation

dull7295 commented Jun 5, 2024

What Does This PR Do?

What Should Reviewers Watch For?

What Security Implications Does This PR Have?

What Needs to Be Merged and Deployed Before this PR?

Any Migrations?

Submitter Checklist

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

jimmyfagan 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

jimmyfagan 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

jimmyfagan left a comment

Choose a reason for hiding this comment