Repository navigation
Feature/edu role survey model - #10198
Draft
gmorador-tribu wants to merge 6 commits into
Draft
gmorador-tribu wants to merge 6 commits into
gmorador-tribu wants to merge 6 commits into
Conversation
Elimpizza
added this pull request to stack #10201
September 22, 2026 14:50
Elimpizza
force-pushed
the
feature/edu-role-survey-model
branch
from
September 23, 2026 15:12
592b984 to
bc133e6
Compare
Elimpizza
reviewed
Sep 23, 2026
| "instructor_survey": ( | ||
| "Show the EDU role survey in the client sidebar. Also the kill switch: " | ||
| "h stops sending the survey as soon as this is off. First-party only, " | ||
| "and enabling it for `everyone` will not widen it past the first-party " |
Contributor
There was a problem hiding this comment.
"and enabling it for
everyonewill not widen it past the first-party "
Is this actually true?
| The column allows an answer to be replaced rather than being write-once, | ||
| because product plans to re-run the survey for the people who dismissed it | ||
| so they get to answer a second time. Nothing replaces one yet: the API | ||
| drops or rejects a second answer (h.views.api.profile.update_preferences), |
Contributor
There was a problem hiding this comment.
so they get to answer a second time. Nothing replaces one yet: the API drops or rejects a second answer
There's no such check. UserService.update_preferences assigns edu_role_survey_response and edu_role_survey_responded_at unconditionally, so a retry after a lost response, or a dismissal from a second sidebar, silently replaces an earlier answer.
I'd either add the write-once guard or drop the sentence.
Declare the user_survey_response table added by migration 2fdc26d19dbc as the UserSurveyResponse model, with User.survey_responses for the rows of a user. Two enums name the valid values: Survey (edu_role) and EduRoleSurveyResponse (instructor, not_instructor, dismissed). The CheckConstraint is built from the enums so the model and the migration can't drift apart. Nothing reads or writes the table yet: no eligibility logic, no feature flag, no API change. DO NOT MERGE until migration 2fdc26d19dbc has been applied everywhere: Tasks -> "db upgrade to head" on staging and on prod, in every region, and Tasks -> "db current revision" reporting 2fdc26d19dbc. h doesn't run migrations on deploy, so merging this first would leave the code declaring a table that doesn't exist, and every load of User.survey_responses would fail with UndefinedTable.
Register the instructor_survey feature flag and the client.survey.instructor_role.shown analytics event. Both are inert: nothing checks the flag and nothing emits the event yet. The flag's row is created switched off by Feature.all(), and it gates the survey from the server side so that turning it off stops h sending the survey from the next profile fetch on, with no client release to propagate through the CDN and the browser extension. The flag description spells out that it is first-party only, and that ticking `everyone` does not widen it past the first-party authority -- FeatureService checks `everyone` before it looks at the user's authority.
Accept instructor_survey_response in update_preferences, validate it against the EduRoleSurveyResponse enum, and record it in user_survey_response along with the timestamp. The answer is upserted on (user_id, survey), and the upsert only replaces a dismissal: product plans to re-run the survey for the people who dismissed it, so a dismissal has to be replaceable, but an "instructor" or "not_instructor" answer is final. The rule is part of the same INSERT ... ON CONFLICT DO UPDATE ... WHERE statement, so two answers sent at once can't both see a dismissal and replace each other. The enum's docstring is updated to say so -- it previously claimed we only ask once, which stopped being true with that decision. update_preferences stops being a staticmethod, because it now needs the session. Every caller already calls it on the service instance. The upsert goes around the session, which may already hold the user's answers, so it RETURNs the row with populate_existing to refresh an answer already loaded, and expires User.survey_responses to pick up a new one. Validation failures raise TypeError, which the profile view already turns into an HTTPBadRequest, so a bad value is a 400 rather than an IntegrityError from the table's CHECK constraint.
The survey is only ever offered to first-party users, so only they can answer
it. Without this a third-party account -- an LMS user who could never have
been shown the panel -- can PATCH an answer straight into the column the
HubSpot sync will read, polluting the dataset the survey exists to collect.
Follows the authority enforcement in h/views/api/users.py::create: an explicit
check in the view raising ValidationError, which is h's HTTPBadRequest
subclass, rather than a bare HTTPBadRequest.
The check sits inside the existing try block on purpose. `preferences` is
unvalidated JSON straight off the request body, so `"..." in preferences`
raises TypeError for a non-mapping body such as {"preferences": null}; above
the try that would escape as a 500, where today the `**preferences` below
makes it a 400. There's a regression test for it.
This is only part of the eligibility the read side applies -- the feature flag
and the EDU domain check need h/session.py and h/util/edu_domains.py, and land
with them. The comment in the view says so.
…lder Two documentation fixes to text that is read at the moment a decision is made. The EduRoleSurveyResponse docstring now says what UserSurveyResponse.responded_at holds: when the answer sitting beside it was recorded, which is the first answer unless a later one replaced a dismissal. Whoever builds the HubSpot sync reads this docstring. The warning that EDU_DOMAINS is a placeholder that matches every *.edu address lived only in a comment in h/util/edu_domains.py. The person about to tick `first_party` on the flag is reading the description in the admin feature UI, which said nothing about it, so the warning was not where the mistake gets made. It is in the flag description now.
The instructor_survey description said the flag is the only thing keeping the placeholder domain list from real users. _feature_overrides turns any flag on for any user, logged out included, from ?__feature__[<name>] in the query string -- on the PATCH as much as the GET. The description names that exception now; the comment in h/session.py already did.
gmorador-tribu
force-pushed
the
feature/edu-role-survey-model
branch
from
October 9, 2026 03:29
bc133e6 to
1b40d61
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #10196 — do not merge until that migration has been applied in every environment and region.
Part 2 of the EDU role survey (product-backlog#1751). Code only, no migrations.
The answer lives in its own
user_survey_responsetable, created by #10196, rather than in two columns onuser.UserService.update_preferencesupserts it on(user_id, survey), and the upsert only replaces a dismissal: aninstructorornot_instructoranswer is never overwritten, not even by two requests sent at the same time.Why this is a draft
hdoesn't run migrations on deploy, and merging deploys automatically. If this lands before2fdc26d19dbchas been applied, anything that loadsUser.survey_responsesfails withUndefinedTable: deleting a user, and any PATCH that sendsinstructor_survey_response. It comes out of draft once Tasks → "db current revision" reports2fdc26d19dbcon staging and on prod in both regions (us-west-1andca-central-1).