-
Notifications
You must be signed in to change notification settings - Fork 528
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
Fix #4879: Add NPS survey models #4914
Conversation
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.
Thanks @adhiamboperes! Took a first pass--PTAL.
…CABLE_WONT_USE_OPPIA_ANYMORE 'NA' is a bit ambiguous, and the context is "won't use Oppia anymore". If they aren't using the app, they presumably wouldn't be filling out this survey.
For proto enums, we don't need to prefix-qualify each one since they'll be part of a generated Java enum -- except the first one since the unspecified constant should always repeat the enum name per proto convention.
Refactored to explain the ID format and the uniqueness guaranteed.
To convey that it is a multiple choice answer option.
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.
Hey @BenHenning, aside from these two review comments, I have addressed all of your previous comments and I'd like you to take another look. Thanks.
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.
Thanks @adhiamboperes! Took an incremental pass. PTAL.
Once all of these are addressed, I'll take one more full pass just to make sure I didn't miss anything.
Leaving a proto list empty raises the potential ambiguity of an empty proto value, which can make it harder to reason about the model. By using oneof, we can better control the structure of the message and make it clearer what fields are relevant to each question type.
We can use this field to filter open-ended responses in analytics
STARTED_NOT_COMPLETED is already used in topic.
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.
Thanks @adhiamboperes! Took another pass--PTAL.
Also fixed some spacing issues and updated comments.
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.
Thanks @adhiamboperes! The proto is looking really good, just had one comment about two fields left--PTAL.
Since we localize text in the frontend, these are represented by enums; SurveyQuestionName and oneof answer respectively.
Reorder field numbers after removing some fields.
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.
Thanks @adhiamboperes. This LGTM!
I'll leave it to you to merge since it'll affect your downstream chain PR.
Hi @adhiamboperes, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Explanation
Fix #4879.
This is Part 1 of 7 PRs that the add the new survey feature.
This PR adds the proto model and BUILD definitions required for the feature.
Essential Checklist