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

tweak(adminPanel): RN-1274: Remove outdated survey responses and associated answers from DHIS via sync queue #5827

Merged
merged 5 commits into from
Aug 8, 2024
Merged

tweak(adminPanel): RN-1274: Remove outdated survey responses and associated answers from DHIS via sync queue #5827

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1fecea8
Remove outdated survey responses and associated answers from dhis via…
alexd-bes Aug 5, 2024
849fdd8
Add tests
alexd-bes Aug 8, 2024
6053ff3
Add answers back into queue when survey response is changed back to c…
alexd-bes Aug 8, 2024
b8c9b7d
Handle answers for outdated->current tests
alexd-bes Aug 8, 2024
5aaf274
Fix tests
alexd-bes Aug 8, 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
70 changes: 64 additions & 6 deletions packages/central-server/src/dhis/DhisChangeValidator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import { getUniqueEntries } from '@tupaia/utils';
import { ChangeValidator } from '../externalApiSync';

export class DhisChangeValidator extends ChangeValidator {
queryValidSurveyResponseIds = async (surveyResponseIds, excludeEventBased = false) => {
queryValidSurveyResponseIds = async (
surveyResponseIds,
excludeEventBased = false,
includeOutdated = false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
includeOutdated = false,
outdated = false,

I felt the wording here was a little misleading, as includeOutdated suggests that we get both outdated and not outdated responses to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
excludeEventBased = false,
includeOutdated = false,
{ excludeEventBased = false, oudated = false } = {},

How about wrapping these parameters into a single options argument? That way callers can do:

this.queryValidSurveyResponseIds(surveyResponseIds, { excludeEventBased: true, outdated: true });
// instead of
this.queryValidSurveyResponseIds(surveyResponseIds, true, true);

Which reads a lot more clearly to me.

) => {
const nonPublicDemoLandUsers = (
await this.models.database.executeSql(
`
Expand Down Expand Up @@ -41,21 +45,71 @@ export class DhisChangeValidator extends ChangeValidator {
? `AND ${this.models.surveyResponse.getExcludeEventsQueryClause()}`
: ''
}
AND survey_response.outdated = ?
AND survey_response.id IN (${batchOfSurveyResponseIds.map(() => '?').join(',')});
`,
[...nonPublicDemoLandUsers, ...batchOfSurveyResponseIds],
[...nonPublicDemoLandUsers, includeOutdated, ...batchOfSurveyResponseIds],
);
validSurveyResponseIds.push(...batchOfSurveyResponses.map(r => r.id));
}
return validSurveyResponseIds;
};

getOutdatedAnswers = async surveyResponseIds => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
getOutdatedAnswers = async surveyResponseIds => {
getDeletesForAssociatedAnswers = async surveyResponseIds => {

// get the answers associated with the survey responses
const associatedAnswers = await this.models.answer.find({
survey_response_id: surveyResponseIds,
});

// filter out answers for questions that do not sync to dhis
const dhisLinkedAnswers = await this.filterDhisLinkedAnswers(associatedAnswers);

// create delete changes for the answers
const deleteAnswers = await Promise.all(
dhisLinkedAnswers.map(async a => ({
record_type: 'answer',
record_id: a.id,
type: 'delete',
new_record: null,
old_record: await a.getData(),
})),
);

return deleteAnswers;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to test to check that this code doesn't lead to us attempting to delete the same answer twice if it the survey response changes. Admittedly we're moving away from changing survey responses in favour of outdating but it does still happen.

Looking at this code, it does seem like we'd attempt to delete it twice, and if so that might be a problem. A fix would be to not push a delete to the dhis sync queue if there's already one there?

};

getOutdatedAnswersAndSurveyResponses = async changes => {
// get the survey response ids that are being updated to 'outdated'
const surveyResponseUpdateIds = await this.getValidSurveyResponseUpdates(changes, false, true);

// get the survey response changes that are being updated to 'outdated'
const surveyResponseChanges = this.filterChangesWithMatchingIds(
changes,
surveyResponseUpdateIds,
);

const outdatedSurveyResponseDeletes = surveyResponseChanges.map(s => ({
...s,
type: 'delete',
new_record: null,
}));

// get the answers that are associated with the survey responses that are being updated to 'outdated'
const deleteAnswers = await this.getOutdatedAnswers(surveyResponseUpdateIds);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const deleteAnswers = await this.getOutdatedAnswers(surveyResponseUpdateIds);
const outdatedAnswerDeletes = await this.getOutdatedAnswers(surveyResponseUpdateIds);

I know this one is super nitpicky but it feels in line with oudatedSurveyResponseDeletes


return [...outdatedSurveyResponseDeletes, ...deleteAnswers];
};

getValidDeletes = async changes => {
return this.getPreviouslySyncedDeletes(
const outdatedSurveyResponseDeletes = await this.getOutdatedAnswersAndSurveyResponses(changes);

const previouslySyncedDeletes = await this.getPreviouslySyncedDeletes(
changes,
this.models.dhisSyncQueue,
this.models.dhisSyncLog,
);

return [...outdatedSurveyResponseDeletes, ...previouslySyncedDeletes];
};

getValidAnswerUpdates = async updateChanges => {
Expand All @@ -65,7 +119,7 @@ export class DhisChangeValidator extends ChangeValidator {

// check which survey responses are valid
const validSurveyResponseIds = new Set(
await this.queryValidSurveyResponseIds(surveyResponseIds, true),
await this.queryValidSurveyResponseIds(surveyResponseIds, true, false),
);

// filter out answers for questions that do not sync to dhis
Expand Down Expand Up @@ -101,13 +155,17 @@ export class DhisChangeValidator extends ChangeValidator {
return filteredAnswers;
};

getValidSurveyResponseUpdates = async updateChanges => {
getValidSurveyResponseUpdates = async (
updateChanges,
excludeEventBased = false,
includeOutdated = false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would suggest same change here

Suggested change
excludeEventBased = false,
includeOutdated = false,
{ excludeEventBased = false, outdated = false } = {},

) => {
const surveyResponseIds = this.getIdsFromChangesForModel(
updateChanges,
this.models.surveyResponse,
);
if (surveyResponseIds.length === 0) return [];
return this.queryValidSurveyResponseIds(surveyResponseIds);
return this.queryValidSurveyResponseIds(surveyResponseIds, excludeEventBased, includeOutdated);
};

getValidEntityUpdates = async updateChanges => {
Expand Down