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

[Feature Branch] Implemented SavedView model #5639

Merged
merged 47 commits into from
May 18, 2024
Merged

[Feature Branch] Implemented SavedView model #5639

merged 47 commits into from
May 18, 2024

Conversation

HanlinMiao
Copy link
Contributor

@HanlinMiao HanlinMiao commented Apr 26, 2024

Closes #1758

What's Changed

Screenshots

Implemented Saved View Model
SavedView List View
Screenshot 2024-05-01 at 5 07 28 PM
SavedView Dropdown List
Screenshot 2024-05-01 at 5 09 28 PM
SavedView modal form
Screenshot 2024-05-01 at 5 09 40 PM

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example App Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

changes/1758.dependencies Outdated Show resolved Hide resolved
nautobot/core/models/__init__.py Outdated Show resolved Hide resolved
nautobot/core/tables.py Outdated Show resolved Hide resolved
nautobot/core/tables.py Outdated Show resolved Hide resolved
nautobot/core/templates/generic/object_list.html Outdated Show resolved Hide resolved
nautobot/users/models.py Outdated Show resolved Hide resolved
nautobot/users/navigation.py Outdated Show resolved Hide resolved
nautobot/users/tables.py Outdated Show resolved Hide resolved
nautobot/users/templates/users/advanced_settings_edit.html Outdated Show resolved Hide resolved
nautobot/users/views.py Outdated Show resolved Hide resolved
@HanlinMiao
Copy link
Contributor Author

Will have tests and documentation included in another PR

nautobot/core/models/generics.py Outdated Show resolved Hide resolved
nautobot/users/tables.py Outdated Show resolved Hide resolved
nautobot/core/tables.py Outdated Show resolved Hide resolved
nautobot/core/templates/generic/object_list.html Outdated Show resolved Hide resolved
nautobot/core/templates/inc/search_panel.html Show resolved Hide resolved
nautobot/users/views.py Outdated Show resolved Hide resolved
nautobot/users/views.py Outdated Show resolved Hide resolved
nautobot/users/models.py Outdated Show resolved Hide resolved
nautobot/users/views.py Outdated Show resolved Hide resolved
nautobot/users/views.py Outdated Show resolved Hide resolved
nautobot/core/templates/generic/object_list.html Outdated Show resolved Hide resolved
nautobot/core/templatetags/helpers.py Outdated Show resolved Hide resolved
Comment on lines 316 to 318
current_saved_view = SavedView.objects.restrict(request.user, "view").get(
view=list_url, pk=current_saved_view_pk
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's needed for handling in case the requested view doesn't exist or can't be accessed by the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be a reasonable extra layer of protection? Worst case scenario current_saved_view would be None

try:
     current_saved_view = SavedView.objects.restrict(request.user, "view").get(view=list_url, pk=current_saved_view_pk)
except ObjectDoesNotExist:
      pass

Copy link
Contributor

Choose a reason for hiding this comment

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

Worth a try - hard to say for sure without trying it myself. At the very least might be good to do a messages.error(request, "Saved view <pk> not found") or similar?

nautobot/core/views/renderers.py Outdated Show resolved Hide resolved
nautobot/users/models.py Show resolved Hide resolved
nautobot/users/templates/users/advanced_settings_edit.html Outdated Show resolved Hide resolved
<li>
<a href="{% url 'user:savedview' pk=request.GET.saved_view %}">
<i class="mdi mdi-arrow-u-right-bottom text-danger" aria-hidden="true"></i>
<span class="text-danger">Clear View</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

This, I assume, is used to undo all unsaved modifications to the view and restore the UI to its initial view state?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so I don't think Clear View might be the right wording here

Copy link
Contributor

Choose a reason for hiding this comment

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

This actually updates the saved view to make it have no filters/tableconfig/etc. I'm still not convinced this is necessary/desirable functionality, personally.

)

