New view for Data Dict JSON to support chunk load #34787
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: corehq/apps/data_dictionary/util.py
📄 File: corehq/apps/data_dictionary/views.py (Click to Expand)
Did you find this useful? React with a 👍 or 👎 |
ajeety4
force-pushed
the
ay/data-dict-chunk-load
branch
from
da777c5 to
bac90de
Compare
ajeety4
requested review from
esoergel,
orangejenny and
zandre-eng
as code owners
ajeety4
added
the
product/feature-flag
corehq/apps/data_dictionary/views.py
Outdated
| "properties_count": case_type.properties_count, | ||
| } | ||
|
|
||
| if case_type_name: |
There was a problem hiding this comment.
Nit: This function is quite long. Could we move the code related to fetching data for a single case type into its own separate function? This could even be split up into two separate views.
There was a problem hiding this comment.
I was in favour of this (moving into a different view) however went against it seeing some amount of common code. I was keen to see what others say during review.
I am going to split them.
There was a problem hiding this comment.
+1 for splitting function
There was a problem hiding this comment.
Yeah, I would also advocate splitting. I would make a few changes:
- Split
load_fhir_resource_mappings()into two functions: One for case types and one for case properties. - Pull
_get_case_data()out. Move into it some of the code that would become duplicated. Maybe something like:def get_case_type_data(domain, case_type): case_type_app_module_count = get_case_type_app_module_count(domain) used_props_by_case_type = get_used_props_by_case_type(domain) module_count = case_type_app_module_count.get(case_type.name, 0) used_props = used_props_by_case_type.get(case_type.name, []) data = { "name": case_type.name, "fhir_resource_type": None, "is_deprecated": case_type.is_deprecated, "module_count": module_count, "is_safe_to_delete": len(used_props) == 0, "properties_count": case_type.properties_count, } if toggles.FHIR_INTEGRATION.enabled(domain): resource_type_name_by_case_type = get_fhir_resource_type_map(domain) data["fhir_resource_type"] = resource_type_name_by_case_type.get(case_type) return data - Split the view at the
else:into two views: One if the case type is given, and one to list the case types.
There was a problem hiding this comment.
Addressed in 384263b
corehq/apps/data_dictionary/views.py
Outdated
| queryset = CaseType.objects.filter(domain=domain).annotate(properties_count=Count('property')) | ||
| if not request.GET.get("load_deprecated_case_types", False) == "true": | ||
| queryset = queryset.filter(is_deprecated=False) | ||
| case_types_data = [_get_case_data(case_type) for case_type in queryset] |
There was a problem hiding this comment.
Do we need the full case type JSON when just retrieving the list of case types and their property counts? I imagine properties like is_safe_to_delete and module_count we only need when loading the full set of data for a case type (since this is when a user will be able to delete the case type).
There was a problem hiding this comment.
Great point. I went with the expected response in the tech spec but I agree on this. I can check on this and get rid of things that we do not need.
There was a problem hiding this comment.
After thinking/looking more on this, I think logically all the case type related data including is_safe_to_delete and module_count should be part of the Case Type API.
Instead, we should consider not returning these case type details when we use the case properties endpoint - just return case type name, _links, properties_count and their grouped properties groups.
Does that makes sense ? @zandre-eng @kaapstorm
There was a problem hiding this comment.
I'm okay with what you've done in f91eaf5 -- I'm feeling quite pragmatic about this. The viewmodel is the only thing that will be consuming this API, and as long as it has what it needs before it needs it, I think that works.
| from corehq import privileges | ||
| from corehq.util.test_utils import flag_enabled | ||
| # TODO Remove this once we migrate to the new view | ||
| urlpatterns.insert(0, url(r"^json_v2/$", data_dictionary_json_v2, name='data_dictionary_json_v2')) |
There was a problem hiding this comment.
Instead of dynamically inserting these URL patterns for the tests, would it make sense to rather just add these to the URLs file? They already have a different URL path to the current DD JSON endpoint, and we can always switch them out when we remove the old views.
There was a problem hiding this comment.
Sure, that can be done. I only was not in favour of adding new urls , however considering we are going to remove them in short time, I am good to add these in the urls.
There was a problem hiding this comment.
Addressed in 384263b
corehq/apps/data_dictionary/views.py
Outdated
| }) | ||
| case_type_data["groups"].append(group_data) | ||
|
|
||
| return JsonResponse(case_type_data) |
There was a problem hiding this comment.
Should the geo_case_property not also be returned here? After the case property data has been loaded we use the property to identify which ones are used in the Geospatial feature.
There was a problem hiding this comment.
Good question. Although not very sure on that, I think this was probably not required as Norman has completed the changes on the JS side.
corehq/apps/data_dictionary/views.py
Outdated
| current_url = request.build_absolute_uri() | ||
| links = {"self": update_url_query_params(current_url, {"skip": skip, "limit": limit})} | ||
| if skip: | ||
| links["previous"] = update_url_query_params( | ||
| current_url, | ||
| {"skip": max(skip - limit, 0), "limit": limit} | ||
| ) | ||
| if case_type_data["properties_count"] > (skip + limit): | ||
| links["next"] = update_url_query_params(current_url, {"skip": skip + limit, "limit": limit}) | ||
| case_type_data["_links"] = links |
There was a problem hiding this comment.
This also feels like it could be split out into a separate method, maybe something like update_url_params or so.
There was a problem hiding this comment.
Addressed in 1339a9d
There was a problem hiding this comment.
Just wanted to suggest a slightly different approach to the PR
- refactor the current
data_dictionary_jsonto extract out pieces, specifically things that will stay the same - copy exactly the same view into a v2
- modify the bits that need to change
This would make it easier to identify
- what has stayed the same or what code is not new
- what has changed and is also relevant for review
As in the current state, I believe the PR is really hard to read and review.
P.S: You can ignore this if a lot has changed from the previous view.
Good suggestion in terms of making the review better. |
Hey @ajeety4 Though if others are happy to review the way it is currently, I am not going to block on this. |
Hi @mkangia , |
ajeety4
force-pushed
the
ay/data-dict-chunk-load
branch
from
f91eaf5 to
a97f285
Compare
Hello Team, as conveyed , I have made the commit changes and force pushed it. So the first commit creates a copy of existing view and have made changes on top of that which makes it better to understand. Apologies for any inconvenience. |
|
Hey @ajeety4 Just checking that this should be reviewed from the first commit again? |
corehq/apps/data_dictionary/views.py
Outdated
| properties_queryset = CaseProperty.objects.select_related('group').filter(case_type=case_type) | ||
| properties_queryset = properties_queryset.order_by('group_id', 'index', 'pk')[skip:skip + limit] | ||
| properties_queryset = properties_queryset.prefetch_related( | ||
| Prefetch('allowed_values', queryset=CasePropertyAllowedValue.objects.order_by('allowed_value')) | ||
| ) |
There was a problem hiding this comment.
Perhaps very subjective, but I find blocks like this to be more readable when they are inside parentheses instead of reassigned over several lines. e.g.
| properties_queryset = CaseProperty.objects.select_related('group').filter(case_type=case_type) | |
| properties_queryset = properties_queryset.order_by('group_id', 'index', 'pk')[skip:skip + limit] | |
| properties_queryset = properties_queryset.prefetch_related( | |
| Prefetch('allowed_values', queryset=CasePropertyAllowedValue.objects.order_by('allowed_value')) | |
| ) | |
| properties_queryset = ( | |
| CaseProperty.objects | |
| .select_related('group') | |
| .filter(case_type=case_type) | |
| .order_by('group_id', 'index', 'pk')[skip:skip + limit] | |
| .prefetch_related(Prefetch( | |
| 'allowed_values', | |
| queryset=CasePropertyAllowedValue.objects.order_by('allowed_value') | |
| )) | |
| ) |
I'm okay with leaving this as it is though.
There was a problem hiding this comment.
I too feel this is more readable. Thanks Norman. Addressed in 606be7c
ajeety4
force-pushed
the
ay/data-dict-chunk-load
branch
2 times, most recently
from
2cd5ea9 to
a97f285
Compare
ajeety4
added
product/all-users-all-environments
Product Description
Technical Summary
This is one of the sub tasks that creates a new django view with support to load case properties in a chunked fashion using skip and limit (defaults to 500).
If no case type is provided, the view returns all case types with their respective properties count.
With case type parameter, the view returns properties for that case type based on skip and limit query params.
See Technical Spec for more details.
Once the related changes are completed, the current urls for fetching case types and case properties will be migrated to this new view and old view would be removed.
JIRA Ticket and see linked tickets for related tasks as part of chunk loading.
The related tasks are basically updating the javascript model and template to support chunk loading.
Feature Flag
N/A. Available on all plans with Data Dictionary.
Safety Assurance
Safety story
Creates a new view that is not yet used, so changes are extremely safe.
Local testing done for new view
Automated test coverage
New test cases added
QA Plan
None
Rollback instructions
Labels & Review