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.

Sign up for GitHub

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

Jump to bottom

DO NOT MERGE - Contact Details' Load Tasks #9014

Closed
wants to merge 2 commits into from
Closed

DO NOT MERGE - Contact Details' Load Tasks #9014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
062efb5
Merge branch 'master' of https://github.com/medic/cht-core
latin-panda Apr 16, 2024
4db6bbd
Playground
latin-panda Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shared-libs/rules-engine/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ module.exports = db => {
* @param {string[]} contactIds An array of contact ids. If undefined, returns tasks for all contacts
* @returns {Promise<Object[]>} All the fresh task docs owned by contactIds
*/
fetchTasksFor: contactIds => wireupToProvider.fetchTasksFor(provider, contactIds),
fetchTasksFor: (contactIds, docs) => wireupToProvider.fetchTasksFor(provider, contactIds, docs),

/**
* Returns a breakdown of tasks by state and title for the provided list of contacts
Expand Down
10 changes: 8 additions & 2 deletions shared-libs/rules-engine/src/pouchdb-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,25 +122,31 @@ const medicPouchProvider = db => {
return rowsOf(dbQuery( 'medic-client/tasks_by_contact', options));
},

taskDataFor: (contactIds, userSettingsDoc) => {
taskDataFor: (contactIds, contactDocs, userSettingsDoc) => {
if (!contactIds || contactIds.length === 0) {
return Promise.resolve({});
}

return docsOf(db.allDocs({ keys: contactIds, include_docs: true }))
const contactsPromise = contactDocs?.length ?
Promise.resolve(contactDocs) : docsOf(db.allDocs({ keys: contactIds, include_docs: true }));

return contactsPromise
.then(contactDocs => {
console.warn('pouchProvider ==> taskDataFor ==> contactDocs:', contactDocs);
const subjectIds = contactDocs.reduce((agg, contactDoc) => {
registrationUtils.getSubjectIds(contactDoc).forEach(subjectId => agg.add(subjectId));
return agg;
}, new Set(contactIds));

const keys = Array.from(subjectIds);
console.warn('pouchProvider ==> taskDataFor ==> subjectIds:', keys);
return Promise
.all([
docsOf(dbQuery('medic-client/reports_by_subject', { keys, include_docs: true })),
self.tasksByRelation(contactIds, 'requester'),
])
.then(([reportDocs, taskDocs]) => {
console.warn('pouchProvider ==> taskDataFor ==> reportDocs:', reportDocs);
// tighten the connection between reports and contacts
// a report will only be allowed to generate tasks for a single contact!
reportDocs = reportDocs.filter(report => {
Expand Down
26 changes: 18 additions & 8 deletions shared-libs/rules-engine/src/provider-wireup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,20 @@ module.exports = {
* @param {string[]} contactIds An array of contact ids. If undefined, returns tasks for all contacts
* @returns {Promise<Object[]>} All the fresh task docs owned by contacts
*/
fetchTasksFor: (provider, contactIds) => {
fetchTasksFor: (provider, contactIds, docs) => {
if (!rulesEmitter.isEnabled() || !wireupOptions.enableTasks) {
return disabledResponse();
}

return enqueue(() => {
const calculationTimestamp = Date.now();
return refreshRulesEmissionForContacts(provider, calculationTimestamp, contactIds)
.then(() => contactIds ? provider.tasksByRelation(contactIds, 'owner') : provider.allTasks('owner'))
return refreshRulesEmissionForContacts(provider, calculationTimestamp, contactIds, docs)
.then(() => {
if (contactIds) {
return provider.tasksByRelation(contactIds, 'owner');
}
return provider.allTasks('owner');
})
.then(tasksToDisplay => {
const docsToCommit = updateTemporalStates(tasksToDisplay, calculationTimestamp);
provider.commitTaskDocs(docsToCommit);
Expand Down Expand Up @@ -217,7 +222,7 @@ const disabledResponse = () => {
return p;
};

const refreshRulesEmissionForContacts = (provider, calculationTimestamp, contactIds) => {
const refreshRulesEmissionForContacts = (provider, calculationTimestamp, contactIds, docs) => {
const refreshAndSave = (freshData, updatedContactIds) => (
refreshRulesEmissions(freshData, calculationTimestamp, wireupOptions)
.then(refreshed => Promise.all([
Expand Down Expand Up @@ -247,18 +252,23 @@ const refreshRulesEmissionForContacts = (provider, calculationTimestamp, contact
))
);

const refreshForKnownContacts = (calculationTimestamp, contactIds) => {
const refreshForKnownContacts = (calculationTimestamp, contactIds, docs) => {
// eslint-disable-next-line no-console
console.warn('rules engine - refreshForKnownContacts - ids', contactIds);
// eslint-disable-next-line no-console
console.warn('rules engine - refreshForKnownContacts - docs', docs);
const dirtyContactIds = contactIds.filter(contactId => rulesStateStore.isDirty(contactId));
return provider.taskDataFor(dirtyContactIds, rulesStateStore.currentUserSettings())
const dirtyContactDocs = docs.filter(doc => rulesStateStore.isDirty(doc._id));
return provider.taskDataFor(dirtyContactIds, dirtyContactDocs, rulesStateStore.currentUserSettings())
.then(freshData => refreshAndSave(freshData, dirtyContactIds))
.then(() => {
rulesStateStore.markFresh(calculationTimestamp, dirtyContactIds);
});
};

return handleIntervalTurnover(provider, { monthStartDate: rulesStateStore.getMonthStartDate() }).then(() => {
if (contactIds) {
return refreshForKnownContacts(calculationTimestamp, contactIds);
if (contactIds || docs?.length) {
return refreshForKnownContacts(calculationTimestamp, contactIds, docs);
}

// If the contact state store does not contain all contacts, build up that list (contact doc ids + headless ids in
Expand Down
8 changes: 4 additions & 4 deletions webapp/src/ts/services/rules-engine.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,11 +341,11 @@ export class RulesEngineService implements OnDestroy {
.then(taskDocs => this.translateTaskDocs(taskDocs));
}

fetchTaskDocsFor(contactIds) {
return this.ngZone.runOutsideAngular(() => this._fetchTaskDocsFor(contactIds));
fetchTaskDocsFor(contactIds, docs?) {
return this.ngZone.runOutsideAngular(() => this._fetchTaskDocsFor(contactIds, docs));
}

private _fetchTaskDocsFor(contactIds) {
private _fetchTaskDocsFor(contactIds, docs?) {
const trackName = [ 'rules-engine', 'tasks', 'some-contacts' ];
let trackPerformanceQueueing;
let trackPerformanceRunning;
Expand All @@ -358,7 +358,7 @@ export class RulesEngineService implements OnDestroy {
);

return this.rulesEngineCore
.fetchTasksFor(contactIds)
.fetchTasksFor(contactIds, docs)
.on('queued', () => trackPerformanceQueueing = this.performanceService.track())
.on('running', () => {
trackPerformanceRunning = this.performanceService.track();
Expand Down
18 changes: 17 additions & 1 deletion webapp/src/ts/services/tasks-for-contact.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,19 @@ export class TasksForContactService {
return contactIds;
}

getDocsForTasks(model) {
const contactDocs: any[] = [];
if (!model?.type?.person && model?.children) {
model.children.forEach(child => {
if (child?.type?.person && child?.contacts?.length) {
contactDocs.push(...child.contacts.map(contact => contact.doc));
}
});
}
contactDocs.push(model.doc);
return contactDocs;
}

private areTasksEnabled(type) {
if (!type) {
return Promise.resolve(false);
Expand Down Expand Up @@ -82,8 +95,11 @@ export class TasksForContactService {
}

const contactIds = this.getIdsForTasks(model);
const docs = this.getDocsForTasks(model);
console.warn('TasksForContactService.get - model', model);
console.warn('TasksForContactService.get - contact docs', docs);
return this.rulesEngineService
.fetchTaskDocsFor(contactIds)
.fetchTaskDocsFor(contactIds, docs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @dianabarsan! I opened this PR to show you an idea; this code is not even close to being ready for prod—still a playground.

When loading Contact Details, the loadTasks sends the selected contact to this function. It contains children, lineage, and its doc.
I noticed those are the same docs that pouch provider is fetching from the db when there are dirty contacts.

What do you think about splitting the process in this.rulesEngineService.fetchTaskDocsFor(contactIds)
and this.rulesEngineService.fetchTaskDocsForContactDocs(contactDocs)? Then contact.effects sends the docs using the later function

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking of assigning myself as a reviewer, otherwise there's a risk I forget about this. Do you mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure that's okay! Thanks for your time

Copy link
Member

Choose a reason for hiding this comment

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

I think there might be an issue with a race condition here.

The rules-engine has a queue for executable jobs, so that we don't end up starting multiple task calculation processes in parallel. This will mean that, if the queue is already populated, we could be executing the task refresh for this list of contacts a little bit later than this call, and at that time, there's a chance that some of the contacts in the list were edited and marked dirty, and we could be calculating tasks for old versions of these docs, and marking the docs as clean, preventing correct tasks from being calculated.

How resource intensive is the allDocs in rules engine? Is it worth to risk this hard to track behavior for this gain in performance?

.then(tasks => this.decorateAndSortTasks(tasks));
});
}
Expand Down
Loading