feat: honor per-activity CFP reopen in the submission edit gate - #90
Conversation
An admin can reopen submission for a single presentation for a time-boxed window. The portal decides editability on its own, so canEdit() has to honor that override and stop honoring it the moment the window lapses. canEdit() computed closed-ness once in the constructor, so a page opened before a deadline kept rendering the edit form past it. It is now computed per call, takes the server-synced clock, and mirrors the API's isSubmissionReopened() on all three of its invariants: the plan is enabled, its submission window has actually ended, and the grant is still live. The last two matter because "not open" also covers "has not started yet", and because a plan disabled after a grant does reach client state: navigating refetches the plan by id, and that endpoint applies no enabled filter, unlike the /me feed the client discovers plans through. The clock those comparisons read was seeded with Date.now(), milliseconds, where uicore's Clock, the time service and the API's datetime_epoch fields are all epoch seconds, so an active reopen window read as long expired. It now seeds epoch seconds, matching event-site, track-chairs and fnmeeting. Seeding was not sufficient on its own: clockState was persisted, and redux-persist's blacklist only filters outbound writes while autoMergeLevel2 merges every stored key back in, so a returning user rehydrated the old millisecond value onto the very render that decides whether to redirect to the preview page. That redirect is one way and preview offers only Done, so handling REHYDRATE in the reducer is what keeps a speaker off a dead end. The model also captured its selection plan at construction while the layout only ever handed it a fresh presentation payload, so it is now given a fresh plan too. Adds a banner naming the deadline while a grant is live, formatted and phrased like the submission deadline the app already renders. Known limitation: if the time service accepts a request and never answers, uicore's Clock never ticks and nowUtc stays frozen at its seed, so a window does not lapse client side. The server still refuses late writes. Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds live UTC clock state, time-aware presentation editability, selection-plan updates, and progress-link handling. The edit page displays a localized warning while a submission reopening window remains active. ChangesPresentation editing and reopening
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ClockReducer
participant ReduxStore
participant PresentationLayout
participant Presentation
participant EditPresentationPage
ClockReducer->>ReduxStore: publish current nowUtc
ReduxStore->>PresentationLayout: provide nowUtc
PresentationLayout->>Presentation: updateSelectionPlan(selectionPlan)
PresentationLayout->>Presentation: evaluate canEdit(nowUtc)
ReduxStore->>EditPresentationPage: provide nowUtc
EditPresentationPage->>Presentation: request getReopenedUntil(nowUtc)
EditPresentationPage->>EditPresentationPage: render localized warning
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/model/presentation.js`:
- Line 15: Update the submission-window checks in the presentation flow around
lines 158–179 to use the server-synchronized nowUtc argument for every
comparison. Replace any nowAfter or nowBetween calls that read the browser
clock, while preserving the existing open-window and redirect behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33ba363f-186d-49fc-9c89-13a0503cf53f
📒 Files selected for processing (7)
src/components/presentations-table.jssrc/i18n/en.jsonsrc/layouts/presentation-layout.jssrc/model/presentation.jssrc/pages/edit-presentation-page.jssrc/reducers/clock-reducer.jssrc/store.js
The reopen predicate mixed clocks in a single expression: submissionEnded read the browser clock through nowAfter, while the deadline comparison beside it read the server-synced nowUtc. The spec's rule is that the plan window check stays on local time for consistency with the rest of the app, and the short reopen check uses the trusted clock. This line belongs to the second, so it now compares against nowUtc. submissionIsClosed deliberately stays on nowBetween. It is the plan window check the spec keeps local, every other window check in the app agrees with it, and moving it would change the gate for everyone rather than for a reopened talk. Co-Authored-By: Claude <noreply@anthropic.com>
|
Taken partially, in 9823e80. Accepted for Declined for One note on the stated failure mode, since it reads more severe than it is. A grant can only exist if the server already saw the window as ended: Verified by running the model directly: the eleven-case |
There was a problem hiding this comment.
Pull request overview
This PR updates the speaker CFP portal’s “edit submission” gating so it honors a per-presentation reopen window (submission_reopened_until) and correctly re-evaluates editability over time using the server-synced clock, including rendering a warning banner while a reopen grant is active.
Changes:
- Reworked
Presentation.canEdit()/getProgressLink()to compute editability per call (time-aware) and honor per-activity reopen windows. - Fixed clock seeding to epoch seconds and prevented stale persisted clock data from rehydrating and incorrectly gating edits.
- Added a speaker-facing banner indicating the reopen deadline.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/store.js | Stops persisting clockState to avoid stale time rehydration. |
| src/reducers/clock-reducer.js | Seeds nowUtc in epoch seconds and overrides rehydration to prevent stale/incorrect values. |
| src/pages/edit-presentation-page.js | Displays a warning banner while submission_reopened_until is still in effect. |
| src/model/presentation.js | Updates editability logic to honor reopen windows and use nowUtc per call. |
| src/layouts/presentation-layout.js | Passes nowUtc into edit gating to redirect to preview when edits are no longer allowed. |
| src/i18n/en.json | Adds localized text for the reopen banner. |
| src/components/presentations-table.js | Routes progress links through getProgressLink(nowUtc) for time-aware navigation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } from '../actions/clock-actions'; | ||
| const localNowUtc = Date.now(); | ||
| // epoch SECONDS, matching the Clock ticks that replace it and every consumer of nowUtc | ||
| const localNowUtc = moment().unix(); |
There was a problem hiding this comment.
@caseylocker The millisecond→seconds seed fix is right, but SDS §5 asked for a different fix here and the other half of it didn't land: the canEdit() redirect is still evaluated against the seed, before the first Clock tick.
sds/per-activity-cfp-reopen.md:354-356 (rev 6) reads: "Fix: seed nowUtc: null and suppress the canEdit() redirect until the first Clock tick populates it", and §8:467-469 lists the matching test — "the editability redirect is suppressed until the first tick, not evaluated against a bad seed." The PR description covers the units correction and the REHYDRATE guard, but doesn't mention departing from the suppression, so I want to check it's deliberate rather than dropped.
Why it still matters with a seconds seed: uicore's Clock only sets state.timestamp inside processServerTimeResponse / processServerTimeResponseError, and tick() no-ops while it is null (lib/components/clock.js). So nowUtc holds the browser's clock for at least one time-service round trip. During that window presentation-layout.js:64 can redirect a speaker with a live grant to /preview if their device clock runs fast past submission_reopened_until — and that redirect is one-way: the guard is !location.pathname.endsWith('preview'), so a corrected tick a second later never brings them back, and preview-presentation-page.js offers only a Done button. This is the reopen window specifically, which is why the SDS flagged it: a 24h grant makes skew a far larger fraction than a multi-week plan window does.
Suggested fix — seed null and skip the redirect while it is null:
// clock-reducer.js
const DEFAULT_STATE = { nowUtc: null };
// presentation-layout.js:64
if (!isNew && nowUtc !== null && match.params.presentation_id == entity.id
&& !this.presentation.canEdit(nowUtc) && !location.pathname.endsWith('preview')) {One thing to verify before applying it: the other nowUtc consumers would then receive null too — presentation.js:114-128 (getStatus) and presentations-table.js:76 both currently assume a number, so they need a null branch or the same suppression.
There was a problem hiding this comment.
Deliberate omission on my part rather than a considered departure, so thank you for catching it. Fixed in ba8dc8a: DEFAULT_STATE and the REHYDRATE branch both now return nowUtc: null, and the redirect at presentation-layout.js is skipped while it is null. I confirmed your mechanism in the pinned uicore build before applying it: tick is null !== timestamp && (...) and timestamp is only assigned in processServerTimeResponse / processServerTimeResponseError, so onTick never fires and UPDATE_CLOCK never dispatches until the time service answers. The seconds seed fixed the 1000x symptom but left ordinary device skew, which is the part that matters for a 24h grant.
On your closing note about the other consumers, you were right and there were two more than the two you named:
getStatusnow returnsnullwhile the clock is unsynced. Every branch in it classifies the submission and selection windows againstnowUtc, andnullcoerces to0in those comparisons, so it would have rendered a confidently wrong status rather than none.presentations-table.jsrow click (handleEditPresentation, not just thegetStatuscall on line 76) no-ops while unsynced. It callsgetProgressLink, which askscanEdit, so pre-tick it resolved every reopened presentation to/previewand stranded the speaker through the same one-way guard.- The banner also read
nowUtcdirectly; it now goes through the model, per your other comment.
No test for it, per the standing decision that this repo has no test infrastructure and we are not adding any here. Verified by build and manual walkthrough.
| <div className="presentation-header-wrapper"> | ||
| <h2>{title} {`${selectionPlanSettings?.CFP_PRESENTATIONS_SINGULAR_LABEL || T.translate("edit_presentation.presentation")}`}</h2> | ||
| </div> | ||
| {entity.submission_reopened_until > nowUtc && |
There was a problem hiding this comment.
@caseylocker The banner condition drops the two invariants canEdit() deliberately added, so it can announce a deadline that isn't the operative one.
canEdit() requires three things (presentation.js:164,177,178) — plan enabled, window actually ended, grant still live — and those mirror the API's isSubmissionReopened() one-for-one (summit-api Presentation.php:2607 for IsEnabled(), :2614 for $now <= $submission_end_date). This banner checks only the third.
Concrete case: an admin grants a reopen on a closed plan, then extends the plan's submission_end_date past it — a routine ops action. The plan is open again, the speaker edits under normal open-window rules, and the banner tells them "Submission reopened until <grant expiry>. Finish your changes before it closes", naming a time that has no bearing on when they actually lose edit access. The comment you wrote at presentation.js:172-176 explains exactly why "not open" isn't the same as "ended"; the banner is the one place that reasoning wasn't applied.
Suggested fix — compute it once in the model and let the view ask, so the two can't drift:
// presentation.js
getReopenedUntil(nowUtc) {
if (!this._selectionPlan || this._selectionPlan.is_enabled === false) return null;
const until = this._presentation.submission_reopened_until;
if (!until) return null;
if (nowUtc <= this._selectionPlan.submission_end_date) return null;
return nowUtc < until ? until : null;
}
// edit-presentation-page.js
const reopenedUntil = presentation.getReopenedUntil(nowUtc);
{reopenedUntil && <div className="alert alert-warning">…formatEpoch(reopenedUntil)…</div>}That also lets canEdit() reuse it (const reopened = !!this.getReopenedUntil(nowUtc)), which keeps the gate and the banner on one definition.
There was a problem hiding this comment.
Agreed, and taken as suggested. ba8dc8a adds Presentation.getReopenedUntil(nowUtc) with all three invariants (plan enabled, window actually ended, grant live, plus null-safe for the clock change above). canEdit() now reads const reopened = !!this.getReopenedUntil(nowUtc) and the banner renders from the same call, so the gate and the display cannot drift.
Your extend-the-window case is the one that convinced me: it is a routine ops action and the banner named a time that had no bearing on when the speaker actually loses access.
Worth flagging that the same defect existed on the Show Admin side, keyed on the grant alone in both the offer control and the reopened-state display. I have fixed it there too and credited this comment in that PR (fntechgit/summit-admin#1042).
| this.props.getPresentation(newId); | ||
| } | ||
|
|
||
| this.presentation.updateSelectionPlan(newProps.selectionPlan); |
There was a problem hiding this comment.
@caseylocker Adding clockState to this component's mapStateToProps puts componentWillReceiveProps — and the whole edit-form subtree — on a 1 Hz loop.
Before this PR the layout wasn't subscribed to the clock, so this hook ran only when the entity, plan, track or route changed. Now nowUtc changes every second, so every second we re-run updateSelectionPlan() plus updatePresentation(), and the latter is not cheap or side-effect free: it recomputes getAllowedMediaUploads() and getAllowedTags() (which maps and filters over tagGroups × track.allowed_tags), rewrites this._steps[].showInNav, and mutates the redux entity in place via this._presentation.progressNum = currentStep.step (presentation.js:63-77). React then reconciles the entire EditPresentationPage tree behind it. All of that to re-evaluate one boolean.
development-practices.md § Performance is explicit that hot paths — render loops, polling — must not redo work when the input hasn't changed. The per-tick render is exactly what makes the form lock at the right second, so that part should stay; it's the recomputation that has no reason to run on a clock tick.
Suggested fix — gate the hook on the props it actually depends on, and leave the render-side canEdit(nowUtc) to consume the tick:
componentWillReceiveProps(newProps) {
const oldId = this.props.match.params.presentation_id;
const newId = newProps.match.params.presentation_id;
if (newId && oldId !== newId) this.props.getPresentation(newId);
if (newProps.selectionPlan !== this.props.selectionPlan)
this.presentation.updateSelectionPlan(newProps.selectionPlan);
if (newProps.entity !== this.props.entity || newProps.track !== this.props.track)
this.presentation.updatePresentation(newProps.entity, newProps.track);
}Worth confirming the identity check holds for entity: presentation-reducer.js:115,127 both build a new object on RECEIVE_PRESENTATION / PRESENTATION_UPDATED, so a real update always changes the reference — but updatePresentation() mutating progressNum in place means a save that changes only that field wouldn't. Worth a check before merging this guard.
There was a problem hiding this comment.
Fixed in ba8dc8a, gated exactly as you suggested. I verified the identity check you flagged as worth confirming: presentation-reducer.js builds a new object on both RECEIVE_PRESENTATION and PRESENTATION_UPDATED, so newProps.entity !== this.props.entity detects real changes and skips clock-only ticks.
One thing worth adding to the cost you listed: updatePresentation also writes this._presentation.progressNum = currentStep.step, and _presentation is the redux entity object itself, so this was mutating store state once per second rather than only burning CPU.
The per-tick re-render stays, since that is what makes the form lock at the right second.
There was a problem hiding this comment.
src/components/presentations-table.js:93 — outside this PR's diff hunk, so GitHub won't let me anchor to it; file-level comment instead.
@caseylocker Not a change request on this PR — recording the Delete-button defect here so the analysis lives next to the code rather than only in the Copilot thread that got resolved.
The guard on the Delete button is dead, and has been since it was written:
{!presentation.submissionIsClosed && presentation.canDelete() && <button … >}Presentation has never defined a submissionIsClosed member. On master the class only declared the underscore-prefixed private field (presentation.js:42), read internally by canEdit() (:150); there is no getter and no assignment to the un-prefixed name anywhere in the file. So presentation.submissionIsClosed is undefined, !undefined is true, and the first term is a constant.
That leaves the condition as canDelete() alone (presentation.js:198-203), which checks !is_published and selection_plan_id === selectionPlan.id and nothing time-related. The Delete button therefore renders on any unpublished presentation of the plan, including long after the submission window closed.
No data is at risk — the server does enforce it. PresentationService::deletePresentation (summit-api :590-592):
$can_delete_closed_submissions = is_null($current_user) || $current_user->isAdmin();
if ($presentation->isSubmissionClosed() && !$can_delete_closed_submissions)
throw new ValidationException(...)So the user-visible defect is a destructive action offered in the UI that always fails once the window is closed.
Confirming your read in the resolved Copilot thread: this PR did not cause it. Removing _submissionIsClosed changed nothing, because nothing outside the class ever read it. Your call to leave it alone here is right — the fix belongs in canDelete(), and it would change delete behaviour for every submission rather than for reopened ones:
canDelete(nowUtc) {
if (!this._selectionPlan) return false;
if (this._presentation.is_published) return false;
if (this._presentation.selection_plan_id !== this._selectionPlan.id) return false;
return !this.isSubmissionClosed(nowUtc); // the check canEdit() already computes
}presentations-table.js already has nowUtc in scope (mapStateToProps:121), so the call site becomes {presentation.canDelete(nowUtc) && …}. Worth its own ticket so it gets tested on its own terms.
Three items from smarcet's review, kept in one commit because they interleave within presentation.js and presentation-layout.js and cannot be separated by path. 1. Seed nowUtc as null rather than the browser clock, and suppress the canEdit() redirect until the first Clock tick, as SDS section 5 asked for. uicore's Clock leaves timestamp null until the time service answers and tick() no-ops until then, so a seed survives a whole round trip; a device clock running fast past submission_reopened_until read a live grant as expired, and that redirect is one-way. The other nowUtc consumers get null branches as flagged: getStatus renders nothing rather than a wrong status (null coerces to 0 in its window comparisons), and the presentations-table row click no-ops rather than resolving every reopened presentation to /preview, which was sticky through the same guard. 2. Add Presentation.getReopenedUntil() as the single definition of an operative grant: plan enabled, window actually ended, grant live. canEdit() gates on it and the banner displays it, so the banner can no longer announce a deadline that nothing gates on, which is what happened once an admin extended submission_end_date past a grant. 3. Gate componentWillReceiveProps on the props each call actually reads. Subscribing the layout to the clock put updateSelectionPlan and updatePresentation on a 1 Hz loop, and updatePresentation recomputes allowed media uploads and grouped tags, rewrites step visibility, and writes progressNum onto the redux entity. The per-tick re-render stays, since that is what locks the form on time. Co-Authored-By: Claude <noreply@anthropic.com>
| // getProgressLink asks canEdit, so before the first Clock tick it would resolve every | ||
| // reopened presentation to /preview, and the layout's redirect guard makes that one-way. | ||
| // Do nothing until the clock is real rather than navigate somewhere we cannot come back from. | ||
| if (nowUtc == null) return; |
There was a problem hiding this comment.
@caseylocker The nowUtc == null guard trades a wrong navigation for a silent dead click, and in the degraded mode the PR's Known-limitation paragraph describes, that dead click is indefinite.
If the time-service request hangs — accepted, never answered, never errored — uicore's Clock never assigns state.timestamp: tick() is null !== timestamp && (...), the error fallback (processServerTimeResponseError, which seeds moment().unix()) only fires on a rejected fetch, and the fetch has no timeout of its own. Until the browser's fetch timeout finally rejects it — minutes, not seconds — nowUtc stays null, so every title click here no-ops with no feedback and getStatus(nowUtc) renders the status column blank. The Known-limitation section covers the frozen-window symptom ("a window does not lapse client side") but not this navigation dead-end, which is the part a speaker would actually notice and report.
The guard itself is right — navigating through getProgressLink on a wrong clock is the one-way /preview strand this PR exists to prevent. Suggested fix, either level is fine:
// minimal: make the disabled state visible instead of silent
<a onClick={ev => handleEditPresentation(ev, presentation)}
className={nowUtc == null ? "disabled" : ""}
title={nowUtc == null ? T.translate("presentations.clock_syncing") : ""}>{p.title}</a>or simply extend the PR description's Known-limitation paragraph to state that while unsynced the table is non-navigable and statuses are blank, so the tradeoff is on record. Recovery paths already exist (visibilitychange re-sync, browser fetch timeout → error fallback), so I'd not block on either.
There was a problem hiding this comment.
Took the documentation option, and you were more right than the comment claimed: the Known limitation paragraph was not just incomplete, it was wrong about the state. It said nowUtc "stays frozen at its seed". There is no seed. clock-reducer.js:25 seeds null, which is the entire reason the guard you are looking at exists. Rewritten.
Your mechanism checks out end to end in the installed bundle: initial state = {timestamp: null}, tick is null !== e && (...) so the interval no-ops forever while null, getServerTime is a bare fetch with no AbortController, and processServerTimeResponseError is only reachable from .catch. Nothing here gates the table's render on the clock either, so the dead click is exactly as long-lived as you describe.
The paragraph now records all three symptoms rather than only the frozen window: non-navigable table, with the reason the no-op is deliberate rather than an oversight, and the blank status column.
One thing worth adding, because it is also the workaround: the block is only on this navigation path. presentation-layout.js:79 gates its redirect on nowUtc != null, so a speaker arriving at the edit URL directly, from the admin's deep link, a bookmark or browser back, is unaffected and edits normally. So the table is non-navigable while unsynced, but the app is not stuck. That is in the paragraph too, alongside the recovery paths you named.
Left the visible-disabled affordance out. It needs a new string and a disabled style for a state that self-heals on tab re-focus, and you said you would not block on either, so I would rather not add UI for it inside this PR. Happy to take it if you would rather see it.
| // every null to empty string; a falsy check covers both | ||
| const until = this._presentation.submission_reopened_until; | ||
| if (!until) return null; | ||
| if (nowUtc <= this._selectionPlan.submission_end_date) return null; |
There was a problem hiding this comment.
@caseylocker getReopenedUntil mirrors three of the API's four invariants — it omits the null-end-date guard, so the stated one-for-one mirror has a gap.
Presentation::isSubmissionReopened() returns false when the plan has no submission end date (summit-api Presentation.php:2609-2610: if (is_null($submission_end_date)) return false;). Here, a null/undefined submission_end_date makes nowUtc <= this._selectionPlan.submission_end_date evaluate false, so the check falls through and the grant is honored: the form renders and the banner announces a deadline, while the server refuses every save.
Reachability is admittedly thin: the reopen endpoint refuses to stamp a grant on a plan without an end date (PresentationSubmissionReopenService.php:70-72), so it takes the end date being cleared after a grant exists — I have not verified admin tooling even allows that. But the doc comment on this method claims a full mirror of isSubmissionReopened(), and this is the one branch where it isn't, at the cost of one line:
if (!this._selectionPlan.submission_end_date) return null;placed before the nowUtc <= comparison. Falsy check on purpose, matching the file's own convention for API-coerced empties (null on list feeds, '' after the detail-feed reducer coercion).
There was a problem hiding this comment.
Confirmed and fixed in c1cb9b1. Your citations are exact: Presentation.php:2610 on merged main is if (is_null($submission_end_date)) return false;, and PresentationSubmissionReopenService.php:71-72 refuses to stamp the grant in the first place. The doc comment claimed a full mirror and enforced three of four, so I took your line verbatim, falsy rather than null for the reason you gave, and corrected the comment to say four.
Two things I found verifying it that make this worse than the write-refusal you described.
It gates edit access, not just the banner. nowBetween is local and unguarded (src/utils/methods.js:54-57): now < (null * 1000) is now < 0, false. So a missing end date reads as a closed window, submissionIsClosed is true, and with the grant wrongly honored canEdit() rests entirely on it. The form renders and is editable. With the guard, canEdit() returns false at the submissionIsClosed && !reopened line. So this was not only a bogus banner.
The refusal is a 500, not a validation error. PresentationService.php:547 evaluates !$current_selection_plan->isSubmissionOpen() first, which reaches SelectionPlan::getStageStatus(), which dereferences $start_date->format(...) and $end_date->format(...) inside a Log::debug(sprintf(...)) at lines 668-678, before the if (empty($start_date) || empty($end_date)) return null; at line 680. sprintf arguments evaluate eagerly, so a null end date throws before its own guard is reached. Pre-existing on main, unrelated to either PR, and I am not touching it here. Worth noting because it means the speaker would have filled in a form and then taken a 500 rather than a clear message, which raises what this one line is worth.
On reachability, which you flagged as unverified: SummitSelectionPlanValidationRulesFactory.php:35,65 has submission_end_date as nullable, and required_with:submission_begin_date only binds when begin is sent, so the API's own validation permits the state. I did not trace whether the update service treats an explicit null as clear or as unchanged, so I am not calling it confirmed reachable, only not structurally blocked.
No test: this repo has no test harness at all, no jest and no test script, so there is nothing to add without introducing one. Verified by reasoning through the three falsy shapes plus ended-and-live, not-yet-ended and expired, and yarn build-dev compiles.
There was a problem hiding this comment.
src/layouts/presentation-layout.js:84 — outside this PR's diff hunk, so GitHub won't let me anchor to it; file-level comment instead.
@caseylocker Not a change request on this PR — recording a latent crash sitting three lines below the redirect this PR modified, so it doesn't stay undocumented. It is pre-existing: the line is byte-identical on master.
The !speaker branch references a variable that doesn't exist in scope:
if (!speaker) {
history.push(`/app/${summit.slug}/all-plans/profile`);
}render() destructures match, entity, speaker, history, loading, location, selectionPlan, selectionPlansSettings, nowUtc — no summit. There is no module-scope or imported summit in this file either, so if the branch ever fires, summit.slug throws a ReferenceError and the render crashes, instead of redirecting the speaker-less user to the profile page as intended. The value is available the whole time: mapStateToProps already maps summit: baseState.summit; it just never gets pulled out of props.
Reachability is the open question: speaker is baseState.speaker, and a logged-in member without a speaker profile navigating to a presentation URL would land here — I have not traced whether an upstream guard makes that state unreachable in practice. Either way the branch as written can only ever crash or no-op; it cannot do what it says.
Fix is one word — add summit to the destructuring on line 71:
let { match, entity, speaker, history, loading, location, selectionPlan, selectionPlansSettings, nowUtc, summit } = this.props;Since it predates the branch and is not on a line this PR changes, keeping it out of this PR is the right call — same reasoning as the canDelete() item. Worth folding both into the same cleanup ticket. (Side note for that ticket: history.push during render is itself a side effect worth converting to a <Redirect/> like the line above it.)
There was a problem hiding this comment.
Noted, and agreed it stays out of this PR for the reason you give: it predates the branch and is not on a line this PR touches.
getReopenedUntil() claimed to mirror the API's isSubmissionReopened(), but it enforced three of that method's four invariants: the API returns false when the plan has no submission end date (Presentation.php:2610) and it had no equivalent. Without the guard the comparison below it is nowUtc <= 0, since null and '' both coerce and undefined gives NaN. That is false, so the check falls through and the grant is honored. nowBetween() reads the same missing end date as a closed window, so canEdit() then rests entirely on the grant and the form renders, while the banner announces a deadline nothing enforces. Nothing valid is behind that: the reopen endpoint refuses to stamp a grant on a plan without an end date, so reaching this state takes the end date being cleared after the fact, and every write from the reopened form fails once it is. Falsy check rather than a null check, matching the file's convention for API-coerced empties. Reported by smarcet on PR #90. Co-Authored-By: Claude <noreply@anthropic.com>
* feat: add reopen and close submission-period thunks
Co-Authored-By: Claude <noreply@anthropic.com>
* feat: offer a reopen submission control on the activity form
Co-Authored-By: Claude <noreply@anthropic.com>
* feat: show the active reopen window, close action and speaker link
Co-Authored-By: Claude <noreply@anthropic.com>
* refactor: dedupe speaker deep-link URL into a local const
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: gate CFP reopen on selection plan, disable Reopen w/o valid hours
Final review fix wave for the per-activity CFP reopen feature:
- Gate the reopen block on entity.selection_plan_id > 0 (matches the
existing > 0 convention at the ProgressFlags gate a few lines down),
not just isPresentation()/!isNew(). Without a selection plan the
server 412s the reopen call, so the button previously offered an
action that could only fail; this also removes the only path that
could produce a /all-plans/null/ deep link.
- Disable the Reopen button when no valid hours value is selected
(e.g. Custom left blank/non-numeric), so it doesn't silently no-op.
- Make isSubmissionReopened() null-safe with optional chaining, since
epochToMomentTimeZone can return undefined (not null) when
time_zone_id is missing.
- Drop the unreferenced edit_event.reopen_deep_link_unavailable i18n
key.
Updated the event-form test fixtures (selection_plan_id + a matching
selectionPlansOpts entry) so existing reopen-flow tests still clear
the new gate, and added focused tests for the no-selection-plan and
disabled-button cases.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: reject non-positive custom reopen hours
parseInt let "-1" through as a truthy negative, so the Reopen button
stayed enabled, the confirm dialog named a deadline in the past, and
the request went out only for the server to 412 it.
Guard at the getter rather than at each call site, so the existing
truthiness checks in handleReopenSubmission and the button's disabled
prop both become correct without changing either.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs: record why the reopen thunks omit a terminal catch
An API error rejects the returned promise and the form fire-and-forgets
it, which is the same shape as saveEvent and every other write thunk in
this file. Matching the repo convention was a deliberate choice, so
record it where the next reader will look rather than in a PR comment.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: read the reopen thunks from props on the activity page
The page destructured every other action from props but not these two, so
the JSX referenced the module import instead of the dispatch-bound prop.
Calling it returned an un-dispatched thunk: the Reopen and Close now
buttons issued no request and raised no error.
Unit tests could not catch this. EventForm is presentational and its
tests inject onReopenSubmission directly, so the page wiring is never
exercised; the full suite was green with the buttons inert. Found by
driving the real app against the dev API.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: swallow the expected 412 rejection on the reopen handlers
The no-client-cap design makes an over-ceiling hours value an expected
outcome, not a fault: the server answers 412 and snackbarErrorHandler
puts its message ("hours must be between 1 and 168.") in front of the
admin, which is the only way the ceiling is discoverable. uicore's
response handler rejects after running that handler, so every such 412
also escaped as an unhandled rejection, reaching Sentry as a fault and
raising a full-screen error overlay in development.
Catch at the two call sites rather than in the thunks, keeping them the
same shape as saveEvent and the other write thunks.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: require a positive integer for custom reopen hours
parseInt read "1.5" and "1e3" as 1, so both enabled the button and would
have silently granted a one hour window instead of what the admin typed.
"1e3" also never reached the server's 412 for 1000 hours, which is the
only way the ceiling is discoverable.
Verified in Chrome rather than assumed: a number input preserves the raw
"1e3" and "1.5" in .value, so this is reachable, not a jsdom artifact.
The test sets the value instead of typing it because jsdom normalises a
typed "1e3" to "1000", which would pass against the old code.
Co-Authored-By: Claude <noreply@anthropic.com>
* test: guard the activity page against forwarding raw action imports
Covers the regression fixed in 4e0d009, where the page forwarded the
module import instead of the dispatch-bound prop and the reopen controls
issued no request while the suite stayed green.
Asserts on store.dispatch rather than on the action creator. Once the
actions module is mocked the raw import and the connect-bound prop are
the same jest.fn, so asserting the creator was called passes even with
the bug present; only the dispatch assertion fails. Verified by
reintroducing the defect: both tests fail, and the creator assertion is
not the one that catches it.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: ignore a reopen or close response for a different activity
Both responses merged into the single current entity with no check of
which activity they belonged to. Granting or revoking on activity A and
navigating to B before the request finished left B showing A's grant,
with A's deadline and attribution and a deep link for a window that was
never opened on B.
The close action already carried its eventId and the reducer simply
never read it; the reopen action now carries one too, and both cases
drop a response whose id does not match the loaded entity. The guard
returns the same state object, so a discarded response cannot trigger a
re-render.
Reproduced in the browser by holding the request and navigating mid
flight: without the guard the other activity flips to the reopened
state when the response lands, with it the activity is untouched.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: bring the reopen thunks in line with the async-thunk conventions
Convention pass against the show-admin playbooks, including the async
lifecycle rules proposed in ftn-docsnsklz PR #64.
- Dispatch startLoading() before the token await. A refresh can take
seconds, and anything dispatched after it leaves a window where the UI
is neither blocked nor marked in-flight.
- Move stopLoading() into .finally(). snackbarErrorHandler happens to
clear it on the error paths today, so this fixes no live defect, but
.then() alone is one library change away from a stuck overlay.
- Give the custom-hours input an accessible name. It had only a
placeholder, which is not a name, while the select beside it already
had a label. The test now selects it by label rather than placeholder.
- Report success through snackbarSuccessHandler. The feature already
used the MUI confirm dialog and the MUI error snackbar, so a
SweetAlert success left it mixing two feedback systems. The one
pre-existing showSuccessMessage call in this file pairs Swal with
authErrorHandler, which is consistent in the legacy direction; this
makes ours consistent in the MUI direction.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: only surface the reopen block once the plan window has ended
The API reopens a submission only when three things hold: the selection
plan is enabled, its submission window has actually ended, and a grant
is live. The UI keyed on the grant alone, so it offered a Reopen button
the server could only answer 412 for, and it announced a deadline that
was no longer the operative one.
The case that matters: an admin grants a reopen, then extends the plan's
submission_end_date past it. The speaker is editing under normal
open-window rules again, but the panel still said "Reopened until
<grant expiry>". Gating the whole block covers both symptoms, since the
granted display lives inside it.
Found by smarcet's review of the same invariant on the
call-for-presentations side (fntechgit/call-for-presentations#90).
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: read the reopen helpers' entity from state, not props
The render gate reads entity from state while isReopenApplicable() and
getReopenDeadline() read it from props, and the two diverge:
handleChangeSelectionPlan writes selection_plan_id into state without
saving, and componentDidUpdate only syncs props into state, never back.
So after an admin picks a different plan in the dropdown, the gate reads
the new plan's id from state while eligibility is judged against the
persisted plan from props. The control could stay visible for a plan
whose window is still open, or hide for one that is eligible. Nothing
incorrect reached the server, which judges against the persisted plan,
but the control's visibility reflected a plan other than the one shown.
handleReopenSubmission and handleCloseSubmission move too, so the id
they submit comes from one source.
Not unit tested: every existing test passes one entity object as a prop
which the component copies into state, so both sources are identical and
no test can reproduce the divergence. Reproducing it needs the Selection
Plan dropdown driven for real; left to manual verification.
Reported by CodeRabbit on PR #1042.
Co-Authored-By: Claude <noreply@anthropic.com>
* feat: cap the reopen hours client side from CFP_MAX_REOPEN_HOURS
The custom hours entry accepted any positive integer, so an over ceiling
value was only refused by the server's 412, after the admin had already
confirmed the dialog. SDS section 6 specifies the custom entry capped at
MAX_REOPEN_HOURS.
The ceiling is server side config exposed by no serializer or route, so
mirror it as a deploy time env value rather than hardcoding it: the same
.env.example plus window.* mapping the repo already uses for
CFP_APP_BASE_URL. Named CFP_MAX_REOPEN_HOURS to match the server's own
var so the two twins are greppable together.
Applied in getSelectedReopenHours(), which drives the button's disabled
state, so it covers the presets as well as the custom entry: a ceiling
configured below 72 cannot offer a preset the server would refuse. The
max attribute alone would not enforce anything, since a number input
still accepts a typed over max value.
Unset means uncapped, so a deployment that never sets it behaves exactly
as before and the server's 412 stays the authoritative backstop. Values
arrive from dotenv as strings, hence the Number coercion.
Suggested by smarcet on PR #1042.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: reject a reopen duration that overflows the deadline calculation
getSelectedReopenHours() accepted any digit-only value, and with no
ceiling configured nothing bounded it. Past roughly 2.4e9 hours
moment().add() yields an invalid date, unix() gives NaN, and uicore's
epochToMomentTimeZone returns NaN unwrapped rather than a moment, so the
confirm dialog's .format() throws.
The method is async and wired straight to onClick, so it surfaced as an
unhandled rejection: the button did nothing, no dialog, no message to the
admin, and a TypeError to Sentry. 9999999999 is enough to trigger it.
Guard on the deadline being representable rather than on the magnitude of
the input. A safe-integer check is not the boundary, since that value
passes one and still overflows, and every unsafe integer is far past the
range anyway, so one condition covers both.
Only reachable where CFP_MAX_REOPEN_HOURS is unset, since any configured
ceiling rejects these first.
Reported by CodeRabbit on PR #1042.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
ref: https://app.clickup.com/t/86bba82ph
Speaker-facing half of Per-Activity CFP Reopen. An admin reopens submission for a single presentation for a time-boxed window; this makes the CFP portal honor that override, and stop honoring it the moment the window lapses.
Spec is section 5 of
sds/per-activity-cfp-reopen.md.Depends on summit-api #581
submission_reopened_untilis served bySubmissionPresentationSerializer, which ships in OpenStackweb/summit-api#581. Until that merges and deploys, the field is absent from every payload this app reads, the new condition is simply falsy, and behavior is identical to today. Safe to merge and deploy ahead of the backend, but it does nothing until the backend lands.What changed
canEdit()decided editability from a snapshot taken in the constructor, so a page opened before a deadline kept rendering the edit form past it. It now computes per call and mirrors the API'sisSubmissionReopened()on all three of its invariants:The second and third matter more than they look. "Not open" also covers "has not started yet", and the API refuses a grant there. A plan disabled after a grant does reach client state, because navigating refetches the plan by id and that endpoint applies no enabled filter, unlike the
/mefeed the client discovers plans through.The clock those comparisons read was seeded with
Date.now(), milliseconds, while uicore'sClock, the time service and the API'sdatetime_epochfields are all epoch seconds. An active reopen window therefore read as long expired. It now seeds epoch seconds, matchingevent-site,track-chairsandfnmeeting.Seeding was not sufficient on its own.
clockStatewas persisted, and redux-persist'sblacklistonly filters outbound writes whileautoMergeLevel2merges every stored key back in, so a returning user rehydrated the old millisecond value onto the very render that decides whether to redirect to/preview. That redirect is one way and preview offers only a Done button, so a returning speaker with a live grant could be stranded. HandlingREHYDRATEin the reducer is what prevents that.Also adds a banner naming the deadline while a grant is live, formatted and phrased like the submission deadline the app already renders in the header.
Verification
No test infrastructure exists in this repository, so this was verified by driving the app against
api.dev.fnopen.complus targeted node checks against the real modules.Browser, on a closed plan with the payload injected (the backend field does not exist on dev yet):
/previewwith no bannerNode, against the real
Presentationmodel and the real redux-persist reducer chain:canEdit()truth table including pre-open, disabled, and absent-is_enabledpayloadsclockStaterejected on rehydrate, with normal ticks unaffectedNot observed in a browser:
presentations-table.js's progress link, which required test data that could not be created without the backend. It threads an already-verified value into an already-verified function, and the adjacent pre-existing call on the same prop proves the value is in scope.Known limitation
If the time service accepts a request and never answers, uicore's
Clocknever ticks andnowUtcstaysnull, which is whatclock-reducer.jsseeds and what every consumer here treats as "no answer yet".Clock.getServerTime()is a barefetchwith no timeout of its own, and its error fallback only fires on a rejection, so this state lasts until the browser's own network timeout finally rejects the request. While it lasts:getProgressLinkaskscanEditand a wrong answer there resolves to/preview, which the layout's redirect guard makes one-way. Doing nothing is recoverable, navigating is notgetStatushas no answer to give without a clockThe edit route itself is unaffected, so the admin's deep link, a bookmark or browser back all still reach the form. Recovery is automatic on tab re-focus, via the
visibilitychangere-sync, or when the hung request finally errors.Making the dead click visible rather than silent would need a disabled affordance and a new string; fixing the underlying hang means putting a timeout in the shared uicore
Clock. Both are out of scope here.Mid-session grant discovery remains a v1 gap per decision D8: a page already open when a grant is issued does not unlock until the speaker reloads or follows the admin's deep link. This app has no refetch path and adding one is a feature, not a fix.
Summary by CodeRabbit
New Features
Bug Fixes