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.

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

Warning System #167

Merged
merged 45 commits into from
Jan 9, 2025
Merged

Warning System #167

merged 45 commits into from
Jan 9, 2025

Conversation

kennsippell
Copy link
Member

@kennsippell kennsippell commented May 8, 2024

#20

Note this is based on #161

Warning System:

  • Adds a new unique attribute on the ContactProperties in config. These properties can be on place_properties or contact_properties.
  • When unique attributes are set on place_properties, they actually verify the true uniqueness of the data by checking it against production data on the instance. When unique attributes are set on contact_properties, they verify only the uniqueness of the data within the current batch of loaded jobs. This was done for performance reasons to avoid downloading and managing the cache of thousands of person contacts.
  • Adds a new WarningSystem library which supports abstract "Warning Classifiers"
  • Adds UniquePropertyClassifier to trigger warnings whenever data is not unique (eg. the CHP Area name is not unique within the CHU; or the CHU code is not unique within the county)
  • Adds RedundantReplaceClassifier to trigger warnings whenever multiple entries are replacing the same place
  • Integration tests capture requirements for 20 unique warning scenarios
  • Updates Kenya, Uganda, and Togo configs to specify keys which should be unique

Here is how warnings appear to the user:
image

Other Stuff:

  • Foundations for Warnings System #161 broke the credential download experience, so I refactored that so there is a single test for it
  • Bug Fix: proper sorting of breadcrumbs when moving CHPs
  • Bug Fix: clicking on places in the move_result screen should open them in new windows so the posted information isn't lost
  • Refactors the interface on RemotePlaceCache to fetch and cache all needed information in parallel and to store only the needed information required to ensure uniqueness

@kennsippell kennsippell requested a review from freddieptf May 8, 2024 10:07
: Promise<RemotePlace[]> {
const domainStore = await RemotePlaceCache.getDomainStore(chtApi, contactType, hierarchyLevel);
return domainStore;
public static async getRemotePlaces(chtApi: ChtApi, contactType: ContactType, atHierarchyLevel?: HierarchyConstraint): Promise<RemotePlace[]> {
Copy link
Collaborator

@freddieptf freddieptf Nov 7, 2024

Choose a reason for hiding this comment

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

Isn't this a really expensive call in this context? also if i'm not mistaken, doesn't the cache immediately get stale after a successful upload?

Copy link
Member Author

@kennsippell kennsippell Nov 7, 2024

Choose a reason for hiding this comment

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

well it should load from cache most of the time. the cache is only refreshed when users click refresh or after Ro's PR to add time expiry.

the cache is updated during upload (not by requesting data from the server again), but just by adding new places into the cache as they are created

Copy link
Collaborator

Choose a reason for hiding this comment

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

Been bit by this in #227, this should be the case for any high frequency contact type

Base automatically changed from format-once to main December 13, 2024 18:16
@kennsippell
Copy link
Member Author

@freddieptf This is ready for review again whenever you're back from holiday. I have merged, updated, and tested.

Copy link
Collaborator

@freddieptf freddieptf left a comment

Choose a reason for hiding this comment

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

LGTM

@kennsippell kennsippell merged commit 97aea6e into main Jan 9, 2025
1 check passed
@kennsippell kennsippell deleted the 20-warnings-formatbase branch January 9, 2025 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants