-
-
Notifications
You must be signed in to change notification settings - Fork 54
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 suggestion: Add ModelSelect2Mixin.result_from_instance() method. #272
Conversation
db22be6
to
e8f374c
Compare
Oh, and the other use case would be improving performance by using class MyModelWidget(ModelSelect2Widget):
search_fields = ["name"]
model = Product
def filter_queryset(*args, **kwargs):
return super().filter_queryset(*args, **kwargs).values("pk", "name")
def result_from_instance(self, obj):
return {"id": obj["pk"], "text": obj["name"]} |
Hi @sdolemelipone 👋 Thank you for reaching out. I think you are describing a rather common scenario. I always went ahead and wrote custom views that accompany my widgets. However, I do see a point in using reflection, to only write a widget that includes all extension. Usually, I'd appreciate a discussion or ticket first, but let's move this along anyway. I didn't do a full review yet, but off the top of my head: Please add the request to the methods context (needed to construct fully qualified URLs or permission management). I also need to be cautious with state leakage. Since the widget is passed between requests, modifications to the widget instance, might propagate to other requests. Cheers! |
Hi @codingjoe, thanks for taking the time to look at my suggestion. Noted, next time I'll start with a ticket - I found writing the PR interesting so at least no wasted effort on my part there :-). I'll add the request to the function arguments as you suggested. Regarding the state leakage, this seems like a problem a user could introduce if they did something unusual in result_from_instance(), which involved previous calls to the method - or is there something I'm missing? Is it something that needs to be mentioned in the documentation? Thanks |
Hi @codingjoe, |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #272 +/- ##
==========================================
+ Coverage 98.21% 98.22% +0.01%
==========================================
Files 7 7
Lines 280 282 +2
==========================================
+ Hits 275 277 +2
Misses 5 5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Thanks 🥇
cc @syphar |
Great 😃 thanks @codingjoe. |
It would be useful to have a way to override or add additional data to the results returned by AutoResponseView for particular widgets.
The use case is processing the
select2:select
jQuery event on the client side and updating another part of the web page with the related object information. E.g. a ModelChoiceField selects a Product object, and the product's price is rendered alongside the form.Let me know if this pull request looks acceptable.
Thanks