def get_filter_params(self, request):
"""Helper function - take request.GET and discard any parameters that are not used for queryset filtering."""
filter_params = request.GET.copy()
print(filter_params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

Comment on lines +307 to +309
saved_views = (
SavedView.objects.filter(view=list_url).restrict(request.user, "view").order_by("name").only("pk", "name")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

illd suggest this?:

saved_views = (
             SavedView.objects.filter(view=list_url)
             .restrict(request.user, "view")
         )

then

Line 317

current_saved_view = saved_views.get(pk=current_saved_view_pk)

and
Line 370

 "saved_views": saved_views.order_by("name").only("id", "name")

Might help in reusing code

glennmatthews and others added 4 commits May 8, 2024 10:38
…g, config, and pagination values (#5682)

* Make 'saved_view=...' query param imply the correct filtering, sorting, config, and pagination values

* Fix update behavior and create behavior when given an existing instance

* Test fix
* added factories and some documentation

* added api and view tests

* filter test and ruff fix

* include savedview.md in nav

* delete print statement

* user factory modifications

* fix user unittest

* delete comment

* address PR feedback

* add additional tests

* factory changes

* delete output.txt

* added saved view clear view

* added all_filters_removed non_filter_params

* added tooltip title to asterik

* added user guide for creating and updating saved view

* added refereneces to user guide

* completed user guide

* refactor create saved view

* address some pr feedback

* refactored SavedViewModal

* attempt to fix unittests

* fix screenshots

* specify is_saved_view_model=False on ComponentTemplateModel

* added clear_view_modal

* fixed saved view edit url

* address pr feedback on user guide

* fix all unittests

* markdownlint

* fix unittest

* ruff

* fix factory and tests

* refactored SavedViewClearView to custom action on SavedViewUIViewSet

* address PR feedback

* ruff fix

* pylint and unittest fixes

* fix unittest

* address pr feedabck

* refactor and remove is_saved_view_model=False on StaticGroup

* ruff fix

* unittests

* Enforce users:change_savedview instead of users:clear_savedview

* pylint
nautobot/core/jobs/__init__.py Outdated Show resolved Hide resolved
nautobot/core/models/generics.py Outdated Show resolved Hide resolved
<li>
<a href="{% url 'user:savedview' pk=request.GET.saved_view %}">
<i class="mdi mdi-arrow-u-right-bottom text-danger" aria-hidden="true"></i>
<span class="text-danger">Clear View</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually updates the saved view to make it have no filters/tableconfig/etc. I'm still not convinced this is necessary/desirable functionality, personally.

nautobot/core/templates/generic/object_list.html Outdated Show resolved Hide resolved
nautobot/core/templates/generic/object_list.html Outdated Show resolved Hide resolved
nautobot/core/views/mixins.py Outdated Show resolved Hide resolved
Comment on lines 273 to 278
saved_views = (
SavedView.objects.filter(view=list_url)
.restrict(request.user, "view")
.order_by("name")
.only("pk", "name")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

can/should we gate this by model.is_saved_view_model?

Comment on lines +317 to +319
config = models.JSONField(
encoder=DjangoJSONEncoder, blank=True, default=dict, help_text="Saved Configuration on this view"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a custom clean() implementation that enforces any particular structure on the config data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure this is feasible right now because our factory is using randomized pydict data. All those would fail to be created if I assert any structures on the config data.

nautobot/users/models.py Outdated Show resolved Hide resolved
nautobot/users/views.py Outdated Show resolved Hide resolved
"""
Find all models that have fields with the specified names, and return them grouped by app.
Find all models that have fields with the specified names and/or satisfy the additional constraints when the field names are not available,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Find all models that have fields with the specified names and/or satisfy the additional constraints when the field names are not available,
Find all models that have fields with the specified names and satisfy the additional constraints,

messages.success(request, f"Successfully updated current view {sv.name}")
return redirect(list_view_url)

def create(self, request, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create a common helper function for the shared bits of logic between create, update views as well as perhaps saved_view_modal and view_changes_not_saved? Just begins to seem like a lot of repetition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that there is a lot of repeated logic but they are all subtly different in some ways and need a lot of special case handling. I can open a follow-up issue for this.

@HanlinMiao HanlinMiao merged commit b9f5b38 into next May 18, 2024
17 checks passed
@HanlinMiao HanlinMiao deleted the u/hanlin-#1758 branch May 18, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants