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

[feature]: Save AMR questionnaire on button click #310

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

deeonwuli
Copy link

@deeonwuli deeonwuli commented Nov 29, 2024

📌 References

📝 Implementation

  • Save questionnaire data values on button click (not on question row change)

Metadata changes

The data element formNames in dataSet AMR_AMR_DS_Questionnaire_Implementation [OYc0CihXiSn] have been modified to include the prefix Qn (where n is the question number).

📹 Screenshots/Screen capture

Screen.Recording.2024-12-02.at.15.38.20.mov

🔥 Testing

#8696nnwfh

@deeonwuli deeonwuli changed the title save questionnaire form on button click [feature]: Save AMR questionnaire on button click Dec 2, 2024
@ifoche
Copy link
Member

ifoche commented Dec 2, 2024

@deeonwuli deeonwuli marked this pull request as ready for review December 2, 2024 14:21
Copy link
Contributor

@tokland tokland left a comment

Choose a reason for hiding this comment

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

[code-only review] In-line comments:

private getDataValuesForQuestion(questionnaire: QuestionnaireSelector, question: Question): D2DataValue[] {
const { type } = question;
const { orgUnitId, year } = questionnaire;
private getDataValuesForQuestions(questionnaire: QuestionnaireSelector, questions: Question[]): D2DataValue[] {
Copy link
Contributor

@tokland tokland Dec 3, 2024

Choose a reason for hiding this comment

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

Proposal: as the "singular" method already exists, instead of refactoring it to make it "plural" (which creates an extra indentation level), let's create a new method getDataValuesForQuestions that uses the old (untouched) getDataValuesForQuestion. This will also prevent your name from showing in the "blame" for a code that is not yours :)

execute(questionnaire: QuestionnaireSelector, question: Question) {
return this.questionnaireReposotory.saveResponse(questionnaire, question);
execute(questionnaire: QuestionnaireSelector, questions: Question[]) {
return this.questionnaireReposotory.saveResponse(questionnaire, questions);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's fix the old typo: Reposotory

onClick={saveQuestionnaire}
disabled={disableSave}
>
Save Questionnaire
Copy link
Contributor

Choose a reason for hiding this comment

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

i18n.t

return [...prevState, newQuestion];
}

return prevState.map(question => (question.id === newQuestion.id ? newQuestion : question));
Copy link
Contributor

Choose a reason for hiding this comment

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

Being the last expression of the block, let's do a if/else.

const validationErrors = useMemo(() => {
return questionnaire?.sections.flatMap(section =>
_(section.questions)
.map(question => (question.validationError ? question.validationError : undefined))
Copy link
Contributor

Choose a reason for hiding this comment

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

a ? a : undefined -> a

snackbar.error(err);
}
);
}, [compositionRoot.questionnaires, questionnaire, questionsToSave, savingActions, snackbar]);
Copy link
Contributor

@tokland tokland Dec 3, 2024

Choose a reason for hiding this comment

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

This callback is running an effect, so it should be using useEffect (so it can be cancelled). This could be done with a useCallback+useState+useEffect, but we already have this common pattern abstracted: useCallbackEffect. See some usages in the app.

@deeonwuli deeonwuli requested a review from tokland December 3, 2024 14:23
…into feat/update-validation-rules-for-amr-questionnaire
Copy link
Contributor

@tokland tokland left a comment

Choose a reason for hiding this comment

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

Code-wise, all good!

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.

3 participants