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('