Use server side search for user fields#2677
Conversation
richardebeling
left a comment
richardebeling
left a comment
There was a problem hiding this comment.
Looks good, thanks. My biggest concern is that we need to find some descriptive terminology around this (and tests), otherwise just nits. Marking as draft until we have tests.
| self.fields["contributor"].queryset = self.get_contributor_queryset() | UserProfile.objects.filter( | ||
| pk=self.instance.contributor.pk | ||
| ) | ||
| self.fields["contributor"].widget.search_url = reverse("staff:fetch_contributor_user_profiles") |
There was a problem hiding this comment.
Does this mean that if you accidentally remove someone from the selection, you'll not be able to add them back?
How does TomSelect handle a mixture of HTML-supplied and server-supplied options? Does merge the server-options with the HTML options on an update, or will the HTML options be overwritten? If the former, maybe we can remove a large part of the specialization logic we have for "keep allowing existing participants" etc.
There was a problem hiding this comment.
The HTML options don't get overriden.
There was a problem hiding this comment.
Does this work then?
maybe we can remove a large part of the specialization logic we have for "keep allowing existing participants" etc.
I would imagine that if we just provide all current participants as in-HTML options, we can simplify the view logic drastically, leading to cacheability etc
There was a problem hiding this comment.
We need frontend tests that ensure this works as expected.
|
I think this is very cool! I didn't have time to look at it closely yet, but now I will let you settle Richard's comments first :) |
hansegucker
force-pushed
the
server-side-user-search
branch
from
1e35d6f to
1398755
Compare
| def get_delegates(self): | ||
| return self.exclude(is_active=False).exclude(is_proxy_user=True) |
There was a problem hiding this comment.
Those are not "delegates" as defined in https://github.com/e-valuation/EvaP/wiki/Terminology -- should we just name it get_delegate_options?
| return redirect("evaluation:index") | ||
|
|
||
|
|
||
| class UserProfileOptionsBaseView(View): |
There was a problem hiding this comment.
What are the access permissions here? Would it suffice to only make this available to staff and editors (or maybe contributors)? Can we limit the query to contain at least three letters?
| def get_participants_queryset(cls, evaluation: Evaluation | None) -> QuerySet[UserProfile]: | ||
| queryset = UserProfile.objects.exclude(is_active=False) | ||
| if evaluation is not None and evaluation.pk is not None: | ||
| queryset = (queryset | evaluation.participants.all()).distinct() |
There was a problem hiding this comment.
I had hoped we would not need this anymore (by just passing the initial options in HTML) -- why is this still needed here?
| fetch(url) | ||
| .then(response => response.json()) | ||
| .then(json => { | ||
| callback(json.options); | ||
| }).catch(() => { | ||
| alert(window.gettext("The server is not responding.")); | ||
| }); |
There was a problem hiding this comment.
Why are we using a braced body here once? Could we just do this?
| fetch(url) | |
| .then(response => response.json()) | |
| .then(json => { | |
| callback(json.options); | |
| }).catch(() => { | |
| alert(window.gettext("The server is not responding.")); | |
| }); | |
| fetch(url) | |
| .then(response => response.json()) | |
| .then(json => callback(json.options)) | |
| .catch(() => alert(window.gettext("The server is not responding."))); |
|
|
||
|
|
||
| @staff_permission_required | ||
| class ParticipantsUserProfileSearchView(UserProfileOptionsBaseView): |
There was a problem hiding this comment.
OptionsView instead of SearchView? (Here and below)
hansegucker
force-pushed
the
server-side-user-search
branch
from
d69cc52 to
a6a4a08
Compare
hansegucker
force-pushed
the
server-side-user-search
branch
from
a6a4a08 to
10719c9
Compare
hansegucker
force-pushed
the
server-side-user-search
branch
from
10719c9 to
23734a6
Compare
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
("reviewing" to get this out of the queue, feel free to re-request)
3 participants
Close #1681