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

Fixes: #17360 - Ensure model is defined when rendering bulk_edit_button #17535

Merged
merged 4 commits into from
Sep 26, 2024

Conversation

bctiemann
Copy link
Contributor

@bctiemann bctiemann commented Sep 18, 2024

Fixes: #17360

Addresses a common issue in HTMX navigation where the inclusion of a call to bulk_edit_button where the param model is a string raises an AttributeError: 'str' object has no attribute '_meta'.

This fix adds a check for truthiness of model before trying to render {% bulk_edit_button %} or {% bulk_delete_button %} in table.py.

@bctiemann bctiemann self-assigned this Sep 18, 2024
Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

I'm not sure this is the correct fix (or might be a partial fix) as I think it might just be masking the underlying issue - if you do the scenario to go to the interface and then click the edit bulk edit button (without having any items selected) you are presented with an empty page instead of a toast as you are without HTMX navigation enabled and I think it might be because of this this change allowing a blank model.

I think (not sure) that we need to find why model isn't getting set and get it set correctly.

@bctiemann
Copy link
Contributor Author

Yeah, agreed -- this didn't really sit right with me except as a one-liner noninvasive fix. I don't yet quite understand what the underlying issue is or whether model is even supposed to be present, but I'll set this to Draft and keep digging.

@bctiemann bctiemann marked this pull request as draft September 19, 2024 16:36
@bctiemann
Copy link
Contributor Author

bctiemann commented Sep 19, 2024

@arthanson I think the blank page on "no interfaces selected" condition is a separate issue entirely; it has to do with the fact that it's an HTMX request that encounters an error and then gets a redirect to the return_url, but because it's in an HTMX container it doesn't properly send the browser back to that URL at the top level. I think that needs to be addressed in a separate bugfix.

return redirect(self.get_return_url(request))

To deal with the immediate issue, I've reverted the change to the template and set model explicitly in the page context, after investigating more and finding that that variable is being set in a lot of other pages (particularly list views) but not in this ObjectChildrenView. It seems to behave correctly now without having to catch the conditional in the template because model is not used except in the {% if request.htmx %} block.

@bctiemann bctiemann marked this pull request as ready for review September 19, 2024 18:29
@bctiemann
Copy link
Contributor Author

Note that the surface-level error (model being interpreted as a str) was because the construct {% bulk_edit_button model query_params=request.GET %} passes in an unset or missing model context variable as an empty string, because it's interpolated as nothing. It's not that model is actually a str, it's that it isn't present in the context at all and the template processor just swallows that condition by treating the missing param as a space in the tag.

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

The template htmx/table.html is used in several places; this needs to be addressed in all of them. It's probably sufficient to tweak the template to pass object to the bulk action button tags, rather than model.

@bctiemann
Copy link
Contributor Author

All the other occurrences of bulk_action_button are in list views though. Wouldn't those have an object_list rather than an object?

@bctiemann
Copy link
Contributor Author

Passing self.child_model (e.g. Interface) to the template instead of model (e.g. Device) seems like the right answer here. The success URL now pushes to the Interfaces tab as is correct.

@jeremystretch jeremystretch merged commit e93719e into develop Sep 26, 2024
6 checks passed
@jeremystretch jeremystretch deleted the 17360-htmx-fix-empty-model branch September 26, 2024 19:36
bctiemann added a commit that referenced this pull request Oct 11, 2024
…on (#17535)

* Ensure model is defined when rendering bulk_edit_button

* Move model check to inner conditional

* Set model in context

* Return child_model instead of model for use in bulk_edit_button
jeremystretch pushed a commit to alehaa/netbox that referenced this pull request Oct 11, 2024
…g bulk_edit_button (netbox-community#17535)

* Ensure model is defined when rendering bulk_edit_button

* Move model check to inner conditional

* Set model in context

* Return child_model instead of model for use in bulk_edit_button
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] netbox 4.0.11 and 4.1.0 error 500 while displaying the interfaces of a device
3 participants