From 434fe801ba52c9c33eb0d696d8a7b6c3ee93331b Mon Sep 17 00:00:00 2001 From: James Griffin Date: Thu, 30 Jul 2026 13:51:52 -0300 Subject: [PATCH] Ask some registration questions of students only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every account-signup question was asked of everybody who registered, on the same terms: "school and grade" had to be put to an adult signing themselves up, and a question a studio needed answered for each student could only be made required by demanding it of everyone. A question now carries an audience — everyone, or only the students someone registers on behalf of — and its own required flag for each side, so optional for you and required for every student you enrol is expressible. Both settings are account-scope only: an offering asks its questions once, about the student being booked, so there is no second audience to differ from, and an offering question mirrors its single "required" into both columns. Every caller reads askedOfSelf()/isRequiredForSelf()/isRequiredForChild() rather than the raw flags, so a students-only question can neither block the account holder nor have an answer filed against them by a crafted post. The family screen, which only ever adds a student, is held to the students' rule. is_required_child arrives from dbDelta defaulting to 0, which would quietly stop every existing required question being required of the students a guardian registers — the case it most likely existed for. A one-time backfill copies is_required across, guarded by its own option so a question later made optional for students stays that way. Closes #163 Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 5 +- docs/features/registration-questions.md | 45 ++++- src/Auth/RegistrationPage.php | 23 ++- src/Guardian/FamilyPage.php | 6 +- src/Plugin.php | 9 + src/Registration/Question.php | 104 ++++++++++-- src/Registration/QuestionController.php | 24 ++- src/Registration/QuestionEndpoint.php | 46 ++++-- src/Registration/QuestionField.php | 12 +- src/Registration/QuestionRepository.php | 48 ++++-- src/Schema.php | 2 + templates/admin/questions.php | 65 +++++++- templates/frontend/family-page.php | 7 +- templates/frontend/register-page.php | 19 ++- tests/Unit/Auth/RegistrationPageTest.php | 155 +++++++++++++++++- tests/Unit/Guardian/FamilyPageTest.php | 79 ++++++++- tests/Unit/Registration/QuestionFieldTest.php | 85 ++++++++++ .../Registration/QuestionRepositoryTest.php | 53 ++++++ tests/Unit/Registration/QuestionTest.php | 103 +++++++++++- unsupervised-schedular.php | 4 +- 20 files changed, 809 insertions(+), 85 deletions(-) create mode 100644 tests/Unit/Registration/QuestionFieldTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index a703d46..88d5115 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,10 @@ When a `v*` tag is pushed, `.gitea/workflows/release.yml` publishes the matching the plugin to the next patch version and adds a fresh section here for it. Record each change under the current top section as you work. -## [1.4.2] +## [1.5.0] + +### Added +- **A registration question can now be asked of students only, and can be required of a student without being required of the account holder.** Every account-signup question was asked of everybody who registered, on the same terms — so "School and grade" had to be put to the adult signing themselves up, and a question a studio needed answered for a child could only be made required by demanding it of everyone. Each question now says who it is asked of — everyone, or only the students you register on behalf of — and carries its own **Required** setting for each: optional for you, required for every student you enrol, is now a thing a studio can ask for. Existing questions are untouched: they stay asked of everyone, and one that was required stays required of everyone. ## [1.4.1] diff --git a/docs/features/registration-questions.md b/docs/features/registration-questions.md index 363c7c0..9423fad 100644 --- a/docs/features/registration-questions.md +++ b/docs/features/registration-questions.md @@ -23,11 +23,32 @@ and the same authoring page (**Offerings → Questions**). | `label` | VARCHAR(255) | The question text shown to the registrant | | `field_type` | VARCHAR(20) | `text` / `textarea` / `select` / `checkbox` | | `options` | TEXT | JSON array of choices (for `select`); NULL otherwise | -| `is_required` | TINYINT(1) | 1 = registrant must answer to continue | +| `audience` | VARCHAR(20) | `all` (default) or `child` — who the question is asked of (account scope) | +| `is_required` | TINYINT(1) | 1 = the **account holder** must answer to continue | +| `is_required_child` | TINYINT(1) | 1 = each **student being registered** must answer to continue | | `sort_order` | INT | Display order within the scope | | `is_active` | TINYINT(1) | 0 = retired, 1 = shown on the form | | `created_at` | DATETIME | Insertion time | +## Audience and Required-ness (account scope) +An account-scope question is asked in two places, and the two are configured separately: + +- **The account holder's own "About you" panel** — shown when they are registering + themselves (`self` or `both`). Governed by `audience` (a `child` question is not asked + here at all) and by `is_required`. +- **Each student block** — one per person they are registering on behalf of, on the signup + form and on the guardian's family screen. Every question is asked here regardless of + `audience`; `is_required_child` decides whether it blocks submission. + +That split is what lets a studio ask "School and grade" of children only, or make +"Previous experience" optional for an adult signing themselves up but required for every +child they enrol. `audience = 'child'` leaves `is_required` moot — the question never +reaches the account holder's panel. + +`audience` and `is_required_child` are ignored for offering-scope questions: booking and +enrolment ask their intake questions once, about the student being booked, with no separate +account-holder form to differ from. + ## Data Model — `{prefix}us_question_answers` | Column | Type | Notes | @@ -51,19 +72,24 @@ lesson, a group enrolment, or an account signup (`account` + the user ID). ## Account-scope Flow (signup) 1. The `[us_student_register]` page (`Auth\RegistrationPage`) loads active account-scope questions via `QuestionRepository::findByScope('account')`. -2. The form is a single page. The questions sit in an **About you** panel, alongside the account holder's birth year, between the "Who are you registering?" choice and the students being added. `assets/js/register.js` disables and hides that whole panel when the choice is "on behalf of students" — the questions describe a student and a pure guardian is not one — and puts the same questions in every child block instead. Progressive enhancement: without JS every panel shows and the single submit still works. This applies to **every** signup path (invite, group link, self-approval). -3. On submit, required answers are validated **before** the user is created (a missing answer returns an error and creates no account); after creation each answered question is written to `us_question_answers` with `registration_type = 'account'`, `registration_id = student_id = `. +2. The form is a single page. The questions sit in an **About you** panel, alongside the account holder's birth year, between the "Who are you registering?" choice and the students being added — minus any `audience = 'child'` question, which is never asked of the account holder. `assets/js/register.js` disables and hides that whole panel when the choice is "on behalf of students" — the questions describe a student and a pure guardian is not one — and puts the full question set in every child block instead. Progressive enhancement: without JS every panel shows and the single submit still works. This applies to **every** signup path (invite, group link, self-approval). +3. On submit, required answers are validated **before** the user is created (a missing answer returns an error and creates no account) — `is_required` against the account holder's panel, `is_required_child` against each student block; after creation each answered question is written to `us_question_answers` with `registration_type = 'account'`, `registration_id = student_id = `. An answer posted for a `child`-audience question against the account holder is discarded, not stored. 4. A studio admin reviews the answers on the student's admin screen under **Registration Information** (`Auth\StudentHistory::registrationInfo()` lists every account question paired with the student's answer, "—" when unanswered). These rows are excluded from the offering-scope "Intake answers" table. ## Admin Interface Both scopes are edited from **Offerings → Questions** (`Registration\QuestionController`): - Pick an offering to edit its questions, or **"Account signup (all registrations)"** for the account-scope questions. +- The account-scope form adds **Asked of** (everyone / students only) and a second **Required** checkbox for students; both are hidden for offering scope, where they have no meaning. - Studio admin (`manage_questions` + `manage_instructors`) edits any offering's questions and the account-scope questions. - Instructor (`manage_questions`) edits questions only on their own offerings; the account-scope option is hidden. ## REST API Only offering-scope questions are exposed over REST. Account-scope questions are managed -through the server-rendered admin page and read directly by `RegistrationPage`. +through the server-rendered admin page and read directly by `RegistrationPage` — a request +naming one is turned away as not found, since the owner check has no offering to check +against, so REST can neither read nor overwrite an `audience`. An offering question written +over REST mirrors its single `is_required` into `is_required_child`, as the admin form and +the upgrade backfill both do. | Method | Endpoint | Permission | |----------|---------------------------------------------------|----------------------| @@ -74,12 +100,13 @@ through the server-rendered admin page and read directly by `RegistrationPage`. ## Implementation - Repositories: `Unsupervised\Schedular\Registration\QuestionRepository` (`findByOffering`, `findByScope`), `Unsupervised\Schedular\Registration\AnswerRepository` -- Models: `Unsupervised\Schedular\Registration\Question` (`scope`, nullable `offeringId`), `Unsupervised\Schedular\Registration\Answer` (`REG_ACCOUNT`) +- Models: `Unsupervised\Schedular\Registration\Question` (`scope`, nullable `offeringId`, `audience`, `isRequiredChild`, and the `askedOfSelf()` / `isRequiredForSelf()` / `isRequiredForChild()` readers every caller uses instead of touching `isRequired` directly), `Unsupervised\Schedular\Registration\Answer` (`REG_ACCOUNT`) - Admin controller: `Unsupervised\Schedular\Registration\QuestionController` - REST endpoint: `Unsupervised\Schedular\Registration\QuestionEndpoint` (offering scope only) - Signup form: `Unsupervised\Schedular\Auth\RegistrationPage`, `templates/frontend/register-page.php`, `assets/js/register.js` - Admin review: `Unsupervised\Schedular\Auth\StudentHistory::registrationInfo()`, `templates/admin/student-detail.php` -- Schema: `us_questions.scope` + nullable `us_questions.offering_id` (requires a plugin version bump so `dbDelta` runs) +- Schema: `us_questions.scope` + nullable `us_questions.offering_id`, `us_questions.audience`, `us_questions.is_required_child` (each requires a plugin version bump so `dbDelta` runs) +- Required-for-students backfill: `is_required_child` arrives with `DEFAULT 0`, which would quietly make every existing required question optional for students. `QuestionRepository::backfillChildRequired()` copies `is_required` into it once; `Plugin::boot()` runs it guarded by the `us_questions_child_required_backfilled` option, after the version gate has let `dbDelta` add the column - Nullability repair: `dbDelta` does **not** reliably relax a column from `NOT NULL` to `NULL`, so sites created before account-scope questions kept `offering_id NOT NULL` and rejected account inserts. `QuestionRepository::ensureOfferingNullable()` re-applies the nullable definition (idempotent `ALTER … MODIFY`); `Plugin::boot()` runs it once, guarded by the `us_questions_offering_nullable` option rather than the version gate (affected sites may already be on the current version) ## Tests @@ -87,8 +114,10 @@ through the server-rendered admin page and read directly by `RegistrationPage`. - `tests/Unit/Registration/AnswerRepositoryTest.php` - `tests/Unit/Registration/QuestionTest.php` - `tests/Unit/Registration/AnswerTest.php` +- `tests/Unit/Registration/QuestionFieldTest.php` - `tests/Unit/Auth/RegistrationPageTest.php` - `tests/Unit/Auth/StudentHistoryTest.php` +- `tests/Unit/Guardian/FamilyPageTest.php` ## Per-Child Answers For a parent/guardian signup, **account-scope** questions are asked **once per @@ -96,5 +125,5 @@ child** rather than once per guardian — in practice they describe the student (instrument, level, school), not the account holder. Each answer's `student_id` and `registration_id` are the child's user ID, so a studio admin reading a child's screen sees the information that describes them. The guardian's family -screen asks the same questions when a child is added later. See -`parent-guardian-accounts.md`. +screen asks the same questions when a child is added later, under the same +`is_required_child` rule as the signup form. See `parent-guardian-accounts.md`. diff --git a/src/Auth/RegistrationPage.php b/src/Auth/RegistrationPage.php index 784fc6b..fb2ab8a 100644 --- a/src/Auth/RegistrationPage.php +++ b/src/Auth/RegistrationPage.php @@ -319,6 +319,13 @@ class RegistrationPage { // student themselves. "Both" is both. $registeringFor = $this->submittedRegisteringFor(); + // A "students only" question is never put to the account holder, so it is + // dropped before their answers are validated or stored — a crafted post + // cannot file one against them. + $selfQuestions = array_values( + array_filter( $accountQuestions, static fn( Question $question ): bool => $question->askedOfSelf() ) + ); + // "Students" and "both" collect student blocks; only "self" does not. $isGuardian = self::FOR_SELF !== $registeringFor; @@ -355,7 +362,7 @@ class RegistrationPage { // Checked as two passes rather than one so the message can say *whose* // answers are missing — under "both" a single message could not. foreach ( array_column( $children, 'answers' ) as $set ) { - if ( $this->hasUnansweredRequired( $accountQuestions, $set ) ) { + if ( $this->hasUnansweredRequired( $accountQuestions, $set, forChild: true ) ) { return esc_html__( 'Please answer all required registration questions for each student.', 'unsupervised-schedular' ); } } @@ -369,7 +376,7 @@ class RegistrationPage { return esc_html( GuardianService::ownBirthYearError() ); } - if ( $asksSelf && $this->hasUnansweredRequired( $accountQuestions, $answers ) ) { + if ( $asksSelf && $this->hasUnansweredRequired( $selfQuestions, $answers ) ) { return esc_html__( 'Please answer all required registration questions.', 'unsupervised-schedular' ); } @@ -420,7 +427,7 @@ class RegistrationPage { // After the children, so a rollback that deletes this account cannot // leave its answers behind pointing at a user that no longer exists. if ( $asksSelf ) { - $this->recordAnswers( $accountQuestions, $answers, (int) $userId ); + $this->recordAnswers( $selfQuestions, $answers, (int) $userId ); } if ( $inviteValid && ! $invite->isGroup() ) { @@ -564,12 +571,18 @@ class RegistrationPage { /** * Whether any required question in `$questions` is left blank in `$answers`. * + * `$forChild` picks which required-ness applies: a question can be optional + * for the account holder answering about themselves and still required of + * every student they register. + * * @param list $questions * @param array $answers */ - private function hasUnansweredRequired( array $questions, array $answers ): bool { + private function hasUnansweredRequired( array $questions, array $answers, bool $forChild = false ): bool { foreach ( $questions as $question ) { - if ( $question->isRequired && '' === trim( (string) ( $answers[ (int) $question->id ] ?? '' ) ) ) { + $required = $forChild ? $question->isRequiredForChild() : $question->isRequiredForSelf(); + + if ( $required && '' === trim( (string) ( $answers[ (int) $question->id ] ?? '' ) ) ) { return true; } } diff --git a/src/Guardian/FamilyPage.php b/src/Guardian/FamilyPage.php index a5da4f0..a1c1628 100644 --- a/src/Guardian/FamilyPage.php +++ b/src/Guardian/FamilyPage.php @@ -162,12 +162,16 @@ class FamilyPage { * The first required question left unanswered, as the error to show — or null * when every required question has a value. * + * This screen only ever adds a student the guardian registers, so the + * students' required-ness is the one that applies — the same rule the child + * blocks on the signup form are held to. + * * @param list $questions * @param array $answers question_id => submitted value */ private function firstMissingAnswer( array $questions, array $answers ): ?\WP_Error { foreach ( $questions as $question ) { - if ( $question->isRequired && '' === trim( (string) ( $answers[ (int) $question->id ] ?? '' ) ) ) { + if ( $question->isRequiredForChild() && '' === trim( (string) ( $answers[ (int) $question->id ] ?? '' ) ) ) { return new \WP_Error( 'missing_answer', __( 'Please answer all required questions for this student.', 'unsupervised-schedular' ) ); } } diff --git a/src/Plugin.php b/src/Plugin.php index 9b82f3e..5cd5e8f 100644 --- a/src/Plugin.php +++ b/src/Plugin.php @@ -72,6 +72,15 @@ class Plugin { update_option( 'us_questions_offering_nullable', '1' ); } + // One-time backfill of us_questions.is_required_child, which dbDelta adds + // defaulting to 0 — leaving every question that *was* required no longer + // required of the students a guardian registers. Runs after the version + // gate above, so the column it writes to exists. Guarded so a question + // later made optional for students stays that way. + if ( '1' !== get_option( 'us_questions_child_required_backfilled', '' ) && $questions->backfillChildRequired() ) { + update_option( 'us_questions_child_required_backfilled', '1' ); + } + $answers = new AnswerRepository( $wpdb ); $policies = new PolicyRepository( $wpdb ); $policyVersions = new PolicyVersionRepository( $wpdb ); diff --git a/src/Registration/Question.php b/src/Registration/Question.php index 97a0405..5c79999 100644 --- a/src/Registration/Question.php +++ b/src/Registration/Question.php @@ -21,6 +21,16 @@ class Question { /** Question is studio-wide, asked once at account signup (no offering). */ public const SCOPE_ACCOUNT = 'account'; + /** Asked of everyone: the account holder as a student, and each student they register. */ + public const AUDIENCE_ALL = 'all'; + + /** + * Asked only of the students someone registers on behalf of — never of the + * account holder's own "About you" panel. For the questions that only make + * sense about a child ("school and grade", "who may collect them"). + */ + public const AUDIENCE_CHILD = 'child'; + /** * All valid field types. * @@ -43,9 +53,29 @@ class Question { self::SCOPE_ACCOUNT, ]; + /** + * All valid audiences. + * + * @var list + */ + public const VALID_AUDIENCES = [ + self::AUDIENCE_ALL, + self::AUDIENCE_CHILD, + ]; + /** * Build an intake question value object. * + * `$isRequired` and `$isRequiredChild` are deliberately separate: a studio may + * want an answer from every student it enrols without demanding the same of an + * adult signing themselves up. Read them through {@see isRequiredForSelf()} and + * {@see isRequiredForChild()} rather than directly, so the audience is applied + * with them. + * + * Both `$audience` and `$isRequiredChild` are meaningless for offering scope, + * where a booking asks its questions once about the student being booked and + * there is no separate account-holder form to differ from. + * * @param int|null $offeringId The owning offering, or null for account-scoped questions. * @param list|null $options Choices for a `select` field. */ @@ -58,9 +88,36 @@ class Question { public readonly int $sortOrder = 0, public readonly bool $isActive = true, public readonly string $scope = self::SCOPE_OFFERING, + public readonly string $audience = self::AUDIENCE_ALL, + public readonly bool $isRequiredChild = false, public readonly ?int $id = null, ) {} + /** + * Whether the account holder is asked this question in their own right — true + * for everything except a child-audience question. + */ + public function askedOfSelf(): bool { + return self::AUDIENCE_CHILD !== $this->audience; + } + + /** + * Whether the account holder must answer before the form will submit. A + * child-audience question never reaches them, so it can never block them. + */ + public function isRequiredForSelf(): bool { + return $this->isRequired && $this->askedOfSelf(); + } + + /** + * Whether each student being registered must answer before the form will + * submit. Every question is asked in the student blocks whatever its audience, + * so this stands on its own. + */ + public function isRequiredForChild(): bool { + return $this->isRequiredChild; + } + public static function fromRow( \stdClass $row ): self { $options = null; if ( null !== $row->options && '' !== $row->options ) { @@ -70,16 +127,25 @@ class Question { : null; } + // `audience` and `is_required_child` arrived after the table did, so a row + // read on a site whose dbDelta has not run yet simply lacks them: the + // pre-existing behaviour (asked of everyone, required of nobody in + // particular) is the right reading of a question authored before the + // distinction existed. + $audience = Val::string( $row->audience ?? '' ); + return new self( - offeringId: Val::intOrNull( $row->offering_id ), - label: Val::string( $row->label ), - fieldType: Val::string( $row->field_type ), - options: $options, - isRequired: Val::bool( $row->is_required ), - sortOrder: Val::int( $row->sort_order ), - isActive: Val::bool( $row->is_active ), - scope: Val::string( $row->scope ), - id: Val::int( $row->id ), + offeringId: Val::intOrNull( $row->offering_id ), + label: Val::string( $row->label ), + fieldType: Val::string( $row->field_type ), + options: $options, + isRequired: Val::bool( $row->is_required ), + sortOrder: Val::int( $row->sort_order ), + isActive: Val::bool( $row->is_active ), + scope: Val::string( $row->scope ), + audience: in_array( $audience, self::VALID_AUDIENCES, true ) ? $audience : self::AUDIENCE_ALL, + isRequiredChild: Val::bool( $row->is_required_child ?? false ), + id: Val::int( $row->id ), ); } @@ -90,15 +156,17 @@ class Question { */ public function toArray(): array { return [ - 'id' => $this->id, - 'offering_id' => $this->offeringId, - 'scope' => $this->scope, - 'label' => $this->label, - 'field_type' => $this->fieldType, - 'options' => $this->options, - 'is_required' => $this->isRequired, - 'sort_order' => $this->sortOrder, - 'is_active' => $this->isActive, + 'id' => $this->id, + 'offering_id' => $this->offeringId, + 'scope' => $this->scope, + 'label' => $this->label, + 'field_type' => $this->fieldType, + 'options' => $this->options, + 'audience' => $this->audience, + 'is_required' => $this->isRequired, + 'is_required_child' => $this->isRequiredChild, + 'sort_order' => $this->sortOrder, + 'is_active' => $this->isActive, ]; } } diff --git a/src/Registration/QuestionController.php b/src/Registration/QuestionController.php index 61e7374..b504a4a 100644 --- a/src/Registration/QuestionController.php +++ b/src/Registration/QuestionController.php @@ -89,15 +89,25 @@ class QuestionController { return; } + // Audience and the students' own required-ness are asked for on the + // account-scope form only; an offering's questions are answered once about + // the student being booked, so there is no second audience to differ from. + // An offering question therefore mirrors its single "required" into both + // columns rather than storing a distinction it does not have. + $accountScope = null === $offering; + $audience = sanitize_key( Val::string( wp_unslash( $_POST['audience'] ?? '' ) ) ); + $this->questions->insert( new Question( - offeringId: null === $offering ? null : (int) $offering->id, - label: $label, - fieldType: $fieldType, - options: $this->parseOptions( sanitize_textarea_field( Val::string( wp_unslash( $_POST['options'] ?? '' ) ) ) ), - isRequired: isset( $_POST['is_required'] ), - sortOrder: absint( Val::int( $_POST['sort_order'] ?? 0 ) ), - scope: null === $offering ? Question::SCOPE_ACCOUNT : Question::SCOPE_OFFERING, + offeringId: $accountScope ? null : (int) $offering->id, + label: $label, + fieldType: $fieldType, + options: $this->parseOptions( sanitize_textarea_field( Val::string( wp_unslash( $_POST['options'] ?? '' ) ) ) ), + isRequired: isset( $_POST['is_required'] ), + sortOrder: absint( Val::int( $_POST['sort_order'] ?? 0 ) ), + scope: $accountScope ? Question::SCOPE_ACCOUNT : Question::SCOPE_OFFERING, + audience: $accountScope && in_array( $audience, Question::VALID_AUDIENCES, true ) ? $audience : Question::AUDIENCE_ALL, + isRequiredChild: $accountScope ? isset( $_POST['is_required_child'] ) : isset( $_POST['is_required'] ), ) ); // phpcs:enable WordPress.Security.NonceVerification.Missing diff --git a/src/Registration/QuestionEndpoint.php b/src/Registration/QuestionEndpoint.php index fc8bae3..010062b 100644 --- a/src/Registration/QuestionEndpoint.php +++ b/src/Registration/QuestionEndpoint.php @@ -88,14 +88,21 @@ class QuestionEndpoint { return $this->invalid( __( 'Invalid field type.', 'unsupervised-schedular' ) ); } + $isRequired = (bool) $request->get_param( 'is_required' ); + $question = new Question( - offeringId: $offeringId, - label: $label, - fieldType: $fieldType, - options: $this->sanitizeOptions( $request->get_param( 'options' ) ), - isRequired: (bool) $request->get_param( 'is_required' ), - sortOrder: Val::int( $request->get_param( 'sort_order' ) ), - isActive: null === $request->get_param( 'is_active' ) ? true : (bool) $request->get_param( 'is_active' ), + offeringId: $offeringId, + label: $label, + fieldType: $fieldType, + options: $this->sanitizeOptions( $request->get_param( 'options' ) ), + isRequired: $isRequired, + sortOrder: Val::int( $request->get_param( 'sort_order' ) ), + isActive: null === $request->get_param( 'is_active' ) ? true : (bool) $request->get_param( 'is_active' ), + // An offering asks its questions once, about the student being booked, + // so there is no second audience to differ from: the single "required" + // stands for both, the same way the upgrade backfill left every + // question authored before the two could differ. + isRequiredChild: $isRequired, ); $id = $this->questions->insert( $question ); @@ -129,16 +136,23 @@ class QuestionEndpoint { return $this->invalid( $this->tooLongMessage( __( 'question', 'unsupervised-schedular' ), Question::MAX_LABEL_LENGTH ) ); } + // Only offering-scope questions reach here — an account-scope one has no + // offering to own it and is turned away as not found above — so the same + // single "required" applies to everyone asked. See create(). + $isRequired = $request->has_param( 'is_required' ) ? (bool) $request->get_param( 'is_required' ) : $existing->isRequired; + $question = new Question( - offeringId: $existing->offeringId, - label: $label, - fieldType: $fieldType, - options: $request->has_param( 'options' ) ? $this->sanitizeOptions( $request->get_param( 'options' ) ) : $existing->options, - isRequired: $request->has_param( 'is_required' ) ? (bool) $request->get_param( 'is_required' ) : $existing->isRequired, - sortOrder: $request->has_param( 'sort_order' ) ? Val::int( $request->get_param( 'sort_order' ) ) : $existing->sortOrder, - isActive: $request->has_param( 'is_active' ) ? (bool) $request->get_param( 'is_active' ) : $existing->isActive, - scope: $existing->scope, - id: $id, + offeringId: $existing->offeringId, + label: $label, + fieldType: $fieldType, + options: $request->has_param( 'options' ) ? $this->sanitizeOptions( $request->get_param( 'options' ) ) : $existing->options, + isRequired: $isRequired, + sortOrder: $request->has_param( 'sort_order' ) ? Val::int( $request->get_param( 'sort_order' ) ) : $existing->sortOrder, + isActive: $request->has_param( 'is_active' ) ? (bool) $request->get_param( 'is_active' ) : $existing->isActive, + scope: $existing->scope, + audience: $existing->audience, + isRequiredChild: $isRequired, + id: $id, ); $this->questions->update( $id, $question ); diff --git a/src/Registration/QuestionField.php b/src/Registration/QuestionField.php index 4f8a777..f0e2838 100644 --- a/src/Registration/QuestionField.php +++ b/src/Registration/QuestionField.php @@ -21,12 +21,18 @@ class QuestionField { * the HTML attribute, for a block the browser must not block submission on * because it may not apply at all — the child blocks, which only count when * the parent/guardian box is ticked. The server validates those either way. + * + * `$isRequired` overrides which of the question's two required flags applies + * here — a question can be optional for the account holder and required for + * each student they register, and only the caller knows which block this is. + * Null falls back to the question's own {@see Question::$isRequired}. */ - public static function render( Question $question, string $name, string $id, bool $enforceRequired = true ): string { - $required = $question->isRequired && $enforceRequired ? ' required' : ''; + public static function render( Question $question, string $name, string $id, bool $enforceRequired = true, ?bool $isRequired = null ): string { + $mustAnswer = $isRequired ?? $question->isRequired; + $required = $mustAnswer && $enforceRequired ? ' required' : ''; $label = ''; return '

