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.

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

Uncrispyfy DeviceGroupForm in seeddb #2994

Merged
merged 5 commits into from
Sep 20, 2024
Merged

Conversation

podliashanyk
Copy link
Contributor

@podliashanyk podliashanyk commented Sep 19, 2024

Closes #2988

Introduces extra pattern - uncrispyfying of the required fields.

@podliashanyk podliashanyk requested a review from a team September 19, 2024 16:03
@podliashanyk podliashanyk self-assigned this Sep 19, 2024
@CLAassistant
Copy link

CLAassistant commented Sep 19, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

github-actions bot commented Sep 19, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 987 0 23.75s
✅ PYTHON flake8 987 0 447.87s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

Copy link

github-actions bot commented Sep 19, 2024

Test results

    9 files      9 suites   8m 57s ⏱️
3 330 tests 3 330 ✅ 0 💤 0 ❌
6 399 runs  6 399 ✅ 0 💤 0 ❌

Results for commit 7fe2f74.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.59%. Comparing base (807deb4) to head (7fe2f74).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2994      +/-   ##
==========================================
+ Coverage   56.30%   56.59%   +0.28%     
==========================================
  Files         602      602              
  Lines       43728    43728              
  Branches       48       48              
==========================================
+ Hits        24622    24746     +124     
+ Misses      19094    18970     -124     
  Partials       12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@podliashanyk
Copy link
Contributor Author

Seems like edit- and add-forms in seeddb/* use crispy_forms (what to me seems like) implicitly. They are not mentioned in #2794, or am I missing some django/nav logic where those will be fixed by fixing some other templates mentioned in #2794?

Anyway, with _form_fields.html most of fields in all add/edit forms in /seeddb can be fixed in a separate 1 liner PR.
Very few fields will not be fixed universaly and will require some extra logic or tweaks. I so far noticed only select2 fields not being universaly converted with _form_fields.html. Will investigate and report more tomorrow.

@podliashanyk podliashanyk marked this pull request as ready for review September 19, 2024 16:17
@podliashanyk podliashanyk changed the title Uncrispyfy/device group form Uncrispyfy DeviceGroupForm Sep 19, 2024
@podliashanyk podliashanyk changed the title Uncrispyfy DeviceGroupForm Uncrispyfy DeviceGroupForm in seeddb Sep 19, 2024
Is mostly universal, yet fields that require extra classes (like f.e. select2) need extra logic.
Meaning that with few tweaks this template has a potential to fix all add/edit templates in /seeddb.
Will finish and showcase this in a separate PR

fixup
@podliashanyk podliashanyk force-pushed the uncrispyfy/device-group-form branch from 6af5ca9 to 25ce6c1 Compare September 20, 2024 07:27
@@ -372,6 +372,8 @@ class DeviceGroupForm(forms.ModelForm):
netboxes = forms.ModelMultipleChoiceField(
queryset=Netbox.objects.all(), required=False
)
netboxes.widget.attrs.update({"class": "select2"})
no_crispy = True
Copy link
Contributor

@hmpf hmpf Sep 20, 2024

Choose a reason for hiding this comment

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

When crispy is gone we will no longer need "no_crsipy" for choosing which form to show. It is still useful to hold extra form attributes though, like with the filter forms.

Copy link
Contributor Author

@podliashanyk podliashanyk Sep 20, 2024

Choose a reason for hiding this comment

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

Yes, we can remove no_crispy = True completely once all edit/add forms i seeddb are converted. Because then we can safely remove the {% if form.no_crispy %}-block from the seeddb/edit.html that was added in this PR and render {% include "seeddb/_form_fields.html" %} by default there

@hmpf
Copy link
Contributor

hmpf commented Sep 20, 2024

Are you considerimg adding django-widget-tweaks for the special casing?

@podliashanyk
Copy link
Contributor Author

podliashanyk commented Sep 20, 2024

Are you considerimg adding django-widget-tweaks for the special casing?

I was really tempted to add it while working on this one and investigating the situation with other edit/add forms in seeddb, but there was not overwhelmingly definite need so far. But will see how it goes with conversion of other fields, might happen that django-widget-tweaks will be necessary there

Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

Only a small cosmetic suggestion

python/nav/web/templates/seeddb/_form_fields.html Outdated Show resolved Hide resolved
@podliashanyk podliashanyk merged commit 46c253f into master Sep 20, 2024
12 of 13 checks passed
@lunkwill42 lunkwill42 deleted the uncrispyfy/device-group-form branch September 20, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncrispyfy python/nav/web/seeddb/forms/__init__.py:DeviceGroupForm
4 participants