Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jun 20, 2024
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d008a0c
Remove unnecessary fields from Register an App form BB2-2660
Jun 5, 2024
e3ec59c
Assign the default values for hidden fields BB2-2660
Jun 11, 2024
7213274
Cleaned up the code BB2-2660
Jun 11, 2024
c26b992
Cleaned up the code BB2-2660
Jun 12, 2024
5b8cffa
Merge branch 'master' into dull7295/BB2-2660-Remove-unnecessary-field…
dull7295 Jun 12, 2024
48b4aa7
Fixed Unit test cases and cleaned BB2-2660
Jun 13, 2024
b807c60
Fixed Unit test cases and cleaned BB2-2660
Jun 13, 2024
8430ca7
Removed Blank Lines BB2-2660
Jun 13, 2024
702689f
Merge branch 'master' into dull7295/BB2-2660-Remove-unnecessary-field…
dull7295 Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 4 additions & 23 deletions apps/dot_ext/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ def __init__(self, user, *args, **kwargs):
self.fields["name"].label = "Name*"
self.fields["name"].required = True
self.fields["client_type"].label = "Client Type*"
self.fields["client_type"].required = False
self.fields["authorization_grant_type"].label = "Authorization Grant Type*"
self.fields["authorization_grant_type"].required = False
self.fields["redirect_uris"].label = "Redirect URIs*"
self.fields["logo_uri"].disabled = True

Expand All @@ -86,29 +88,6 @@ class Meta:
required_css_class = "required"

def clean(self):
client_type = self.cleaned_data.get("client_type")
authorization_grant_type = self.cleaned_data.get("authorization_grant_type")

msg = ""
validate_error = False

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

if validate_error:
msg_output = _(msg)
raise forms.ValidationError(msg_output)
else:
pass

return self.cleaned_data

def clean_name(self):
Expand Down Expand Up @@ -176,6 +155,8 @@ def clean_require_demographic_scopes(self):
return require_demographic_scopes

def save(self, *args, **kwargs):
self.instance.client_type = "confidential"
self.instance.authorization_grant_type = "authorization-code"
app = self.instance
# Only log agreement from a Register form
if app.agree and type(self) == CustomRegisterApplicationForm:
Expand Down
27 changes: 0 additions & 27 deletions apps/dot_ext/tests/test_form_application.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

data = passing_app_fields
data['client_type'] = 'confidential'
data['authorization_grant_type'] = 'implicit'
form = CustomRegisterApplicationForm(user, data)
self.assertFalse(form.is_valid())
self.assertIn('Only a confidential client and authorization-code grant type are allowed at this time.',
str(form.errors.get('__all__')))

# Test client_type = 'public' and grant_type = 'authorization-code' not allowed.
data = passing_app_fields
data['client_type'] = 'public'
data['authorization_grant_type'] = 'authorization-code'
form = CustomRegisterApplicationForm(user, data)
self.assertFalse(form.is_valid())
self.assertIn('Only a confidential client and authorization-code grant type are allowed at this time.',
str(form.errors.get('__all__')))

# Test client_type = 'public' and grant_type = 'implicit' not allowed.
data = passing_app_fields
data['client_type'] = 'public'
data['authorization_grant_type'] = 'implicit'
form = CustomRegisterApplicationForm(user, data)
self.assertFalse(form.is_valid())
self.assertIn('Only a confidential client and authorization-code grant type are allowed at this time.',
str(form.errors.get('__all__')))

def test_create_applications_with_logo(self):
"""
regression test: BB2-66: Fix-logo-display-in-Published-Applications-API
Expand Down
8 changes: 4 additions & 4 deletions templates/include/app-form-required-info.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,16 @@ <h2>App Details - Required Info</h2>
<input type="text" name="name" maxlength="255" required="" id="id_name" value="{{ application.name }}" autocomplete="turn_autocomplete_off">

<!-- App Client Type -->
<label for="id_client_type">OAuth - Client Type</label>
<!-- <label for="id_client_type">OAuth - Client Type</label>
<select name="client_type" required="" id="id_client_type">
<option value="confidential" {% if application.client_type != "public" %} selected {% endif %}>Confidential</option>
</select>
</select> -->

<!-- App Grant Type -->
<label for="id_authorization_grant_type">Authorization Grant Type*</label>
<!-- <label for="id_authorization_grant_type">Authorization Grant Type*</label>
<select name="authorization_grant_type" id="id_authorization_grant_type">
<option value="authorization-code" {% if application.authorization_grant_type != "implicit" %} selected {% endif %}>Authorization code</option>
</select>
</select> -->

<!-- App Redirect URIs -->
<label for="id_redirect_uris">Callback URLs / Redirect URIs</label>
Expand Down
Loading