' . $label . self::input( $question, $name, $id, $required ) . '

'; diff --git a/src/Registration/QuestionRepository.php b/src/Registration/QuestionRepository.php index 7443f6b..5e8f196 100644 --- a/src/Registration/QuestionRepository.php +++ b/src/Registration/QuestionRepository.php @@ -15,7 +15,7 @@ class QuestionRepository { $this->db->insert( $this->table, $this->columns( $question ) + [ 'created_at' => current_time( 'mysql' ) ], - [ '%d', '%s', '%s', '%s', '%s', '%d', '%d', '%d', '%s' ] + [ '%d', '%s', '%s', '%s', '%s', '%s', '%d', '%d', '%d', '%d', '%s' ] ); return $this->db->insert_id; @@ -26,7 +26,7 @@ class QuestionRepository { $this->table, $this->columns( $question ), [ 'id' => $id ], - [ '%d', '%s', '%s', '%s', '%s', '%d', '%d', '%d' ], + [ '%d', '%s', '%s', '%s', '%s', '%s', '%d', '%d', '%d', '%d' ], [ '%d' ] ); } @@ -38,14 +38,16 @@ class QuestionRepository { */ private function columns( Question $question ): array { return [ - 'offering_id' => $question->offeringId, - 'scope' => $question->scope, - 'label' => $question->label, - 'field_type' => $question->fieldType, - 'options' => null === $question->options ? null : (string) wp_json_encode( $question->options ), - 'is_required' => $question->isRequired ? 1 : 0, - 'sort_order' => $question->sortOrder, - 'is_active' => $question->isActive ? 1 : 0, + 'offering_id' => $question->offeringId, + 'scope' => $question->scope, + 'label' => $question->label, + 'field_type' => $question->fieldType, + 'options' => null === $question->options ? null : (string) wp_json_encode( $question->options ), + 'audience' => $question->audience, + 'is_required' => $question->isRequired ? 1 : 0, + 'is_required_child' => $question->isRequiredChild ? 1 : 0, + 'sort_order' => $question->sortOrder, + 'is_active' => $question->isActive ? 1 : 0, ]; } @@ -128,4 +130,30 @@ class QuestionRepository { return null !== $sql && false !== $this->db->query( $sql ); } + + /** + * Give every question authored before students had a required-ness of their + * own the one it used to have. + * + * `is_required_child` arrives with `DEFAULT 0`, so without this a question the + * studio had marked required would quietly stop being required of the students + * a guardian registers — the case it most likely existed for. Copying + * `is_required` across preserves exactly the old behaviour: required of + * everyone, or of nobody. + * + * Run once, guarded by an option in {@see \Unsupervised\Schedular\Plugin::boot()}, + * so a question deliberately made optional for students afterwards is not + * quietly made required again. + * + * @return bool True when the statement ran, false if it could not be prepared + * or the query failed. + */ + public function backfillChildRequired(): bool { + $sql = $this->db->prepare( + 'UPDATE %i SET is_required_child = 1 WHERE is_required = 1', + $this->table + ); + + return null !== $sql && false !== $this->db->query( $sql ); + } } diff --git a/src/Schema.php b/src/Schema.php index 57390d3..c6f7a2c 100644 --- a/src/Schema.php +++ b/src/Schema.php @@ -85,7 +85,9 @@ class Schema { label VARCHAR(255) NOT NULL, field_type VARCHAR(20) NOT NULL DEFAULT 'text', options TEXT, + audience VARCHAR(20) NOT NULL DEFAULT 'all', is_required TINYINT(1) NOT NULL DEFAULT 0, + is_required_child TINYINT(1) NOT NULL DEFAULT 0, sort_order INT NOT NULL DEFAULT 0, is_active TINYINT(1) NOT NULL DEFAULT 1, created_at DATETIME NOT NULL, diff --git a/templates/admin/questions.php b/templates/admin/questions.php index b7ea7e6..463eba4 100644 --- a/templates/admin/questions.php +++ b/templates/admin/questions.php @@ -41,7 +41,7 @@ if (! defined('ABSPATH')) {

