Location select widget - filter by whether the location "has users" #34912
Conversation
AddisonDunn
added
the
product/feature-flag
AddisonDunn
requested review from
esoergel,
orangejenny and
mkangia
as code owners
|
Just remembered backend validation is needed, bear with me |
dimagimon
added
the
Risk: High
| delete_all_users() | ||
| return super().tearDownClass() | ||
|
|
||
| @mock.patch('django_prbac.decorators.has_privilege', return_value=True) |
There was a problem hiding this comment.
FWIW there's also a @privilege_enabled decorator you can use for this to target a specific privilege
| url(r'^location_search_has_users_only/$', LocationsSearchView.as_view( | ||
| include_locations_with_no_users_allowed=False), name='location_search_has_users_only'), |
There was a problem hiding this comment.
What would you think of naming this after the place it's used rather than what distinguishes it from the other endpoint? We'll also soon be excluding already-assigned locations from the results, so it may see further tweaking.
There was a problem hiding this comment.
I see from a later commit that the plan is to switch between endpoints depending on the desired functionality. Would it be possible to make this enpdoint specific to the location assignment widget and then move the feature flag check there? Otherwise further changes would need to account for both endpoints.
There was a problem hiding this comment.
Otherwise further changes would need to account for both endpoints
Why would this be the case? Are you imagining the new needed search criteria with the upcoming location select widget work would be passed as URL parameters? Or maybe your are thinking they could be passed directly to the view, ah. I was thinking they would be passed in the request body. If that's the approach we want to take I am open to that, but maybe changes like you're proposing belong as part of that scope of workk.
There was a problem hiding this comment.
Ah, I see that it's still the same view in both cases, but with a different initialization parameter here, I was thinking it was a subclass for some reason. I still think there may be a place for making a UserLocationWidgetView and putting the logic there rather than passing it around, but that will probably make more sense with the next bit of deviation from the standard use case than it does now.
There was a problem hiding this comment.
I'm struggling a bit to articulate what I'm thinking, but let me try. Essentially, there's a generic "locations select widget" that is used in several different places. This is one customization specific to user location assignment, and we've got a couple more customizations likely coming. The way it's done here is to set a flag on the widget, which directs to a different URL, which sets a flag on the view, which sets a flag on the controller, which forks the behavior in the controller's parent class. That's a lot of steps, making it perhaps challenging to see when/how/why the flag is set to what it is.
The alternative I'm imagining would be to switch from the generic widget to one that's specific to user location assignment. Then it's clear in what context the behavior differs. Perhaps that'd still use the same system of passing flags around, only the flag would be called "is user location assignment" or some such. Then when we fork behavior, we could do it all under the same flag or subclass, making the scope under which those changes apply clearer.
Again I don't think that's necessarily better, certainly not with just this logic change. Mostly leaving my thoughts here now because I suspect it'll start to be more compelling as we further fork the logic (though it's always hard to tell for sure without trying it).
From the article Simon shared this morning:
When designing abstractions, do not prematurely couple behaviours that may evolve separately in the longer term.
Here the widget for picking locations for a user is coupled to the generic widget for finding locations on a project, and I'm thinking that they are starting to evolve separately, so we may want to decouple them (though perhaps the enriched context actually should benefit all instances).
There was a problem hiding this comment.
I gotcha. Yeah, what you're saying makes sense.
switch from the generic widget to one that's specific to user location assignment
You mean extending the class to make one thats for assignment to users, right? I agree doing that instead of the "first" flag that's passed through would help clarify to the dev what it's used for/how they should use it. I could also see forking the search view as you're suggesting, into something like UserSelectLocationOptionsView or something like that. I think that's the feel of what you're getting at, right?
There was a problem hiding this comment.
Yeah, something like that, whichever seems cleaner. Like maybe it's just a flag in the widget and a subclass for the options view, not sure. But yeah, some way to tie it to the specific usage rather than the behavior change from the "normal" way.
| @@ -1141,7 +1141,7 @@ def render(self, name, value, attrs=None, renderer=None): | |||
| }) | |||
|
|
|||
|
|
|||
| class BaseLocationForm(forms.Form): | |||
| class SelectUserLocationForm(forms.Form): | |||
| @@ -72,7 +72,7 @@ def schedule_user_organization_recipients_response(self): | |||
| def _get_user_organization_response(self, case_sharing_only=False): | |||
| from corehq.apps.locations.views import LocationOptionsController | |||
| controller = LocationOptionsController(self.request, self.request.domain, | |||
| self.data.get('searchString'), False, | |||
|
Thanks @Jtang-1 ! |
Product Description
Jira.
Part of an epic to add a "has users" setting to organization levels. This particular PR filters the options in the location select dropdown on edit user pages by locations who are allowed to have users.
Changes are behind a FF until the feature's official release.
Technical Summary
Adding an optional argument to the URL for a single boolean felt kinda wrong, I could also have passed a parameter from the widget page, but took the approach of just adding another URL. Open to feedback there.
Feature Flag
LOCATION_HAS_USERS
Safety Assurance
Safety story
Automated test coverage
corehq.apps.locations.tests.test_views.LocationSearchViewTest
QA Plan
QA will be done at the end of this feature's dev
Rollback instructions
Labels & Review