Repository navigation
Conversation
Add an `opens` timestamp to forms, the counterpart to `expires`. Before that time a form does not accept responses and the public link shows a "not yet open" page instead of the questions. An opening date must be before the expiration date. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ferdinand Thiessen <[email protected]>
Add an opening date picker to the form settings. Forms that are not yet open show when they will open, both in the navigation and in place of the questions. The opening and expiration date pickers prevent selecting an opening date after the expiration date. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ferdinand Thiessen <[email protected]>
Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ferdinand Thiessen <[email protected]>
susnux
added
enhancement
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Chartman123
left a comment
Chartman123
left a comment
There was a problem hiding this comment.
Two comments and one typo/incorrect wording
| /** | ||
| * Throws if the opening date is invalid or not before the expiration date | ||
| */ | ||
| private function checkOpeningDateUpdate(Form $form, array $keyValuePairs): void { |
There was a problem hiding this comment.
Do we need the same check for expiration date?
| .fromNow() | ||
| return t('forms', 'Opens {relativeDate}', { relativeDate }) | ||
| } | ||
| if (props.form.expires) { |
There was a problem hiding this comment.
Only show expiration for open forms?
|
|
||
| const opens = moment().add(1, 'hour') | ||
| if (formExpires.value && opens.unix() >= props.form.expires) { | ||
| emit('update:formProp', 'opens', moment().unix()) |
There was a problem hiding this comment.
✅ Verified live at
dd588c85: on an expired form the PATCH returns 400, an "Error while saving form" toast appears, and the switch stays on, reading "Opened on …". ❌ The days in the picker are not disabled:disabledDateis not a prop ofNcDateTimePicker9 (see theSettingsSidebarTab.vue:613thread).
When an expiration date is already set and now + 1h >= expires, this falls back to opens = now. If the form has already expired (expires < now), that value is still >= expires, so the PATCH fails with 400 Opening date must be before expiration date. The switch flips on locally anyway, because the error is only toasted and not rolled back and every day in the picker is disabled by . When it does succeed, isInvalidOpeningDayopens = now is a no-op opening date that immediately reads "Opened on …". It would be better to refuse to enable the switch, or show a hint, when there is no future window before expires.
There was a problem hiding this comment.
One more case on the same branch: when the expiration date is less than an hour away (but not yet passed), this falls back to opens = now. The PATCH succeeds, so the switch shows "on" and the picker reads "Opened on {date}", but the form is already open and responses are accepted immediately. The owner believes an opening date is in force when it has no effect. It is probably better to not enable the switch (or to show a hint) when there is no valid future opening time before expires.
| return props.form.expires > 0 && moment().unix() > props.form.expires | ||
| }) | ||
|
|
||
| const isNotYetOpen = computed<boolean>(() => { |
There was a problem hiding this comment.
✅ Verified live at
dd588c85: a logged-in respondent and an anonymous visitor on the public link both still saw "Form not yet open" 20–25 s after the opening time. A reload showed the form.
computed caches moment().unix(). It only re-evaluates when props.form.opens changes, not when time passes. A respondent who loads the page shortly before the opening time keeps seeing "Form not yet open" until they reload manually, even though the server is already accepting submissions. The same holds for the server-rendered public notyetopen empty-content page. A cheap fix is a timer, or a reload scheduled at opens.
There was a problem hiding this comment.
This is the same for the expiration I guess? So somenone could currently be on a Form that is about to expire and then can't send it
There was a problem hiding this comment.
Yes, you could also look at it the other way around in the opposite scenario which has the same effect, true.
So can be fine as is or cosmetic for sure.
| </template> | ||
| </NcEmptyContent> | ||
| <NcEmptyContent | ||
| v-else-if="isNotYetOpen" |
There was a problem hiding this comment.
This branch also applies to the owner or editors opening the form via "View". Together with loadFormForSubmission throwing This form is not yet taking answers, nobody can preview or test-submit a scheduled form before it opens, and preparing a form before its opening time is the main use case for this feature. Consider skipping the not-yet-open state, or showing it only as an info banner, for users with edit permission.
There was a problem hiding this comment.
Also the same for expired forms...
There was a problem hiding this comment.
So keep as is consistency reason or change in both cases (also for consistency reason)? Same for the above I guess
| } | ||
|
|
||
| if (array_key_exists('opens', $keyValuePairs) | ||
| && (!is_int($keyValuePairs['opens']) || $keyValuePairs['opens'] < 0)) { |
There was a problem hiding this comment.
⚠️ Partly corrected after a live check (see the reply below): theopens/expiresasymmetry is confirmed; theexpires: nullpart is not, because it is stored as0.
The validation is lopsided. opens is rejected unless it is a strict PHP int, so a numeric string from a form-encoded client gets a 400. expires gets no type or sign check at all: {"expires": -5} passes this check and is stored as -5. {"expires": null} passes this check (null falls back to the stored value through ??), and then reaches setExpires.opens the way the entity addType(integer) already does.
There was a problem hiding this comment.
Partial correction after a live check (test instance built from dd588c85):
- confirmed:
openssent as a numeric string ("1793384000") gets 400 "Invalid opening date", whileexpires: -5is accepted and stored as-5; - not reproduced:
{opens: <now+1h>, expires: null}returns 200, but the storedexpiresis0, notnull, so the form is not treated as expired. Please ignore that part of the comment.
The point that stands is the asymmetry: expires gets no type or sign check, while opens does.
| return null; | ||
| } | ||
|
|
||
| $table->addColumn('opens', Types::INTEGER, [ |
There was a problem hiding this comment.
✅ Verified live at
dd588c85(MariaDB 10.6): a 2039 opening date returns 500 withSQLSTATE[22003](see the reply below).
Types::INTEGER is a 32-bit signed column, and the date picker allows any future date. Choosing an opening date after 2038-01-19 overflows the column (on MariaDB 10.6 this is a 500 with an out-of-range error; other databases were not tested). expires has the same limit, but a new column is the cheap moment to use Types::BIGINT.
There was a problem hiding this comment.
I checked this on a running instance (MariaDB 10.6). It is worse than a silent clamp: PATCH {opens: 2200000000} (2039-09-17) returns 500 Internal Server Error with SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'opens', and the date picker lets the owner pick that date. Using Types::BIGINT for the new column avoids it. Pre-existing expires has the same limit, but there's no reason to add a second such column.
| * | ||
| * @param datetime the datepicker Date | ||
| */ | ||
| const isInvalidOpeningDay = (datetime: Date): boolean => |
There was a problem hiding this comment.
❌ Corrected after a live check: these validators are never called, so the point below about them running on every calendar cell is wrong (struck through).
disabledDate and disabledTime are not props of NcDateTimePicker 9. Its declared props include min and max (both Date), but no disabledDate/disabledTime (checked in @nextcloud/vue 9.13.1's NcDateTimePicker.vue.d.ts). So isInvalidOpeningDay, isInvalidOpeningTime, isInvalidExpirationDay and isInvalidExpirationTime have no effect. On a running instance built from dd588c85, the opening picker of an expired form showed no disabled day at all, past days included. The commit's promise that the pickers "prevent selecting an opening date after the expiration date" therefore doesn't hold in the UI. Only the server check in checkOpeningDateUpdate catches it, with an "Error while saving form" toast.
The expiration picker passes the same unsupported props (notBeforeToday/notBeforeNow) on main, so the gap is older than this PR, but the new cross-checks between the two pickers depend on it. Suggested fix: express the limits through :min / :max (e.g. :min="new Date()" and :max="expirationDate" on the opening picker), which also makes the per-cell callbacks unnecessary.
Original comment:
The four new validators rebuild moment(props.form.expires, "X") and moment(props.form.opens, "X") on every calendar cell and time-slot callback. The expirationDate and openingDate computeds already hold exactly these Dates, so datetime > expirationDate.value and similar comparisons would do the same job with less code (the day variants still need startOf/endOf).
| return t('forms', 'Expires {relativeDate}.', { relativeDate }) | ||
| }) | ||
|
|
||
| const openingMessage = computed<string>(() => { |
There was a problem hiding this comment.
The "not yet open" logic is now duplicated three times (Submit.vue isNotYetOpen, AppNavigationForm formSubtitle, SettingsSidebarTab isOpened), and this message string is duplicated from the FormsEmptyContent notyetopen model. A shared helper or composable next to the existing expiry checks would keep them in sync, especially if the reactive-time fix above is applied.
| $form->setOwnerId($user->getUID()); | ||
| $form->setAccess($formData['access']); | ||
| $form->setExpires($formData['expires']); | ||
| $form->setOpens($formData['opens'] ?? 0); |
There was a problem hiding this comment.
ℹ️ Not verified live: the test instance has no
user_migrationapp, so this comes from the code only.
The new invariant (opens >= 0 and opens < expires when both are set) is enforced only in ApiController::checkOpeningDateUpdate. Import goes through here unchecked, so an export with "opens": 1800000000, "expires": 1700000000 creates a form that can never open. Moving the invariant into a shared helper (for example in FormsService), and calling it from both the update and import paths, would close this gap.
| <NcDateTimePicker | ||
| id="opensDatetimePicker" |
There was a problem hiding this comment.
Accessibility: with this PR the settings sidebar contains two date pickers, and both have the same accessible name. Neither the opening picker nor the expiration picker passes ariaLabel, so each input falls back to the library default ("Datepicker input", or "Eingabe Datumsauswahl" in German), and the "Set opening date" / "Set expiration date" switch above it is not programmatically associated with the input. A screen-reader user tabbing through the sidebar hears two identical "Datepicker input" text boxes and can only tell them apart by their current value text (WCAG 2.4.6 Headings and Labels, 1.3.1 Info and Relationships).
I checked this on a running instance built from dd588c85. The input's accessibility tree was textbox "Eingabe Datumsauswahl", with no aria-labelledby and no <label>. axe does not flag it, because each input technically has a name.
NcDateTimePicker (9.13.1) already has the props for this, so the fix is two lines per picker:
| <NcDateTimePicker | |
| id="opensDatetimePicker" | |
| <NcDateTimePicker | |
| id="opensDatetimePicker" | |
| :ariaLabel="t('forms', 'Opening date')" | |
| :ariaLabelMenu="t('forms', 'Choose opening date')" |
The same goes for expiresDatetimePicker at line 90 ('Expiration date' / 'Choose expiration date'). That picker had this gap before this PR, but it only becomes ambiguous now that there are two of them.
| return $this->provideEmptyContent(Constants::EMPTY_EXPIRED, $form); | ||
| } | ||
|
|
||
| if (!$this->formsService->hasFormOpened($form)) { |
There was a problem hiding this comment.
✅ Verified live at
dd588c85: before the opening date, a respondent the form is shared with gets 200 with all questions fromGET /forms/{id}(submitting returns 403). The Submit view's own request carries the question text too.
This check only protects the rendered page (and loadFormForSubmission protects the submit endpoints). ApiController::getForm (GET /api/v3/forms/{formId}, line 275) only calls getFormIfAllowed($formId, Constants::PERMISSION_SUBMIT) and never looks at opens, so before the opening date anyone the form is shared with still gets the full form, including all questions and options.
Scenario: an owner shares a quiz or exam form directly with users and sets it to open tomorrow. A shared user calls GET /ocs/v2.php/apps/forms/api/v3/forms/{id} (or just looks at the Submit view's network request) and reads every question before the form opens. The "Form not yet open" empty content only hides them in the UI.
If keeping questions confidential until the opening date is part of the feature's promise, getForm should not return the questions to users with only submit permission while !hasFormOpened($form). Owners and users with edit or results permission still need them. Otherwise it's worth stating explicitly that the opening date only gates submissions, not visibility.
| throw new OCSForbiddenException('This form is no longer taking answers'); | ||
| } | ||
|
|
||
| if (!$this->hasFormOpened($form)) { |
There was a problem hiding this comment.
✅ Verified live at
dd588c85: after a future opening date was set, the respondent's/{hash}/submit/{submissionId}page shows "Form not yet open", andPUT /submissions/{id}returns 403.
loadFormForSubmission is used by newSubmission, updateSubmission (ApiController.php:1546) and uploadFiles (:1742), so this check also blocks editing existing submissions while a form is not yet open.
Scenario: a live form with allowEditSubmissions = true has already received responses. The owner sets opens to next Monday to pause it. Respondents opening /{hash}/submit/{submissionId} now see "Form not yet open", and a PUT to update their submission returns 403 "This form is not yet taking answers".
That may be intended ("paused means paused"), but it is a behaviour change for existing respondents and isn't covered by a test either way. If it's not intended, the check should apply only to new submissions (and their uploads), not to updateSubmission.
| const expirationDate = computed(() => | ||
| moment(props.form.expires, 'X').toDate(), | ||
| ) | ||
| const isOpened = computed(() => props.form.opens <= moment().unix()) |
There was a problem hiding this comment.
isOpened is a computed over moment().unix(), so it is cached and only re-evaluates when props.form.opens changes, not when time passes. If the sidebar stays open across the opening time, the picker label keeps saying "Opens on {date}" after the form has opened, until something else changes opens or the component remounts.
Same root cause as isNotYetOpen in Submit.vue. A shared reactive "now" (e.g. a ref updated by a timer, or useNow if already available) would fix both, or this could be a plain function evaluated in stringifyOpeningDate, since the picker calls format on render anyway.
Review summary: live verification at
|
| Finding | Live result |
|---|---|
Questions readable through the API before the opening date (PageController.php:206 thread) |
✅ a shared respondent's GET /forms/{id} returns 200 with all questions, and the Submit view's own request carries them; submitting returns 403 |
| Owner/editors can't preview or test before opening | ✅ the owner's View shows "Form not yet open" with no questions; the owner's submit returns 403 |
| Pausing with a future opening date blocks editing existing submissions | ✅ /{hash}/submit/{submissionId} shows "Form not yet open"; PUT /submissions/{id} returns 403 |
| "Not yet open" view doesn't update at the opening time | ✅ for a logged-in respondent and on the public link, still shown 20–25 s after opening; fixed by a reload |
| Sidebar label keeps "Opens on" after opening | ✅ the navigation entry also kept "Opens in a few seconds" |
| New: the date validators are never called | ✅ disabledDate/disabledTime aren't NcDateTimePicker 9 props; no day is disabled, past days included (SettingsSidebarTab.vue:613 thread, which replaces the earlier performance remark) |
| Enabling the opening date on an expired form | ✅ PATCH 400, "Error while saving form" toast, the switch stays on reading "Opened on …"; ❌ the days are not disabled (see the previous row) |
| Enabling it less than an hour before expiry | ✅ opens is saved as "now", the switch is on, the form is already open |
32-bit opens column |
✅ worse than described: a 2039 date returns 500 (SQLSTATE[22003], MariaDB 10.6) |
opens/expires validation asymmetry |
✅ strict int vs. expires: -5; ❌ the expires: null part is withdrawn (stored as 0) |
| Date pickers share one accessible name | ✅ seen in the accessibility tree |
Import bypasses the opens < expires rule (FormsMigrator.php) |
code-level: user_migration isn't installed on the test instance |
| "Not yet open" logic copied three times | code-level (maintainability) |
| <NcDateTimePicker | ||
| id="opensDatetimePicker" | ||
| :clearable="false" | ||
| :disabled="locked" |
There was a problem hiding this comment.
✅ Verified live on the dev instance (PR head
dd588c85): on a form withlockedUntil = 0, the opening picker still opens; picking a day and pressing Enter sentPATCH {opens}→ 200, andlockedUntilwent from0totime() + 900. The other-user-lock case below was traced in code only.
disabled isn't an NcDateTimePicker 9 prop either, just like disabledDate/disabledTime (see the :613 thread). The component doesn't set inheritAttrs: false or forward $attrs to the inner VueDatePicker, so :disabled="locked" lands as an inert attribute on the wrapper <div>. The picker stays interactive while the switches around it are disabled (NcCheckboxRadioSwitch does have a disabled prop). The same goes for :editable and :showSecond.
Two effects:
- Form locked by another user: the picked date is applied optimistically, the PATCH fails in
obtainFormLockwith 403 "Form is currently locked by another user.", and the rejected date stays on screen until reload. - Form permanently locked by the owner (
lockedUntil = 0, which makesisSidebarLockedtrue for the owner too): the owner can still change the opening date. The PATCH passesobtainFormLockbecause the owner holds the lock, andobtainFormLockthen setslockedUntil = time() + 15 * 60, which is persisted. The permanent lock silently becomes a 15-minute lock.
main already had :disabled="locked" on the expiration picker, so the gap is older than this PR, but the PR adds a second control with it. One fix: render the date as plain text when locked, or return early from onOpeningDateChange/onExpirationDateChange when props.locked.
| const relativeDate = moment(props.form.opens, 'X') | ||
| .locale(window.OC?.getLanguage()) | ||
| .fromNow() | ||
| return t('forms', 'Opens {relativeDate}', { relativeDate }) |
There was a problem hiding this comment.
✅ Verified live on the dev instance (PR head
dd588c85): with the account language set to Catalan and an opening date ~1 h ahead, the navigation entry readsOpens d'aquí una hora.
t() HTML-escapes placeholder values by default, and the subtitle is rendered as text, so an apostrophe in moment's relative time shows up as a literal entity. In Catalan the subtitle reads Opens d'aquí una hora, because the ca future string is d'aquí %s with an ASCII apostrophe. In Italian it reads Opens tra un'ora. On main, the Expires {relativeDate} strings here and in Submit.vue already have this problem. SharingSidebarTab.vue shows the fix for a text-rendered placeholder: pass { escape: false, sanitize: false } as the options argument of t(). The LLL dates are not affected with moment 2.31.0, whose Catalan month names use a typographic apostrophe.
| import IconSchedule from '@material-symbols/svg-400/outlined/schedule.svg?raw' | ||
| import { loadState } from '@nextcloud/initial-state' | ||
| import { t } from '@nextcloud/l10n' | ||
| import moment from '@nextcloud/moment' |
There was a problem hiding this comment.
✅ Verified live: built
main(be06e1fb) and the PR head (dd588c85) and compared the import closure offorms-emptyContent.mjs. The sharedNcContentchunk grows from 22 KB to 323 KB (moment-with-locales lands there), and the gzipped total anotfoundpage loads grows from 171 KB to 248 KB. Loading/s/<unknown hash>on the instance fetches that chunk.
This pulls moment into the standalone forms-emptyContent entry, which had no moment dependency before. @nextcloud/moment imports moment/min/moment-with-locales.js (about 390 KB minified / 79 KB gzipped with the pinned moment 2.31.0), and nothing else in that entry loads it. The bundle serves every empty-content page: notfound (any stale /s/ or /embed/ link), expired, and the new notyetopen. The notyetopen model is built at module scope, so the notfound and expired pages also format the epoch for a string they never show. Formatting with the platform avoids the dependency, e.g. new Date(loadState('forms', 'opens', 0) * 1000).toLocaleString(getCanonicalLocale(), { dateStyle: 'long', timeStyle: 'short' }), computed in setup() only when renderAs === 'notyetopen'.
| * | ||
| * @param datetime the opening Date | ||
| */ | ||
| const onOpeningDateChange = (datetime: Date | [Date, Date] | null): void => { |
There was a problem hiding this comment.
Nit: this copies onExpirationDateChange with a different key, and the two already convert differently: moment(datetime).unix() here, parseInt(moment(datetime).format('X')) there. Binding both pickers to a single onDateChange(key, datetime) would keep them the same.
| $this->assertEquals($expected, $this->formsService->hasFormOpened($form)); | ||
| } | ||
|
|
||
| public static function dataLoadFormForSubmission() { |
There was a problem hiding this comment.
Nit: dataLoadFormForSubmission() is a verbatim copy of dataHasFormOpened() above. Putting #[DataProvider('dataHasFormOpened')] on testLoadFormForSubmissionOpeningDate would do the same job.
| - To transfer the ownership of a form to another user, you must only send a single _keyValuePair_ containing the key `ownerId` and the user id of the new owner. | ||
| - To link a file for submissions, the _keyValuePairs_ need to contain the keys `path` and `fileFormat` | ||
| - To unlink a file for submissions, the _keyValuePairs_ need to contain the keys `fileId` and `fileFormat` need to contain the value `null` | ||
| - If both `opens` and `expires` are set (not `0`), `opens` must be before `expires` |
There was a problem hiding this comment.
Nit (docs): the new 403 This form is not yet taking answers from loadFormForSubmission isn't documented anywhere. The submission section's 403 list (line 1031) still reads "access-settings, form expired, already submitted", and the @throws list on newSubmission (ApiController.php, around line 1438) only names "This form is no longer taking answers". uploadFiles and updateSubmission go through the same check.
Verified live: POST /api/v3/forms/{id}/submissions before the opening date returns 403 This form is not yet taking answers.
AndyScherzinger
left a comment
AndyScherzinger
left a comment
There was a problem hiding this comment.
Four more nits from the same pass, cut from the earlier review to keep it short. None of them change behaviour.
| const onFormExpiresChange = (checked: boolean): void => { | ||
| if (checked) { | ||
| emit('update:formProp', 'expires', moment().add(1, 'hour').unix()) | ||
| const start = formOpens.value |
There was a problem hiding this comment.
Nit: the ternary isn't needed. When the opening date is off, props.form.opens is 0, and moment.max(moment(), moment(0, 'X')) is already moment(). const start = moment.max(moment(), moment(props.form.opens, 'X')) does the same in one line.
| if (!checked) { | ||
| emit('update:formProp', 'opens', 0) | ||
| return | ||
| } | ||
|
|
||
| const opens = moment().add(1, 'hour') | ||
| if (formExpires.value && opens.unix() >= props.form.expires) { | ||
| emit('update:formProp', 'opens', moment().unix()) | ||
| } else { | ||
| emit('update:formProp', 'opens', opens.unix()) |
There was a problem hiding this comment.
Nit: three emit('update:formProp', 'opens', …) calls where one would do. Computing the value first and emitting once reads more easily and makes the fallback easier to fix (see the :733 thread), e.g.
let opens = 0
if (checked) {
const inOneHour = moment().add(1, 'hour').unix()
opens = formExpires.value && inOneHour >= props.form.expires ? moment().unix() : inOneHour
}
emit('update:formProp', 'opens', opens)| ->willReturn([]); | ||
| $this->userSession->method('isLoggedIn')->willReturn(true); | ||
|
|
||
| $initialStates = []; |
There was a problem hiding this comment.
Nit: the $initialStates capture (the provideInitialState callback) is copied in both new tests. A small helper such as captureInitialStates(): array (returning by reference or via a property), or doing it once in setUp(), would keep the two tests to the parts that differ.
| 'inputJson' => '[{"title":"Link","description":"","created":1646251830,"access":{"permitAllUsers":false,"showToAllUsers":false},"expires":0,"state":0,"lockedBy":null,"lockedUntil":null,"maxSubmissions":null,"isAnonymous":false,"submitMultiple":false,"allowEditSubmissions":false,"showExpiration":false,"lastUpdated":123456789,"confirmationEmailEnabled":true,"confirmationEmailSubject":"Thanks","confirmationEmailBody":"Body","confirmationEmailQuestionId":null,"allowComments":false,"questions":[{"id":14,"order":2,"type":"multiple","isRequired":false,"text":"checkbox","description":"huhu","extraSettings":{},"options":[{"text":"ans1"}]}],"submissions":[{"userId":"anyUser@localhost","timestamp":1651354059,"answers":[{"questionId":14,"text":"ans1"}]}]}]', | ||
| 'expectedOpens' => 0, | ||
| ], | ||
| 'withOpeningDate' => [ |
There was a problem hiding this comment.
Nit: withOpeningDate repeats the whole ~1 KB exactlyOneOfEach JSON with only "opens":1700000000 added. Building both from one decoded array (json_encode(array_merge($base, ['opens' => 1700000000]))) would make the difference between the cases visible and leave one copy to maintain.
Allow to configure a date when the form will be opened, similar to the expiry date.
🤖 AI (if applicable)