-

+

title)); ?>

@@ -76,10 +76,31 @@ if (! defined('ABSPATH')) { - - - - + + + + + +

+ + + + + +
+ +

+ + + + + + + + @@ -94,7 +115,12 @@ if (! defined('ABSPATH')) { - + + + + + + @@ -104,7 +130,32 @@ if (! defined('ABSPATH')) { sortOrder); ?> label); ?> fieldType); ?> - isRequired ? esc_html__('Yes', 'unsupervised-schedular') : esc_html__('No', 'unsupervised-schedular'); ?> + + + askedOfSelf() + ? esc_html__('Everyone', 'unsupervised-schedular') + : esc_html__('Students only', 'unsupervised-schedular'); + ?> + + + isRequiredForSelf()) { + $requiredOf[] = __('account holder', 'unsupervised-schedular'); + } + if ($question->isRequiredForChild()) { + $requiredOf[] = __('students', 'unsupervised-schedular'); + } + echo esc_html([] === $requiredOf ? __('—', 'unsupervised-schedular') : implode(', ', $requiredOf)); + ?> + + + isRequired ? esc_html__('Yes', 'unsupervised-schedular') : esc_html__('No', 'unsupervised-schedular'); ?> +
diff --git a/templates/frontend/family-page.php b/templates/frontend/family-page.php index 5a0dd92..d5f26ae 100644 --- a/templates/frontend/family-page.php +++ b/templates/frontend/family-page.php @@ -94,7 +94,12 @@ if (! defined('ABSPATH')) { id . ']', 'us-family-q-' . (int) $question->id); + echo QuestionField::render( + $question, + 'us_answers[' . (int) $question->id . ']', + 'us-family-q-' . (int) $question->id, + isRequired: $question->isRequiredForChild() + ); ?> diff --git a/templates/frontend/register-page.php b/templates/frontend/register-page.php index 97e0470..0a2e45c 100644 --- a/templates/frontend/register-page.php +++ b/templates/frontend/register-page.php @@ -22,7 +22,7 @@ if (! defined('ABSPATH')) { * @var string $loginUrl Where the post-confirmation sign-in link points. * @var string $error * @var list $policyForms - * @var list $accountQuestions Studio-wide questions, asked of every student — the account holder included when they are one. + * @var list $accountQuestions Studio-wide questions, asked of every student being registered — the account holder included when they are one, unless the question is for students only. */ ?> @@ -121,9 +121,21 @@ if (! defined('ABSPATH')) {

+ askedOfSelf()) { + continue; + } + ?> id . ']', 'us-reg-q-' . (int) $question->id); + echo QuestionField::render( + $question, + 'us_answers[' . (int) $question->id . ']', + 'us-reg-q-' . (int) $question->id, + isRequired: $question->isRequiredForSelf() + ); ?> @@ -150,7 +162,8 @@ if (! defined('ABSPATH')) { $question, 'children[0][answers][' . (int) $question->id . ']', 'us-child-0-q-' . (int) $question->id, - enforceRequired: false + enforceRequired: false, + isRequired: $question->isRequiredForChild() ); ?> diff --git a/tests/Unit/Auth/RegistrationPageTest.php b/tests/Unit/Auth/RegistrationPageTest.php index 323870c..49ed39a 100644 --- a/tests/Unit/Auth/RegistrationPageTest.php +++ b/tests/Unit/Auth/RegistrationPageTest.php @@ -1017,7 +1017,7 @@ class RegistrationPageTest extends TestCase ]; $this->ctx['questions']->shouldReceive('findByScope')->andReturn([ - new Question(offeringId: null, label: 'Instrument', isRequired: true, scope: Question::SCOPE_ACCOUNT, id: 7), + new Question(offeringId: null, label: 'Instrument', isRequired: true, scope: Question::SCOPE_ACCOUNT, isRequiredChild: true, id: 7), ]); Functions\when('email_exists')->justReturn(false); @@ -1247,4 +1247,157 @@ class RegistrationPageTest extends TestCase 'The account holder answers the questions above the students they are adding.' ); } + + /** + * A "students only" question describes a child being registered, so it is put + * to each student and never to the account holder about themselves. + */ + public function testAStudentsOnlyQuestionIsAskedOfTheStudentsAndNotOfTheAccountHolder(): void + { + $this->stubRenderContext(); + + $question = new Question( + null, + 'School and grade', + scope: Question::SCOPE_ACCOUNT, + audience: Question::AUDIENCE_CHILD, + isRequiredChild: true, + id: 7 + ); + $this->ctx['questions']->shouldReceive('findByScope')->with(Question::SCOPE_ACCOUNT, Mockery::any())->andReturn([$question]); + + $html = $this->ctx['page']->render([]); + + self::assertStringNotContainsString('name="us_answers[7]"', $html); + self::assertStringContainsString('name="children[0][answers][7]"', $html); + } + + /** + * The two required flags are read where each applies: the browser is asked to + * enforce the account holder's, and the students' block carries the marker + * without the attribute (it may not be in play at all). + */ + public function testTheFormMarksAQuestionRequiredWhereItActuallyIs(): void + { + $this->stubRenderContext(); + + $question = new Question( + null, + 'Previous experience', + scope: Question::SCOPE_ACCOUNT, + isRequired: false, + isRequiredChild: true, + id: 7 + ); + $this->ctx['questions']->shouldReceive('findByScope')->with(Question::SCOPE_ACCOUNT, Mockery::any())->andReturn([$question]); + + $html = $this->ctx['page']->render([]); + + // No `required` attribute on the account holder's copy, and no marker on + // its label — they may leave it blank. + self::assertStringContainsString('', $html); + self::assertStringContainsString('', $html); + + // The student's copy is marked required, without the attribute: the block + // may not be in play at all, so the server is what enforces it. + self::assertStringContainsString('', $html); + self::assertStringContainsString('', $html); + } + + /** + * The point of the two flags: an adult signing themselves up can leave the + * question blank, while every student they enrol must answer it. + */ + public function testAQuestionOptionalForYouIsStillRequiredOfEachStudent(): void + { + $_POST = [ + 'password' => 'thistle-marrow-42', + 'display_name' => 'Grace', + 'birth_year' => '1990', + 'us_registering_for' => RegistrationPage::FOR_BOTH, + 'us_answers' => ['7' => ' '], + 'children' => [['name' => 'Ada', 'birth_year' => '2015', 'answers' => [7 => ' ']]], + ]; + + $question = new Question(null, 'Instrument', isRequired: false, scope: Question::SCOPE_ACCOUNT, isRequiredChild: true, id: 7); + $this->ctx['questions']->shouldReceive('findByScope')->andReturn([$question]); + Functions\when('email_exists')->justReturn(false); + Functions\expect('wp_insert_user')->never(); + + $result = $this->submit(new Invite(email: 'a@b.test', token: 'hash'), false); + + // The student's blank is what stopped it — the account holder's was fine. + self::assertStringContainsString('for each student', $result); + } + + public function testTheAccountHolderMayLeaveBlankWhatTheirStudentsMustAnswer(): void + { + $_POST = [ + 'password' => 'thistle-marrow-42', + 'display_name' => 'Grace', + 'birth_year' => '1990', + 'us_registering_for' => RegistrationPage::FOR_BOTH, + 'us_answers' => ['7' => ' '], + 'children' => [['name' => 'Ada', 'birth_year' => '2015', 'answers' => [7 => 'Piano']]], + ]; + + $question = new Question(null, 'Instrument', isRequired: false, scope: Question::SCOPE_ACCOUNT, isRequiredChild: true, id: 7); + $this->ctx['questions']->shouldReceive('findByScope')->andReturn([$question]); + $this->ctx['guardians']->shouldReceive('createChild')->once()->andReturn(101); + $this->stubInviteSuccess(); + + $students = []; + $this->ctx['answers']->shouldReceive('insert')->andReturnUsing( + static function (Answer $answer) use (&$students): int { + $students[] = $answer->studentId; + return 1; + } + ); + + self::assertSame('invite', $this->submit(new Invite(email: 'a@b.test', token: 'hash'), false)); + + // Only the student answered, so only the student has an answer stored. + self::assertSame([101], $students); + } + + /** + * A question the account holder is never shown cannot be one they are held + * to, nor one an answer can be filed against them for — a crafted post that + * supplies both is ignored on both counts. + */ + public function testAStudentsOnlyQuestionNeitherBlocksNorStoresAgainstTheAccountHolder(): void + { + $_POST = [ + 'password' => 'thistle-marrow-42', + 'display_name' => 'Grace', + 'birth_year' => '1990', + 'us_registering_for' => RegistrationPage::FOR_BOTH, + 'us_answers' => ['7' => 'Crafted by hand'], + 'children' => [['name' => 'Ada', 'birth_year' => '2015', 'answers' => [7 => 'Grade 4']]], + ]; + + $question = new Question( + null, + 'School and grade', + isRequired: true, + scope: Question::SCOPE_ACCOUNT, + audience: Question::AUDIENCE_CHILD, + isRequiredChild: true, + id: 7 + ); + $this->ctx['questions']->shouldReceive('findByScope')->andReturn([$question]); + $this->ctx['guardians']->shouldReceive('createChild')->once()->andReturn(101); + $this->stubInviteSuccess(); + + $recorded = []; + $this->ctx['answers']->shouldReceive('insert')->andReturnUsing( + static function (Answer $answer) use (&$recorded): int { + $recorded[] = [$answer->studentId, $answer->answerValue]; + return 1; + } + ); + + self::assertSame('invite', $this->submit(new Invite(email: 'a@b.test', token: 'hash'), false)); + self::assertSame([[101, 'Grade 4']], $recorded); + } } diff --git a/tests/Unit/Guardian/FamilyPageTest.php b/tests/Unit/Guardian/FamilyPageTest.php index 210018a..608ed26 100644 --- a/tests/Unit/Guardian/FamilyPageTest.php +++ b/tests/Unit/Guardian/FamilyPageTest.php @@ -76,9 +76,21 @@ class FamilyPageTest extends TestCase return $page; } + /** + * A question required of everyone, or of nobody — the shape every question + * had before the account holder and the students could differ, and the shape + * the upgrade backfill leaves them in. + */ private function question(int $id, bool $required): Question { - return new Question(offeringId: null, label: 'Instrument', isRequired: $required, scope: Question::SCOPE_ACCOUNT, id: $id); + return new Question( + offeringId: null, + label: 'Instrument', + isRequired: $required, + scope: Question::SCOPE_ACCOUNT, + isRequiredChild: $required, + id: $id + ); } public function testLoggedOutVisitorIsOfferedALoginLink(): void @@ -158,6 +170,71 @@ class FamilyPageTest extends TestCase self::assertNull($captured); } + /** + * This screen only ever adds a student, so the students' required-ness is the + * one that applies: a question required of the account holder alone must not + * stop a guardian adding a child. + */ + public function testAddIsNotBlockedByAQuestionRequiredOnlyOfTheAccountHolder(): void + { + $_POST = [ + 'us_family_action' => 'add', + 'child_name' => 'Ada', + 'child_birth_year' => '2015', + 'us_answers' => [7 => ' '], + ]; + + $question = new Question( + offeringId: null, + label: 'Instrument', + isRequired: true, + scope: Question::SCOPE_ACCOUNT, + isRequiredChild: false, + id: 7 + ); + + $this->questions->shouldReceive('findByScope')->once()->andReturn([$question]); + $this->guardians->shouldReceive('createChild')->once()->andReturn(42); + + // Nothing was typed, so nothing is stored — but the add went through. + $this->answers->shouldNotReceive('insert'); + + $captured = null; + $this->capturingPage($captured)->maybeHandleSubmit(); + + self::assertSame('https://studio.test/family/?us_family=added', $captured); + } + + public function testAddIsBlockedByAQuestionRequiredOnlyOfTheStudents(): void + { + $_POST = [ + 'us_family_action' => 'add', + 'child_name' => 'Ada', + 'child_birth_year' => '2015', + 'us_answers' => [7 => ''], + ]; + + $question = new Question( + offeringId: null, + label: 'Instrument', + isRequired: false, + scope: Question::SCOPE_ACCOUNT, + isRequiredChild: true, + id: 7 + ); + + $this->questions->shouldReceive('findByScope')->once()->andReturn([$question]); + $this->guardians->shouldNotReceive('createChild'); + + $captured = null; + $page = $this->capturingPage($captured); + $page->shouldNotReceive('redirect'); + + $page->maybeHandleSubmit(); + + self::assertNull($captured); + } + public function testAddSurfacesAServiceErrorInsteadOfRedirecting(): void { $_POST = ['us_family_action' => 'add', 'child_name' => '']; diff --git a/tests/Unit/Registration/QuestionFieldTest.php b/tests/Unit/Registration/QuestionFieldTest.php new file mode 100644 index 0000000..4751aa7 --- /dev/null +++ b/tests/Unit/Registration/QuestionFieldTest.php @@ -0,0 +1,85 @@ +Your level?', $html); + self::assertStringContainsString('', $html); + } + + public function testARequiredQuestionIsMarkedAndEnforced(): void + { + $question = new Question(7, 'Your level?', isRequired: true, id: 3); + + $html = QuestionField::render($question, 'us_answers[3]', 'us-q-3'); + + self::assertStringContainsString('us-required', $html); + self::assertStringContainsString(' required', $html); + } + + /** + * A block that may not apply at all keeps the marker and drops the attribute, + * so the browser cannot refuse a submit over a field that is out of play. + */ + public function testNotEnforcingRequiredKeepsTheMarkerButDropsTheAttribute(): void + { + $question = new Question(7, 'Your level?', isRequired: true, id: 3); + + $html = QuestionField::render($question, 'us_answers[3]', 'us-q-3', enforceRequired: false); + + self::assertStringContainsString('us-required', $html); + self::assertStringNotContainsString(' required>', $html); + } + + /** + * Which of the question's two required flags applies depends on whose block + * this is, and only the caller knows that. + */ + public function testTheCallerCanOverrideWhichRequiredFlagApplies(): void + { + $question = new Question( + null, + 'Previous experience', + scope: Question::SCOPE_ACCOUNT, + isRequired: false, + isRequiredChild: true, + id: 3 + ); + + $forSelf = QuestionField::render($question, 'us_answers[3]', 'us-q-3', isRequired: $question->isRequiredForSelf()); + $forChild = QuestionField::render($question, 'children[0][answers][3]', 'us-child-0-q-3', isRequired: $question->isRequiredForChild()); + + self::assertStringNotContainsString('us-required', $forSelf); + self::assertStringNotContainsString(' required', $forSelf); + + self::assertStringContainsString('us-required', $forChild); + self::assertStringContainsString(' required', $forChild); + } + + public function testASelectRendersItsOptionsBehindAnEmptyChoice(): void + { + $question = new Question( + 7, + 'Pick a level', + fieldType: Question::FIELD_SELECT, + options: ['Beginner', 'Advanced'], + id: 3 + ); + + $html = QuestionField::render($question, 'us_answers[3]', 'us-q-3'); + + self::assertStringContainsString('