Foundations for Warnings System #161
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| import Place from '../services/place'; | ||
| import { ChtApi } from './cht-api'; | ||
| import { IPropertyValue } from '../property-value'; | ||
| import { ContactType, HierarchyConstraint } from '../config'; | ||
| import { NamePropertyValue } from '../property-value/name-property-value'; | ||
|
|
||
| type RemotePlacesByType = { | ||
| [key: string]: RemotePlace[]; | ||
|
|
@@ -9,18 +12,36 @@ type RemotePlaceDatastore = { | |
| [key: string]: RemotePlacesByType; | ||
| }; | ||
|
|
||
| export type RemotePlace = { | ||
| id: string; | ||
| name: IPropertyValue; | ||
| lineage: string[]; | ||
| ambiguities?: RemotePlace[]; | ||
|
|
||
| // sadly, sometimes invalid or uncreated objects "pretend" to be remote | ||
| // should reconsider this naming | ||
| type: 'remote' | 'local' | 'invalid'; | ||
| }; | ||
|
|
||
| export default class RemotePlaceCache { | ||
| private static cache: RemotePlaceDatastore = {}; | ||
|
|
||
| public static async getPlacesWithType(chtApi: ChtApi, contactType: ContactType, hierarchyLevel: HierarchyConstraint) | ||
| : Promise<RemotePlace[]> { | ||
| const domainStore = await RemotePlaceCache.getDomainStore(chtApi, contactType, hierarchyLevel); | ||
| return domainStore; | ||
| } | ||
|
|
||
| public static add(place: Place, chtApi: ChtApi): void { | ||
|
There was a problem hiding this comment. Since we can figure out if a There was a problem hiding this comment. The reason we need There was a problem hiding this comment. Yes i understand that, the reason i bring it up is the comments here
I noticed we can convert a Place to a Remote place even when it's not created on the server. I think it'd make sense to treat remote places strictly as places we fetched from the server then we could rid of the There was a problem hiding this comment.
I've tried this, and it's a bit complex. Mostly because you need the configuration object when convertingto get data from the Place into a similar structure that comes in RemotePlace. I spent like an entire day on it and I think you're right that what we have here is still unsatisfying. I suspect I think this is probably a lot of code churn and this PR is already large + is the base of #20. Doing that work here would cause a complex merge. Would you mind if we address it in a follow-up ticket? There was a problem hiding this comment. @freddieptf Thoughts? Do you think it is important enough to do it in this PR? There was a problem hiding this comment. i think that can be addressed in another PR |
||
| const { domain } = chtApi.chtSession.authInfo; | ||
| const placeType = place.type.name; | ||
|
|
||
| const places = RemotePlaceCache.cache[domain]?.[placeType]; | ||
| // if there is no cache existing, discard the value | ||
| // it will be fetched if needed when the cache is built | ||
| if (places) { | ||
| places.push(place.asRemotePlace()); | ||
| } | ||
| } | ||
|
|
||
| public static clear(chtApi: ChtApi, contactTypeName?: string): void { | ||
|
|
@@ -34,14 +55,14 @@ export default class RemotePlaceCache { | |
| } | ||
| } | ||
|
|
||
| private static async getDomainStore(chtApi: ChtApi, contactType: ContactType, hierarchyLevel: HierarchyConstraint) | ||
| : Promise<RemotePlace[]> { | ||
| const { domain } = chtApi.chtSession.authInfo; | ||
| const placeType = hierarchyLevel.contact_type; | ||
| const { cache: domainCache } = RemotePlaceCache; | ||
| const places = domainCache[domain]?.[placeType]; | ||
| if (!places) { | ||
| const fetchPlacesWithType = RemotePlaceCache.fetchRemotePlaces(chtApi, contactType, hierarchyLevel); | ||
| domainCache[domain] = { | ||
| ...domainCache[domain], | ||
| [placeType]: await fetchPlacesWithType, | ||
|
|
@@ -50,4 +71,22 @@ export default class RemotePlaceCache { | |
|
|
||
| return domainCache[domain][placeType]; | ||
| } | ||
|
|
||
| private static async fetchRemotePlaces(chtApi: ChtApi, contactType: ContactType, hierarchyLevel: HierarchyConstraint): Promise<RemotePlace[]> { | ||
| function extractLineage(doc: any): string[] { | ||
| if (doc?.parent) { | ||
| return [doc.parent._id, ...extractLineage(doc.parent)]; | ||
| } | ||
|
|
||
| return []; | ||
| } | ||
|
|
||
| const docs = await chtApi.getPlacesWithType(hierarchyLevel.contact_type); | ||
| return docs.map((doc: any): RemotePlace => ({ | ||
| id: doc._id, | ||
| name: new NamePropertyValue(doc.name, hierarchyLevel), | ||
| lineage: extractLineage(doc), | ||
| type: 'remote', | ||
| })); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to the PR but nothing hints that
getPropertyWithNamemight actually throw an error which i think makes it hard to reason about error handling in functions that call it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true. now that we assert it on startup, maybe we don't need it in the property? we know it is also there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i think it would be clearer if
getPropertyWithNamereturned a string or undefined; then the caller can choose to assert if undefined and throw if neededThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we assert at startup, we can guarantee it is never undefined at runtime
this allows us to safely make it a
stringwhich takes the burden off the calling codei personally like this since it leads to cleaner calling code. would you rather i not assert in the get and just use typescript operators to cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we get a runtime error if the property name that was being accessed was not asserted at start up like
nameis? If this function is only used for gettingnamethen maybe it can be renamed and args changed?