feat(meeting-types): use the workplan grid on the create form - #147
Merged
Conversation
The line claimed migrations V1…V25 while V26 and V27 already exist. That range is the one fact in the section that rots on every migration added, and it carries no information a directory listing does not — the rules around it (validate-only Hibernate, never edit an applied migration, add a new V*.sql) are what actually matter and do not change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QF9JXW4V2AFDBicwBfV5zH
Two working documents, no code change. calit-7a6t records the two-week trial of JetBrains Context as a semantic search layer: the measured 2-win/1-miss result against grep, the usage baseline, keep-or-remove criteria for 2026-09-07, and the removal steps. Its .properties blind spot is the reason the verdict is not a foregone conclusion. The trial's own config (CLAUDE.md block, settings.json hooks, .mcp.json entry) is deliberately left uncommitted. The plan implements calit-9d76: point the meeting-type create form at the same workplan grid the edit page uses, and delete the second ruleDay/ruleStart/ruleEnd parser it needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QF9JXW4V2AFDBicwBfV5zH
The create form had a stripped-down working-hours block — seven fixed rows, one start/end pair per weekday — while the edit page and /me/availability both use the workplan grid with multiple frames per day and copy buttons. The create form now renders the same grid, and the create handler reuses persistFrames instead of a second ruleDay/ruleStart/ruleEnd parser, which is deleted. One blank frame per day is still seeded server-side, so the form works unchanged without JavaScript. Closes #120 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
…ded frameDay
createInitialDateOverride still took (typeId, ownerId) while the new
persistFrames call right above it takes (ownerId, typeId) - both Long,
so a future swap between the two adjacent calls would compile clean and
write a cross-tenant ownerId. Reorder createInitialDateOverride's
parameters to (ownerId, typeId, form), matching persistFrames and the
AvailabilityRule.delete("ownerId = ?1 and meetingTypeId = ?2", ...)
convention used elsewhere; no behaviour change.
Also pin the create form's seeded frameDay to its own day: nothing
previously asserted that the hidden input's value matches the day card
it renders inside, so a typo'd constant there would have passed every
test in the file (the no-JS test hand-writes its own frameDay params
instead of deriving them from the rendered markup).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
Marks calit-9d76 completed with a summary of what landed, and files the three items the final code review surfaced but left out of scope: a shared workplan Qute fragment (with the duplicated i18n key families collapsed), the unguarded overrideDate parse that 500s the create endpoint, and the missing accessible names on the grid time inputs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
…ge input createInitialDateOverride called LocalDate.parse(date) with no guard, so a crafted overrideDate on POST /me/meeting-types threw DateTimeParseException out of the transaction and 500ed instead of dropping the value. Pre-existing, but the workplan-grid change made the asymmetry conspicuous: the working-hours half of the same form now routes through persistFrames, which catches parse failures and skips them, while the date-override half directly below it still crashed on garbage. Guard the overrideDate parse the same way persistFrames does (catch and skip, keep the meeting type). persistWindows - the shared helper this path and two other AdminResource endpoints call - had the identical unguarded LocalTime.parse for window start/end; guarded that too, which closes the window-time hole for its other two callers as a side effect. Investigated whether a "bulk-save" date-override endpoint already guards, per calit-8nlx's notes: none exists. Two sibling single-add endpoints (addTypeOverride, createOverride) still parse overrideDate unguarded and share the crash risk, but fixing + testing them touches two more resource methods and two other test files outside this bean's scope - left alone and noted as a follow-up candidate in the bean. SharedMeetingsResource.addOverride already guards both date and window parsing via its own helpers, confirming this is the established safe pattern elsewhere in the codebase. Closes calit-8nlx. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
calit-qec2 claimed addTypeOverride and createOverride both 500 on a malformed date, and that createOverride also 500s on a non-numeric meetingTypeId. Verified empirically with real requests instead of propagating the assumption: the malformed-date half was already wrong. MalformedDateTimeMapper is a globally-registered ExceptionMapper<DateTimeParseException>, so both endpoints already return 400 on a bad date, not 500. The meetingTypeId half was real: Long.valueOf(meetingTypeId) had no mapper for NumberFormatException, so a non-numeric id genuinely 500ed. createOverride now catches it and throws BadRequestException, matching the existing parseLocationType precedent (AdminResource.java:671) - no new i18n key needed, since that precedent already uses a plain message. Added tests pinning the actual behaviour for all three inputs (kept regardless of which way the premise turned out): a garbage date on addTypeOverride, a garbage date on createOverride, and a garbage meetingTypeId on createOverride - all now assert 400 and no persisted row. Corrected calit-8nlx's Summary of Changes to match: its fix's value was never "500 -> 200" - the pre-fix status was already 400, but because createInitialDateOverride shared its transaction with the meeting type's own persist(), that 400 rolled back the entire create, not just the date override. Corrected the docs-site changelog bullet (worktree /home/finkel/work_self/calit-docs-site) to describe "whole request rejected -> bad value skipped, rest saves" instead of "server error", amending the unpushed commit that first stated it. Closes calit-qec2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
The title still asserted a 500 the body now documents as never having happened; the create path rejected the whole form with a 400 from the global DateTimeParseException mapper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
…ragment The working-hours weekly grid existed verbatim in four templates (availability.html, meetingTypeDetail.html, sharedAvailability.html, and meetingTypes.html's create form), each with its own i18n key twin family for the same four button strings. Reviewed on PR #147 and asked for the opposite of what landed there. Extracts the day-cards + <template data-frame-template> markup into AdminResource/_workplanGrid.html, parameterised by `week` (List<WeekRow>) and `cardClass`; all four templates now include it instead of carrying their own copy. The including page still owns the <form> (data-workplan, action, CSRF). The create form previously seeded rows from a bare DayOfWeek[]; it now gets a List<WeekRow> via new WeekRow.blank() (one unpersisted, blank AvailabilityRule per day), matching the shape the three bulk-edit pages already used. The fragment guards the frame-time value expressions with {#if fr.startTime}...{/if} so a blank frame renders value="", never the literal text "null" -- a no-op for the three pages with real (NOT NULL) frame times. Collapses the three duplicated adm_detail_*/adm_availability_*/ adm_meetingTypes_* frame-button key families onto the existing adm_workplan_* family (adm_workplan_clear_day already lived there), removing the 13 now-orphaned keys from AdminMessages.java and both adm_de.properties/adm_he.properties. adm_detail_to and adm_meetingTypes_to stay -- still used by the date-override window rows. Pure refactor: no test was edited to make the suite pass. Full suite (mvn test): 905 tests, 0 failures, 0 errors, 0 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
… in tests Review follow-up on the workplan-grid extraction (098f561): - WeekRow.blank()'s AvailabilityRule.dayOfWeek assignment was dead -- _workplanGrid.html emits the hidden frameDay from row.day, never fr.dayOfWeek. Dropped it rather than keep an assignment nothing reads. - AdminMeetingTypeFormTest.createFormRendersWorkplanGrid now asserts name="frameStart" value="" on the seeded blank frame, pinning that the {#if fr.startTime} guard renders an empty value and not the literal text "null" -- the most fragile part of the no-JS guarantee, previously unpinned by any committed test. mvn test -Dtest=AdminMeetingTypeFormTest,AdminTypeHoursPrefillTest,\ AdminAvailabilityBulkTest,AdminAvailabilityTest,MultiHostMessageParityTest: 14+3+6+2+4 = 29 tests, 0 failures, 0 errors, 0 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
The shared-fragment work is done; the leftover DayOfWeek[] template parameter that no page reads any more gets its own bean, since dropping it spans four templates and three Templates signatures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
|
asm0dey
added a commit
that referenced
this pull request
Aug 22, 2026
Two Unreleased bullets for PR #147: the meeting-type create form now uses the same working-hours grid as the edit page, and malformed dates, times or meeting-type ids on that form no longer reject the whole submission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The meeting-type create form had a stripped-down working-hours block — seven fixed rows, one
ruleStart/ruleEndpair per weekday — with its own server-side parser, while the edit page and/me/availabilityboth used the richer workplan grid. The create form now renders that same grid, andAdminResource.persistFramesbecomes the app's only frame parser.Closes #120.
What changed
meetingTypes.html— theruleDay/ruleStart/ruleEndrows are replaced by the workplan grid: per-day cards,+ Frame, copy-to-all-days, copy-to-weekdays, clear-day, a<template data-frame-template>and/workplan.js.data-workplangoes on the create<form>itself, since the page is one big form and that is whatworkplan.js'sclosest("[data-workplan]")resolves against.AdminResource—createMeetingTypecallspersistFrames(ownerId, typeId, form); the second parsercreateInitialWorkingHoursis deleted. Nothing insrc/mainreadsruleDay/ruleStart/ruleEndany more.adm_meetingTypes_*keys, withdeandhevalues copied verbatim from their existingadm_detail_*twins.Progressive enhancement
One blank frame per weekday is seeded server-side, so a JS-off browser posts exactly what the old form posted.
noJsSubmitOfTheSeededGridCreatesTheTypedRulessubmits all seven seeded rows over HTTP to prove it, rather than asserting on markup alone.Hardening that came along for the ride
Reviews surfaced three things worth naming:
persistFrames(ownerId, typeId, …)ended up next tocreateInitialDateOverride(typeId, ownerId, …)— bothLong, so a future swap would compile clean and write a cross-tenantownerId. Both helpers now read(ownerId, typeId, form).persistWindowsguard also covers time windows added from the date-override pages.meetingTypeIdonPOST /me/date-overridesreturned 500; it now returns 400. (Investigating that corrected a false premise: unparseable dates were already returning 400 via the globalMalformedDateTimeMapper— the bug reports claiming 500 for those were wrong, and the beans now say so.)One grid, not four
Follow-up review asked why the component was copied rather than reused — a fair hit. The grid markup existed four times over (
availability.html,meetingTypeDetail.html,sharedAvailability.html, and this PR's create form), character-identical except for its i18n namespace and card background class, and the same four button strings existed in three key families.It is now one
AdminResource/_workplanGrid.html, included by all four pages and parameterised by the row list and the card class, with a singleadm_workplan_*key family replacing theadm_availability_*/adm_detail_*/adm_meetingTypes_*frame keys (13 keys removed across the bundle and both locale files).workplan.jswas not touched — if the markup contract had drifted, it would have needed to be.The create page normalises onto the same
List<WeekRow>the other three pass, viaWeekRow.blank(), so the fragment has no "is this the create page" branching. Net −102 lines.Testing
Full suite green: 905 tests, 0 failures, 0 errors,
mvn spotless:checkclean.New tests cover the rendered grid markers (including that each seeded row carries its own day), a blank frame skipped, two frames on one day, an inverted frame dropped rather than 500ing, the no-JS submit, and the malformed-input cases above.
Driven in a real browser against the dev server and Postgres:
+ Frame,Copy to all days,Copy to weekdays,×remove andRemove availabilityeach behave correctly and none of them submits the form; a create with a split Monday (09:00–12:00 + 13:00–17:00) round-tripped into the edit page through the same shared fragment.Notes
docs-sitebranch and are not part of this PR; their PR links are#TBDpending this number.83de3dc,4f3e052) are the fork point fromdocs/drop-flyway-version-range, included deliberately.calit-o9rp(the grid's time inputs have no accessible name),calit-x921(thedaysOfWeektemplate parameter is dead now that the fragment takesweek), andcalit-qec2's sibling endpoints.calit-spfuwas folded in — it is the shared-fragment work above.🤖 Generated with Claude Code
https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug