-
Notifications
You must be signed in to change notification settings - Fork 3
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
[M2-4833] Feature: Additional text response #356
Conversation
The French translation is missing for now. I'll replace it once I'm familiar with how to do the translation
If an item requires additional text, the user should be blocked from proceeding if they don't provide 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.
Saw on Slack that this was looking for reviewers but title says WIP. Went ahead and added a few minor comments anyways. Overall it looks great and thank you for the amazing and helpful PR description!
Thanks for taking a look @mbanting. I forgot to remove the WIP from the title. I'll address your other comments in the morning. Regarding the TODO, I was supposed to enquire about the user events today to see if it's something I could add here or if it needs support in the API first. I'll ask the question in Slack to see what needs to happen |
The CardItem component is already configured with a conditional label so the one I'm passing is redundant. Highlighting optionality is also more in line with the existing pattern
src/entities/applet/model/types.ts
Outdated
@@ -13,7 +13,7 @@ import { | |||
TimeRangeItem, | |||
} from "~/entities/activity/lib" | |||
|
|||
export type UserEventTypes = "SET_ANSWER" | "PREV" | "NEXT" | "SKIP" | "DONE" | |||
export type UserEventTypes = "SET_ANSWER" | "SET_ADDITIONAL_TEXT" | "PREV" | "NEXT" | "SKIP" | "DONE" |
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.
No need to create a new type. Reuse "SET_ANSWER"
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.
Here a bit different logic is expected. Basically, additional text changes are not a new event. It extends current events. Let me show some examples:
We have the singleSelect with the additional text.
User choose answer first
The user event will looks like
{
type: "SET_ANSWER",
screen: activityItemScreenId,
time: Date.now(),
response: item.answer[0],
}
Then, the user fills in additional text, and user event will be updated
{
type: "SET_ANSWER",
screen: activityItemScreenId,
time: Date.now(),
response: item.answer[0],
additionalAnswer: "asd"
}
Take into account please that the additional answer has "like text" behavior. So it updates the same event while the user fills in additional text.
And also handle the case when user fills in additional text first and then choose the answer
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.
Is there any specific reason we want to reuse the SET_ANSWER
event instead of creating a new one? I briefly contemplated doing that, but I didn't have a reason to lean either way and I'd like to understand the reasoning
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.
Yes, sure. The main reason is the parity between the web application and the mobile application. The mobile app already has the same functionality and has agreements with the admin panel on what event types and DTOs are used for communication.
So I have discussed with @BamMironov how it was implemented in the mobile app and I think we should implement the feature the same way.
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.
Ah, that makes sense. Thanks for explaining. I'll take another look at the mobile app for inspiration
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.
I've made the change but with a slight caveat. The Mobile App stores its user events with the response portion in this shape:
export type PipelineItemAnswerBase = {
type: ActivityItemType;
value: {
answer?: PipelineItemResponse;
additionalAnswer?: string;
};
};
Where PipelineItemResponse
is one of many types corresponding to the item answers. The web app stores it like this:
export type UserEvents = {
type: UserEventTypes
time: number
screen: string
response?: UserEventResponse
}
export type UserEventResponse =
| string
| {
value: number[]
text?: string
}
Here UserEventResponse
is supposed to be the equivalent of PipelineItemResponse
but it is much less fleshed out. I didn't want to turn this PR into bringing the two types in line with each other, so I made a small update to support additional text on more types besides singleSelect
and multiSelect
items.
export type UserEventResponse =
| string
| {
value: string | string[] | number[]
text?: string
}
This is to avoid storing the additional text by itself for those items (like slider
), because it would look like the user cleared the answer to set the additional text. Let me know if this is a suitable compromise until a wider change can be made
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.
Great PR @sultanofcardio, love the clear commit-by-commit breakdown. Tested the flow with various activity/item configurations and it works as expected. I just had a couple very minor code-related questions.
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.
Looks good to me. Thanks
resolves: M2-4833
Objective
This PR adds an additional text response field in the respondent web app, for those items that support it:
Note
There are some items missing, and some present that are not even supported in the web app yet. I just added support based on the items that are represented in the codebase.
The PR covers these criteria:
Warning
It's not currently possible to prevent a submission with missing additional text for items that require it. The API will need to be updated to facilitate this.
Notes
There are a number of scenarios that need to be tested to verify this change
Most of this is easy enough to validate by creating an applet in the admin app, configuring it with additional text, and then verifying the behaviour in the web app on this branch. However, I think it bears mentioning that in order to confirm that the submission includes the additional text, you'll need to do an applet data export from the admin side (since the answers are encrypted on the backend, and the admin app currently doesn't display any additional text with the answers in the UI).
You may perform this data export by selecting the applet in the dashboard, identifying the respondent, and selecting the export data option
Screen.Recording.2024-01-30.at.3.19.41.PM.mov
This will download a
report.csv
file containing all the answers from that respondent. In theresponse
field, answers containing additional text will be formatted like thisWhere 2 is the answer to the item and Some text is the additional text.
Follow up tasks
N/A