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.

Sign up for GitHub

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

Location select widget - filter by whether the location "has users" #34912

Merged
merged 10 commits into from
Aug 2, 2024
Merged

Location select widget - filter by whether the location "has users" #34912

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2e28575
create basic test for LocationSearchView
AddisonDunn Jul 25, 2024
21c66e5
add url for getting locations with has_users=True only
AddisonDunn Jul 25, 2024
2135843
lint
AddisonDunn Jul 25, 2024
258745c
allow filtering by has_users in location select widget
AddisonDunn Jul 25, 2024
e7bec3b
change form name to clarify it is for selecting locations for a user
AddisonDunn Jul 25, 2024
238bc7a
backend validation that locations can have users
AddisonDunn Jul 25, 2024
c63458e
make arg a kwarg, its simpler
AddisonDunn Jul 29, 2024
86f906a
make test name the exact same as view name
AddisonDunn Jul 30, 2024
ba79133
merge conflicts
AddisonDunn Jul 30, 2024
9bf39ed
lint
AddisonDunn Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions corehq/apps/locations/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

from dimagi.utils.couch.database import iter_docs

from corehq import toggles
from corehq.apps.custom_data_fields.edit_entity import (
CUSTOM_DATA_FIELD_PREFIX,
CustomDataEditor,
Expand Down Expand Up @@ -43,13 +44,17 @@


class LocationSelectWidget(forms.Widget):
def __init__(self, domain, attrs=None, id='supply-point', multiselect=False, placeholder=None):
def __init__(self, domain, attrs=None, id='supply-point', multiselect=False, placeholder=None,
include_locations_with_no_users_allowed=True):
super(LocationSelectWidget, self).__init__(attrs)
self.domain = domain
self.id = id
self.multiselect = multiselect
self.placeholder = placeholder
self.query_url = reverse('location_search', args=[self.domain])
url_name = 'location_search'
if not include_locations_with_no_users_allowed and toggles.LOCATION_HAS_USERS.enabled(self.domain):
url_name = 'location_search_has_users_only'
self.query_url = reverse(url_name, args=[self.domain])
self.template = 'locations/manage/partials/autocomplete_select_widget.html'

def render(self, name, value, attrs=None, renderer=None):
Expand Down
70 changes: 69 additions & 1 deletion corehq/apps/locations/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,12 @@
from corehq.apps.es.users import user_adapter
from corehq.apps.locations.exceptions import LocationConsistencyError
from corehq.apps.locations.models import LocationType
from corehq.apps.locations.tests.util import make_loc
from corehq.apps.locations.views import LocationTypesView
from corehq.apps.users.models import WebUser
from corehq.util.test_utils import flag_enabled
from corehq.apps.users.dbaccessors import delete_all_users
from corehq.apps.users.models import WebUser


OTHER_DETAILS = {
'expand_from': None,
Expand Down Expand Up @@ -152,3 +155,68 @@ def test_invalid_remove_has_users(self, _):
data = {'loc_types': [loc_type1, loc_type2]}
with self.assertRaises(LocationConsistencyError):
self.send_request(data)


class LocationSearchViewTest(TestCase):
@classmethod
def setUpClass(cls):
super(LocationSearchViewTest, cls).setUpClass()
delete_all_users()
cls.domain = "test-domain"
cls.project = create_domain(cls.domain)
cls.username = "request-er"
cls.password = "foobar"
cls.web_user = WebUser.create(cls.domain, cls.username, cls.password, None, None)
cls.web_user.add_domain_membership(cls.domain, is_admin=True)
cls.web_user.set_role(cls.domain, "admin")
cls.web_user.save()
cls.loc_type1 = LocationType(domain=cls.domain, name='type1', code='code1')
cls.loc_type1.save()
cls.loc1 = make_loc(
'loc_1', type=cls.loc_type1, domain=cls.domain
)
cls.loc2 = make_loc(
'loc_2', type=cls.loc_type1, domain=cls.domain
)

def setUp(self):
self.client.login(username=self.username, password=self.password)

@classmethod
def tearDownClass(cls):
cls.project.delete()
delete_all_users()
return super().tearDownClass()

@mock.patch('django_prbac.decorators.has_privilege', return_value=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW there's also a @privilege_enabled decorator you can use for this to target a specific privilege

def send_request(self, url, data, _):
return self.client.get(url, {'json': json.dumps(data)})

def test_search_view_basic(self):
url = reverse('location_search', args=[self.domain])
data = {'q': 'loc'}
response = self.send_request(url, data)
self.assertEqual(response.status_code, 200)
results = json.loads(response.content)['results']
self.assertEqual(results[0]['id'], self.loc1.location_id)
self.assertEqual(results[1]['id'], self.loc2.location_id)

@flag_enabled('LOCATION_HAS_USERS')
def test_search_view_has_users_only(self):
loc_type2 = LocationType(domain=self.domain, name='type2', code='code2')
loc_type2.has_users = False
loc_type2.save()
self.loc3 = make_loc(
'loc_3', type=loc_type2, domain=self.domain
)
self.loc3 = make_loc(
'loc_4', type=loc_type2, domain=self.domain
)
url = reverse('location_search_has_users_only', args=[self.domain])
data = {'q': 'loc'}
response = self.send_request(url, data)
self.assertEqual(response.status_code, 200)
results = json.loads(response.content)['results']
self.assertEqual(len(results), 2)
self.assertEqual(results[0]['id'], self.loc1.location_id)
self.assertEqual(results[1]['id'], self.loc2.location_id)
2 changes: 2 additions & 0 deletions corehq/apps/locations/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
url(r'^$', default, name='default_locations_view'),
url(r'^list/$', LocationsListView.as_view(), name=LocationsListView.urlname),
url(r'^location_search/$', LocationsSearchView.as_view(), name='location_search'),
url(r'^location_search_has_users_only/$', LocationsSearchView.as_view(
include_locations_with_no_users_allowed=False), name='location_search_has_users_only'),
Comment on lines +35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@AddisonDunn AddisonDunn Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@esoergel esoergel Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

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 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

url(r'^location_types/$', LocationTypesView.as_view(), name=LocationTypesView.urlname),
url(r'^import/$', waf_allow('XSS_BODY')(LocationImportView.as_view()), name=LocationImportView.urlname),
url(r'^import_status/(?P<download_id>(?:dl-)?[0-9a-fA-Z]{25,32})/$', LocationImportStatusView.as_view(),
Expand Down
8 changes: 7 additions & 1 deletion corehq/apps/locations/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,10 @@ class LocationOptionsController(EmwfOptionsController):
namespace_locations = False
case_sharing_only = False

def __init__(self, *args, include_locations_with_no_users_allowed=True):
self.include_locations_with_no_users_allowed = include_locations_with_no_users_allowed
super().__init__(*args)

@property
def data_sources(self):
return [
Expand All @@ -270,11 +274,13 @@ def data_sources(self):
@method_decorator(locations_access_required, name='dispatch')
@location_safe
class LocationsSearchView(EmwfOptionsView):
include_locations_with_no_users_allowed = True
Jtang-1 marked this conversation as resolved.
Show resolved Hide resolved

@property
@memoized
def options_controller(self):
return LocationOptionsController(self.request, self.domain, self.search)
return LocationOptionsController(self.request, self.domain, self.search,
include_locations_with_no_users_allowed=self.include_locations_with_no_users_allowed)


@method_decorator(use_bootstrap5, name='dispatch')
Expand Down
4 changes: 2 additions & 2 deletions corehq/apps/registration/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from corehq.apps.domain.models import Domain
from corehq.apps.hqwebapp import crispy as hqcrispy
from corehq.apps.programs.models import Program
from corehq.apps.users.forms import BaseLocationForm, BaseTableauUserForm
from corehq.apps.users.forms import SelectUserLocationForm, BaseTableauUserForm
from corehq.apps.users.models import CouchUser


Expand Down Expand Up @@ -483,7 +483,7 @@ def clean_email(self):
return ""


class AdminInvitesUserForm(BaseLocationForm):
class AdminInvitesUserForm(SelectUserLocationForm):
email = forms.EmailField(label="Email Address",
max_length=User._meta.get_field('email').max_length)
role = forms.ChoiceField(choices=(), label="Project Role")
Expand Down
4 changes: 4 additions & 0 deletions corehq/apps/reports/filters/controllers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from memoized import memoized

from corehq import toggles
from corehq.apps.enterprise.models import EnterprisePermissions
from corehq.apps.es import GroupES, UserES, groups
from corehq.apps.locations.models import SQLLocation
Expand Down Expand Up @@ -96,6 +97,9 @@ def get_locations_query(self, query):

if self.case_sharing_only:
locations = locations.filter(location_type__shares_cases=True)
if (toggles.LOCATION_HAS_USERS.enabled(self.domain)
and not self.include_locations_with_no_users_allowed):
locations = locations.filter(location_type__has_users=True)
return locations.accessible_to_user(self.domain, self.request.couch_user)

def get_locations_size(self, query):
Expand Down
17 changes: 11 additions & 6 deletions corehq/apps/users/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
from corehq.const import LOADTEST_HARD_LIMIT, USER_CHANGE_VIA_WEB
from corehq.pillows.utils import MOBILE_USER_TYPE, WEB_USER_TYPE
from corehq.toggles import (
LOCATION_HAS_USERS,
TWO_STAGE_USER_PROVISIONING,
TWO_STAGE_USER_PROVISIONING_BY_SMS,
)
Expand Down Expand Up @@ -1141,7 +1142,7 @@ def render(self, name, value, attrs=None, renderer=None):
})


class BaseLocationForm(forms.Form):
class SelectUserLocationForm(forms.Form):
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

assigned_locations = forms.CharField(
label=gettext_noop("Locations"),
required=False,
Expand All @@ -1156,10 +1157,11 @@ class BaseLocationForm(forms.Form):
def __init__(self, domain: str, *args, **kwargs):
from corehq.apps.locations.forms import LocationSelectWidget
self.request = kwargs.pop('request')
super(BaseLocationForm, self).__init__(*args, **kwargs)
super(SelectUserLocationForm, self).__init__(*args, **kwargs)
self.domain = domain
self.fields['assigned_locations'].widget = LocationSelectWidget(
self.domain, multiselect=True, id='id_assigned_locations'
self.domain, multiselect=True, id='id_assigned_locations',
include_locations_with_no_users_allowed=False
)
self.fields['assigned_locations'].help_text = ExpandedMobileWorkerFilter.location_search_help
self.fields['primary_location'].widget = PrimaryLocationWidget(
Expand All @@ -1176,7 +1178,9 @@ def clean_assigned_locations(self):
locations = get_locations_from_ids(location_ids, self.domain)
except SQLLocation.DoesNotExist:
raise forms.ValidationError(_('One or more of the locations was not found.'))

if LOCATION_HAS_USERS.enabled(self.domain) and locations.filter(location_type__has_users=False).exists():
raise forms.ValidationError(
_('One or more of the locations you specified cannot have users assigned.'))
return [location.location_id for location in locations]

def _user_has_permission_to_access_locations(self, new_location_ids):
Expand All @@ -1185,7 +1189,7 @@ def _user_has_permission_to_access_locations(self, new_location_ids):
self.domain, self.request.couch_user))

def clean(self):
self.cleaned_data = super(BaseLocationForm, self).clean()
self.cleaned_data = super(SelectUserLocationForm, self).clean()

primary_location_id = self.cleaned_data['primary_location']
assigned_location_ids = self.cleaned_data.get('assigned_locations', [])
Expand All @@ -1209,7 +1213,7 @@ def clean(self):
return self.cleaned_data


class CommtrackUserForm(BaseLocationForm):
class CommtrackUserForm(SelectUserLocationForm):
program_id = forms.ChoiceField(
label=gettext_noop("Program"),
choices=(),
Expand Down Expand Up @@ -1631,6 +1635,7 @@ def __init__(self, *args, **kwargs):
id='id_location_id',
placeholder=_("All Locations"),
attrs={'data-bind': 'value: location_id'},
include_locations_with_no_users_allowed=False
)
self.fields['location_id'].widget.query_url = "{url}?show_all=true".format(
url=self.fields['location_id'].widget.query_url
Expand Down
14 changes: 7 additions & 7 deletions corehq/messaging/scheduling/async_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,18 +159,18 @@ def sms_case_registration_owner_id_response(self):
'id': u['id'],
'text': _("User: {}").format(u['text']),
} for u in users
] +
[
]
+ [
{
'id': g['id'],
'text': _("User Group: {}").format(g['text']),
} for g in groups
] +
[
]
+ [
{
'id': l['id'],
'text': _("Organization: {}").format(l['text']),
} for l in locations
'id': loc['id'],
'text': _("Organization: {}").format(loc['text']),
} for loc in locations
]
)

Expand Down
Loading