Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Modify custom_text handling #1665

Merged
merged 1 commit into from
Feb 21, 2022
Merged

Modify custom_text handling #1665

merged 1 commit into from
Feb 21, 2022

Conversation

TaiWilkin
Copy link
Contributor

@TaiWilkin TaiWilkin commented Feb 18, 2022

Overview

Contributors want search queries in their embedded map to only return
matches for custom text if it is an exact match to custom text which
they submitted.

We have updated the custom_text field to use an array of strings in the
format '{contributor_id}|{custom_text}' (For example, '1|abcd'). We
format the free text query in the search in the same way, and find exact
matches.

In our previous iteration of this feature, given Contributor 1 submitted
a custom text value of "ABCD" for Facility A and Contributor 2
submitted a custom text value of "ABCD" for Facility B, searching for
"D" in Contributor 1's map could return both Facility A and Facility B.

In the current version, searching "D" would return no matches; and
searching "ABCD" in Contributor 1's map would return only Facility A.

Custom text was being reindexed each time the embed config was updated,
which is a lengthy process because we need to reindex custom text for all
facilities for the contributor each time. The embed config view has been updated to
compare the previous searchable column_names to the new searchable column_names,
and only run if the searchable column_names have been updated.

Connects #1652

Demo

Previous custom_text:

<QuerySet [{'custom_text': 'Test Value 2Test Field Value'}, {'custom_text': 'Test Value!'}]>

Updated custom_text:

<QuerySet [{'custom_text': ['3|test value 2', '3|test field value']}, {'custom_text': ['3|test value!']}]>

After preventing the custom_text from reindexing on every save, there is significant difference in speed
between updating when there has been a searchability change and when there has not:
Updating searchable fields (causes reindexing): 532.80 ms
Updating map style (does not cause reindexing): 52.45 ms

Notes

We initially planned to limit contributors to one searchable field. However, due to using an array for the custom text, it doesn't add any additional complexity to the current design to allow multiple searchable fields, so we have decided not to limit the number of searchable fields at this time.

Testing Instructions

  • On develop, sign in as c2@example.com, submit a list with custom fields oar-extra-fields.csv and fully process it. Assign deluxe embed permissions to c2@example.com.
  • Visit c2@example.com's embed settings, and assign a height and width to the map. Make custom_field and custom_field_2 searchable.
  • Run ./scripts/manage shell_plus and then FacilityIndex.objects.distinct('custom_text').values('custom_text'). You will see a list of custom text values.
  • Switch to this branch, and run ./scripts/manage migrate. The migrations should succeed.
  • Run ./scripts/manage shell_plus and then FacilityIndex.objects.distinct('custom_text').values('custom_text'). You will see a list of array-formatted custom text values. No values should have been lost.
  • Search for 'test' in c2@example.com's map. There should be no results.
  • Search for 'Test Value 2' or 'test value 2' in c2@example.com's map. There should be a result.
  • Login as c3@example.com, submit a list with custom fields
    oar-extra-fields-alt.csv, and fully process the it. Assign deluxe embed permissions to c3@example.com.
  • Visit c3@example.com's embed settings, and assign a height and width to the map. Make custom_field and custom_field_2 searchable.
  • Run ./scripts/manage shell_plus and then FacilityIndex.objects.distinct('custom_text').values('custom_text'). You will see a list of custom text values, which should now include the values from c3@example.com's list.
  • Search for 'Test Value 2' or 'test value 2' in c3@example.com's map. There should be no results.
  • Update the searchable fields and note the timing for submitting the request. Update the map style or another field unrelated to the search and note that the time is greatly reduced.

Checklist

  • fixup! commits have been squashed
  • CI passes after rebase
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

if switch_is_active('ppe'):
if embed is not None:
facilities_qs = facilities_qs \
.filter(Q(name__icontains=free_text_query) |
Q(id=free_text_query) |
Q(ppe__icontains=free_text_query) |
Q(custom_text__icontains=free_text_query))
Q(custom_text__contains=[custom_text]))
Copy link
Contributor

@emilyhu0106 emilyhu0106 Feb 18, 2022

Choose a reason for hiding this comment

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

The search works well as exact match, but I'm curious why we are using contains instead of iexact here for an exact match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, we are searching through an array for an exact match within the array. The Array.contains docs says that 'returned objects will be those where the values passed are a subset of the data' (e.g., ['3|abcd'] returns any arrays with '3|abcd' as one of the values). So it's an exact match to an item in the array.

Copy link
Contributor

@emilyhu0106 emilyhu0106 left a comment

Choose a reason for hiding this comment

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

The detailed documentation and test instruction is super helpful to understand the context here. Nice work! I can notice the loading speed between changing map style and updating searchable field.

The only missing piece is the updating the front-end to make only one field to be searchable - using radio button instead of checkboxes.

@TaiWilkin
Copy link
Contributor Author

TaiWilkin commented Feb 21, 2022

The only missing piece is the updating the front-end to make only one field to be searchable - using radio button instead of checkboxes.

After speaking to Justin, we've decided to drop the limitation of only being able to search by one field, so we don't need to change to radio buttons.

Copy link
Contributor

@emilyhu0106 emilyhu0106 left a comment

Choose a reason for hiding this comment

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

Good job on this improvement and thank you for checking with Justin about the scope of this issue!

Contributors want search queries in their embedded map to only return
matches for custom text if it is an exact match to custom text which
they submitted.

We have updated the custom_text field to use an array of strings in the
format '{contributor_id}|{custom_text}' (For example, '1|abcd'). We
format the free text query in the search in the same way, and find exact
matches.

In our previous iteration of this feature, given Contributor 1 submitted
a custom text value of "ABCD" for Facility A and Contributor 2
submitted a custom text value of "ABCD" for Facility B, searching for
"D" in Contributor 1's map could return both Facility A and Facility B.

In the current version, searching "D" would return no matches; and
searching "ABCD" in Contributor 1's map would return only Facility A.
@TaiWilkin
Copy link
Contributor Author

Thank you for reviewing!

@TaiWilkin TaiWilkin merged commit 4191a14 into develop Feb 21, 2022
@TaiWilkin TaiWilkin deleted the tw/modify-custom-text branch February 21, 2022 20:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants