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

Fix Filtering Bug #1272

Merged
merged 4 commits into from Mar 9, 2021
Merged

Fix Filtering Bug #1272

merged 4 commits into from Mar 9, 2021

Conversation

TaiWilkin
Copy link
Contributor

@TaiWilkin TaiWilkin commented Mar 8, 2021

Overview

In the development database, many associated models share the same id; however, in staging and production this is significantly less common. When introducing list filtering, facilities were being filtered based on matching the id to the incorrect model, which was not caught due to the ids matching in all development cases. The filtering is now based on correct models.

In addition, a new set of tests was added with manually set, non-matching ids in order to verify the fix is working locally.

Connects #983

Notes

Much of the code setting up the tests for this is a duplicate of the code in the FacilityAPITestCaseBase, but as I was applying manually set IDs to the data, I felt it was better to separate out the new test cases.

Testing Instructions

  • Visually confirm that the test cases are accurately testing the use case and the believed source of the bug.
  • Run the tests and confirm they are passing.
    • (Note: If you replace line 1237 in modes.py with facilitylistitem__source_id__in=lists).values_list(, the previous text of that line, and run the tests, you can see that they do not pass with the previous code)
  • Complete the tests previously described in #1264
    • Navigate to /api/docs/#!/contributor-lists/contributor_lists_list
    • Enter 2 as the contributor ID
      • You should get a list of contributor lists as ID/name pairs
    • Navigate to http://localhost:8081/api/docs/#!/facilities/facilities_list
    • Set the list's param to 2
      • You should get a list of facilities from the Summer 2019 Compliance List
    • Navigate to http://localhost:6543/
      • You should not see the contributor lists
    • Select Service Provider A
      • You should see the contributor lists select
    • Select the Summer 2019 Compliance List and search
      • You should get a list of facilities from that list
      • The map should load correctly
    • Select Civil Society Organization A
      • The previously selected listed should no longer be selected
      • You should see multiple lists in the dropdown
    • Select Summer 2019 Apparel List and Summer 2019 Compliance List and search
      • You should get a list of facilities from both lists
    • Refresh the page
      • Both contributors and both lists should remain selected
    • Navigate to http://localhost:6543/profile/2
      • The Facility List names should be links
    • Click the link
      • The map should open with the appropriate contributor and list selected

Checklist

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

@TaiWilkin TaiWilkin requested a review from jwalgran March 8, 2021 15:21
@TaiWilkin TaiWilkin changed the base branch from develop to tw/use-craco March 8, 2021 15:45
Copy link
Contributor

@jwalgran jwalgran left a comment

Choose a reason for hiding this comment

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

Nice use of tests to ensure the fixes worked. I retested and did not find any regressions. I opened up a tech debt issue to cover updating resetdb to give us a better chance of not running into bugs like this again.

I would like to include this as the final commit in a 2.40.0 release. Is it possible to change the target of this to develop before merging?

try:
return facility_list.source.contributor.id
except Contributor.DoesNotExist:
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider returning None rather than False here. For a function that returns a number, None is a more natural "couldn't find it" value.

@jwalgran jwalgran assigned TaiWilkin and unassigned jwalgran Mar 8, 2021
@TaiWilkin TaiWilkin changed the base branch from tw/use-craco to develop March 9, 2021 14:16
Facilities were being incorrectly filtered based on the source id
instead of the list id. This has been corrected.

In addition, the lists were being navigated to from the profile based
on the user id, not the contributor id. The lists are now navigated
to based on their associated contributor id.
@TaiWilkin TaiWilkin merged commit 3a33268 into develop Mar 9, 2021
@TaiWilkin TaiWilkin deleted the tw/filtering-bug branch March 9, 2021 17:42
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.

None yet

2 participants