ROCK-8700: Fix double check-in caused by redundant Add after AddOrUpdate - #264
Merged
Conversation
Rock core's AttendanceService.AddOrUpdate returns the existing tracked attendance when one exists for the same person + occurrence. The extra attendanceService.Add() flipped that entity to Added, so EF inserted a duplicate row and dropped the update closing the old attendance, leaving two active attendances (double check-in). - SaveAttendance.cs: remove the redundant Add (AddOrUpdate already adds new records); existing records now update in place. - DataHelper.cs (RoomScanner): comment only. The identical-looking Add in CloneAttendance is intentional clone-by-readd and must not be removed.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a double check-in (duplicate active Attendance rows) caused by calling AttendanceService.Add() on an entity returned by AttendanceService.AddOrUpdate(), which can flip an existing tracked entity to Added and trigger an unintended INSERT rather than an in-place update.
Changes:
- Removed the redundant
attendanceService.Add( attendance )in the FamilyCheckin save workflow path so existing attendance rows update in place instead of being re-inserted. - Added an explanatory note in FamilyCheckin documenting why
Add()must not be called afterAddOrUpdate()for the same tracked entity. - Added an explanatory note in RoomScanner clarifying that the
Add()insideCloneAttendanceis intentionally used to force a clone-by-reinsert behavior in that specific flow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Plugins/org.secc.FamilyCheckin/Workflows/SaveAttendance.cs | Removes redundant Add() after AddOrUpdate() to prevent duplicate inserts / double check-in. |
| Plugins/org.secc.RoomScanner/Utilities/DataHelper.cs | Adds documentation explaining why Add() remains intentional in CloneAttendance. |
jrwake89
approved these changes
Jul 10, 2026
jrwake89
left a comment
jrwake89
left a comment
There was a problem hiding this comment.
Looks good to me, Stephen! I found the same bug when looking into this a bit.
stphnlee
added a commit
that referenced
this pull request
Jul 21, 2026
* ROCK-7057 Added workflow action to handle camp group updates
* ROCK-7057 Updated workflow action to allow dynamically setting the attribute key and value prior in the workflow
* ROCK-8322 Fixing issue with auto-input from iOS (#200)
* ROCK-8322 Fixing issue with auto-input from iOS
* ROCK-8322 Reduced timeout and add enter on submit for phone number
* ROCK-8322 Updated enter submission to format phone number
* ROCK-8322 Add aria announcement for screenreaders on auto submit
* ROCK-8193: Copy StartDate and CustomSchedule at PublishGroup creation, disable schedule fields when CustomSchedule is set
- Add StartDate (from Group.Schedule.EffectiveStartDate) and CustomSchedule (from Group.Schedule.FriendlyScheduleText) to the field copy-over when a new PublishGroup is created
- Disable Day of Week and Time of Day dropdowns on the form when CustomSchedule has a value, since ScheduleText prioritizes CustomSchedule
* ROCK-7057 Updated workflow action to handle diverse situations flexibly
* ROCK-8193: Populate MeetingLocation with full address when Location.Name is empty
Falls back to Location.ToString() (full street address) when the
location has no name set, fixing empty MeetingLocation on PublishGroups.
* ROCK-7057 Improved log message for finding additional groups on removal; added trim to groupnameprefix; updated logic for finding other matching placement groups to include groups attached to the template
* ROCK-8193: Schedule fields always pull from Group, update help text
- Schedule fields (Time of Day, Custom Schedule, Starts On) now always
pull from Group.Schedule and are disabled on the PublishGroup form
- Day of Week also pulls from Group when weekly, but stays editable
when the Group has a custom schedule (for filtering on group list page)
- CustomSchedule only populated at creation when WeeklyDayOfWeek is null
- Updated help text on Day of Week and Custom Schedule info icons
* ROCK-8318 Add logic to handle leader registrations where the groups are not linked directly to the registration instance
* ROCK-8318 Modified group and groupmember lookup logic for efficiency and trimmed workflow group attribute key
* ROCK-8356 Added suppress giving statement logic to Generate Statements
plugin
* ROCK-8328: Medication Manager Updates
Add bulk distribute, manage medications modal, and add medication sub-modal to the MedicationDispense block.
* ROCK-8397 Converted ServerHealth.ashx into a block to confirm that Rock is running before returning that the server is healthy
* Update Plugins/org.secc.SystemsMonitor/org_secc/SystemsMonitor/ServerHealth.ascx.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Update Plugins/org.secc.SystemsMonitor/org_secc/SystemsMonitor/ServerHealth.ascx.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Update Plugins/org.secc.SystemsMonitor/org_secc/SystemsMonitor/ServerHealth.ascx.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* ROCK-8362: Sanitize CustomSchedule to prevent HTML and iCal content in publish groups
- Add FormatScheduleDates() to convert RDATE schedules into readable date lists
(e.g., "Tuesdays at 6:30 PM: Aug 5, Aug 19, Sep 2...")
- Replace FriendlyScheduleText and iCalendarContent references with FormatScheduleDates
- Apply .SanitizeHtml() on save to strip any remaining HTML from CustomSchedule
- Add | StripHtml in GroupFinder Lava templates as defense-in-depth
- Fix NullReferenceException in OnInit when publishGroup is null
* ROCK-8362: Sanitize CustomSchedule to prevent HTML and iCal content in publish groups
- Add FormatScheduleDates() to convert RDATE schedules into readable date lists
(e.g., "Tuesdays at 6:30 PM: Aug 5, Aug 19, Sep 2...")
- Replace FriendlyScheduleText and iCalendarContent references with FormatScheduleDates
- Apply .SanitizeHtml() on save to strip any remaining HTML from CustomSchedule
- Add | StripHtml in GroupFinder Lava templates as defense-in-depth
- Fix NullReferenceException in OnInit when publishGroup is null
* ROCK-8367: Remove SCC staff reference blocking and employer section from volunteer application
Remove the validation that blocks volunteer applications when a reference is a
Southeast Christian Church staff member. The Staff checkbox is kept on the form
for data collection but no longer prevents submission. Also removes the Current
Employer section (CurrentEmployer, PositionHeld, WorkPhone) from the PDF merge
and removes WorkPhone validation from personal information.
* ROCK-8362: Truncate long date lists in FormatScheduleDates
Show first 3 dates + last date for schedules with 5+ specific dates
to prevent overly long Custom Schedule text (e.g., "Tuesdays at 6:30 PM: Aug 5, Aug 19, Sep 2, ... Dec 2")
* ROCK-8422 Optimized job to reduce webfarm messages
* Update Plugins/org.secc.FamilyCheckin/Jobs/ResetGroupLocationSchedules.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* ROCK-8373 Added null checks and better exception handling
* ROCK-8373 Added concurrent workers attribute and comma-separated giving IDs
* ROCK-8442 Improvements to Exporting Statements (#212)
* ROCK-8442 Improvements to Exporting Statements
* ROCK-8442 Removed ZIP
* Update Plugins/org.secc.Finance/org_secc/Finance/ContributionStatementList.ascx.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* ROCK-8404 Removed custom email from group manager
* Update Plugins/org.secc.GroupManager/org_secc/GroupManager/LWYARoster.ascx.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Make From Email Address a required block attribute
Agent-Logs-Url: https://github.com/secc/RockPlugins/sessions/922a0f5c-d114-4800-b5da-146765e1c987
Co-authored-by: stphnlee <902855+stphnlee@users.noreply.github.com>
* ROCK-8214 Initial commit on CSV import block
* ROCK-8428 Fixed false positive critical error
* ROCK-8407 Add jobs to disable/restore communications for inactive people
Adds two Quartz jobs that manage email and SMS preferences based on Person
record status:
- DisableCommunicationsForInactivePeople: snapshots original EmailPreference
and per-phone IsMessagingEnabled to a DefinedType, then sets EmailPreference
to DoNotEmail and disables SMS on all phones. Includes a configurable
lookback window and a Dry Run mode.
- RestoreCommunicationsForReactivatedPeople: for anyone now Active with a
tracking entry, restores the snapshot and deletes the tracking entry. No
lookback so nobody gets stranded if a run is missed.
Snapshots are stored as DefinedValues (Value=PersonId, Description=JSON) in
a new "Inactive Person Communication Overrides" DefinedType, which is
created operationally as part of the initial rollout.
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* ROCK-8407 Track skipped count for benign continue branches
* ROCK-8214 Fixed bug in row processing
* ROCK-8214 Enhanced results page to show per row which updates were successful
* ROCK-8214 Saved column mapping to persist through postback
* Wrap per-iteration RockContext in using block
Addresses PR review feedback: ensures each per-person RockContext is
disposed immediately at the end of its loop iteration instead of
waiting on GC.
* ROCK-8214 Improved handling for duplicate names and improved query for faster performance
* ROCK-8214 Added base parent group to make mapping multiple columns easier
* ROCK-8214 Improved handling for duplicate records
* ROCK-8469 Added background processing
* ROCK-8469 Added efficiency and safety improvements
* ROCK-8469 Added efficiency and safety improvements
* ROCK-8469 Added efficiency and safety improvements
* ROCK-8488 Fall back to minor's mobile phone when parent phone is blank
More minors are providing their own mobile number rather than a parent's,
which left the Decision Analytics grid phone column blank. When IsMinor is
true and ParentPhone is null/whitespace, fall back to the minor's MobilePhone.
* ROCK-8469 Added efficiency and safety improvements
* ROCK-8469 Added efficiency and safety improvements
* ROCK-8488 Fall back to minor's mobile phone when parent phone is blank (#218)
More minors are providing their own mobile number rather than a parent's,
which left the Decision Analytics grid phone column blank. When IsMinor is
true and ParentPhone is null/whitespace, fall back to the minor's MobilePhone.
* ROCK-8469 Implementing comments from PR
* ROCK-8469 Implementing comments from PR
* ROCK-8469 Implementing comments from PR
* ROCK-8469 Implementing comments from PR
* ROCK-8469 Implementing comments from PR
* ROCK-8469 Implementing comments from PR
* ROCK-8488 Add profile-link icon column to Decision Analytics grid
Modal access alone is not discoverable for most users. Add a leftmost
icon column linking to /Person/{PersonId} so staff can navigate from
any row to the candidate's profile in one click. ExcludeFromExport
keeps Excel exports clean.
* ROCK-8488 Fix Parent Email modal binding for minors
The minor branch of LoadDetailModal was binding the Parent Email
literal to decision.Email (the candidate's own email) instead of
decision.ParentEmail. Modal now shows the parent's email under the
Parent Email label.
* ROCK-8488 Use ExcelExportBehavior instead of ExcludeFromExport
ExcludeFromExport is not the correct attribute name in Rock 13.7.
The supported attribute on RockTemplateField is ExcelExportBehavior
with values AlwaysInclude or NeverInclude. Using NeverInclude to
keep the profile-link icon column out of Excel exports.
* ROCK-8488 Stop profile-link click from bubbling to row-select handler
Without stopPropagation, clicking the profile icon both navigates to
/Person/{id} and fires gResults_RowSelected, which opens the row's
detail modal. Browser navigation usually wins the race, but slow
postbacks can flash the modal before navigation completes.
* ROCK-8507 Updating server health block to only monitor for maintenance flag
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* ROCK-8507 Removed vistigial panel elements
* ROCK-8501 Add MedicationActive flag and source-aware medication manager
Adds an Active/Inactive toggle for medications when the camp page reads
the Person master matrix (e.g. HSM Fall Retreat, MSM Discipleship Weekend,
RUCKUS), and restores full Edit/Delete/Add Medication on the snapshot
matrix for camps whose registration workflow copies medications to a
GroupMember-level matrix.
The MedicationActive boolean attribute is added to the medication matrix
template; all consumers filter inactive items consistently — the dispense
grid, the leader notification job, the family-facing medication summary
block, the check-in kiosk label printer, and the camp medication Excel
export stored procedure (via new migration 006).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ROCK-8499 Fix sermon share link to point to YouTube (#225)
* ROCK-8431 Restore missing connection workflow actions (#223)
* ROCK-8431 Restored SetConnectionRequestGroup and SetConnectionAttributeValue workflow actions
Recovered from secc/Rock@26f714c979 (originally authored by Mark Lee, Mar 2018).
The classes were lost during the Rock 13 migration to hotfix-1.13.7 in 2022,
leaving EntityType rows 1616/1617 with empty AssemblyName fields and ~170
workflow references silently failing for the past ~3.5 years.
Namespace stays Rock.Workflow.Action so the existing EntityType.Name records
match. Rock auto-populates AssemblyName via MEF discovery on startup, so no
plugin migration is required.
* ROCK-8431 Use non-obsolete GetWorkflowAttributeValue spelling
The recovered code called the [Obsolete] GetWorklowAttributeValue (typo,
deprecated since Rock 1.11). Swapped to the correctly-spelled
GetWorkflowAttributeValue. Behavior is identical -- the obsolete method
is a 1:1 shim that internally calls the correct one -- but this removes
the CS0618 build warning and survives eventual removal of the shim.
* Typo
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* ROCK-8431 Address code review: cleanup + implement Group Status
Resolves the 8 Copilot review comments on PR #223:
- Fixed "shoud" -> "should" copyright header typo in both files
- Removed unused usings (System.Linq, Rock.Security, Rock.Web.Cache)
- Removed redundant `?? Guid.NewGuid()` on already-null-checked guids
- Hoisted GetMergeFields(action) out of the per-entry loop in
SetConnectionAttributeValue (computed once, reused)
- Implemented the previously-dead "Group Status" dropdown: it now sets
ConnectionRequest.AssignedGroupMemberStatus. Also corrected the
dropdown list source from 3^Pending to 2^Pending so values map
directly to the GroupMemberStatus enum (Inactive=0, Active=1,
Pending=2). The field was never read before, so no existing
workflow's stored value was meaningful.
Group Status is a deliberate behavior change (chosen over removing the
field) to make the action sustainable rather than carry a misleading
no-op config knob.
---------
Co-authored-by: Stephen Lee <slee@secc.org>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Rock 8398/multi file field type (#228)
* Changing workflow csproj file to use robocopy
* ROCK-8398 Adding multi file field type & upload functionality
* Typo fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Typo fix for pull request finding #2
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Robocopy fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Replace robocopy with xcopy in post-build events (#231)
Reverting the use of robocopy to fix build pipeline
* ROCK-8575: Revert Medication Manager to Person-only reads
Roll back the GroupMember snapshot direction in the Medication Dispense
grid. Nothing populates GroupMember.Medications in prod, which was
causing ~6,300 campers to render blank in the grid. Every other
medication consumer (camp report stored proc, kiosk labels, MyEvents
family info block, leader notifications) already reads Person.Medications.
- Strip the qrySnapshot LINQ branch and groupMemberMatrixAttributeIds.
Grid reads exclusively from Person.Medications.
- Manage Medications modal opens against the Person master; Edit and Add
write to the master, Toggle Active flips MedicationActive on the master.
- Remove Delete entirely (column, handler, defense guards). Staff use the
Active toggle to deactivate instead.
- Drop dead source-aware plumbing: SourcePerson/SourceSnapshot constants,
IsSnapshotMatrix method, hfManageMedsSource hidden field, badge literal,
source parameter on the configure helper.
UX add: inactive-but-distributed-today rows stay visible, grayed out
with an "Inactive" badge and Dispense disabled, so same-day audit trail
is preserved. Filter is per-(person, medicine, schedule) so distributing
breakfast and then deactivating the med doesn't leak lunch and dinner
rows into the grid.
Perf: index today's distribution notes into a HashSet keyed by
(PersonId | MatrixItemId | ScheduleGuid) for O(1) lookup during the
inactive filter, avoiding a per-medicine linear scan on heavy-load days.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Output encoding for ChangeManager fields to address stored XSS vulnerability
* Encoding outputs of person info for CheckinMonitor blocks to address xss vulnerabilities
* Encoding outputs for CMS & Communications plugins to address xss vulnerabilities
* ROCK-8632 Added job to re-encrypt encrypted data (#234)
* ROCK-8632 Added job to re-encrypt encrypted data
* Added standard copyright blob
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* ROCK-8637 Fix IDOR / broken access control findings (rows 6-10)
Address the IDOR / broken-access-control batch from the security review:
- PublicProfileEdit (row 8): add server-side ownership gate before save;
reject tampered hfPersonId/ddlGroup that don't belong to CurrentPerson's
families.
- LinkListEditUsers (row 9): re-resolve the editors group from the content
channel item and re-run IsAuthorized(EDIT) on every mutating postback
instead of trusting the tamperable hfSecurityGroupId hidden field
(privilege-escalation fix).
- GroupDetailLava (row 10): add object-level authorization in OnLoad so an
authenticated user cannot swap GroupId to render an arbitrary group's
member PII; matches the sibling-block pattern.
Row 6 (GroupManagerBlock base) is covered by the row 10 block-level fix; row 7
(FinancialTransactions REST) is staff-only/401-gated and accepted as internal
risk per policy. Open-redirect/XSS findings are tracked separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8638 Require authentication on Imaging REST endpoints
Add [Authenticate, Secured] to the api/imaging/test and
api/imaging/generateimage actions. GenerateImage rendered
caller-supplied HTML into a PNG with no auth, exposing an
unauthenticated endpoint and resource-abuse/SSRF vector.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* ROCK-8638 Encode Purchasing outputs to address stored XSS
RequisitionDetail: HTML-encode the user-entered Title and
Status.Value before assigning to labels, preserving the appended
<small> markup.
PODetail: HTML-encode VendorName, street address, and city/state/zip.
Build the vendor web-address anchor from an encoded link text and an
attribute-encoded href restricted to http/https, with rel=noopener.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* ROCK-8638 Encode ChangeManager outputs to address stored XSS
Encode the user-controlled Name and requestor FullName in the header
block, and the requestor/approver comment literals.
In FormatValues, track whether a value column holds trusted Rock HTML
(attribute ValueFormatted or the photo thumbnail) and HTML-encode the
plain-text NewValue/OldValue and Property/Comment only when it does
not, leaving the trusted-HTML branches and the grid's HtmlEncode=false
columns intact.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* ROCK-8638 Adding HTML encoding to remaining RockLiterals in Purchasing plugin
* ROCK-8615: Plugin documentation — pilot READMEs (format review) (#233)
* ROCK-8615: Add pilot plugin README docs (QRManager, PastoralCare, Workflow)
Pilot of the per-plugin documentation format for ROCK-8615: one README per
plugin covering Overview, Project Info, Project Layout, Components,
Dependencies, Migrations, Observations (security/improvements), and Making
Changes. Three representative plugins (small/medium/large) for format review
before documenting the remaining plugins.
* ROCK-8615: Upgrade Workflow README to deep-technical tier (format sample)
Demonstrates the deeper documentation tier alongside the standard-tier
QRManager and PastoralCare pilots: action execution data-flow diagram,
ActionComponent contract/conventions, per-action config-attribute reference
for the most-used actions, edge cases, and an extending guide. Workflow is a
deep-tier plugin under the agreed tiered approach (deep where it pays;
standard for the rest).
* ROCK-8615: Document 8 more plugins (PersonMatch, Jobs, FamilyCheckin, Finance, Communication, Connection, Rest, OAuth)
Standard-tier READMEs for PersonMatch, Jobs, FamilyCheckin, Finance,
Communication, Connection; deep-tier READMEs (data-flow diagram, edge
cases, config-attribute reference, extending guide) for Rest and OAuth.
Follows the tiered format established in the pilot READMEs.
* ROCK-8615: Document remaining 44 plugins + add top-level Plugins/README.md index
Adds a README for every remaining org.secc.* plugin (42 standard tier,
2 newly deep: none added here beyond the 5 payment/security deep tier —
PayFlowPro, PayPalExpress, PayPalReporting, Authentication, Security) and
a top-level Plugins/README.md catalog grouping all 55 plugins by category
with tier + one-line description. Every README was generated from source
and adversarially verified against the code (Migrations and Purchasing
verified by hand after their automated verifiers timed out).
* ROCK-8657 Updated action to get person and group from the passed grou… (#237)
* ROCK-8657 Updated action to get person and group from the passed groupmember entity
* ROCK-8657 Log warning on attribute resolve failure before entity fallback
Address PR review on the placement-group sync action:
- Log a warning when a configured Person/Placement Group workflow attribute fails to resolve, before falling back to the triggering GroupMember entity, so genuine misconfigurations stay debuggable.
- Use AsNoTracking on the Person entity fallback to match the attribute-resolution path and avoid tracking the trigger graph.
* ROCK-8638 Fixing double encoding of ship to information
* ROCK-8637 Fix IDOR / broken access control + PIN logic-flaw findings
- ManageCommunicationLists: resolve the anonymous person via GetByImpersonationToken
instead of a guessable GUID-suffix match (Guid.EndsWith) — closes an anonymous
account-takeover IDOR (email/phone overwrite -> password-reset hijack).
- SuperCheckin: add the missing return so a security-role member can no longer have a
PIN UserLogin minted (privilege escalation via fall-through).
- RequisitionDetail: add object-level UserCanViewRequisition() gating LoadRequisition/
LoadSummary/LoadItems + refresh handlers, plus CreatedBy/CurrentPerson null-safety
— closes a horizontal IDOR that let any authenticated user view any requisition.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8637 Note in code that the ?p= path has no live generator
Document inline that the subscribe link is a bare /s (no token), verified in code and
the Rock DB, so switching to GetByImpersonationToken breaks no existing link.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8637 Tighten security-fix comments for brevity
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8637 Pull ManageCommunicationLists out of this PR
The GetByImpersonationToken change is NOT code-only: live SMS workflows generate the
guessable ?p= links it would reject — WorkflowType 250 (* Communication List Signup,
secc.org/s/{keyword}?p=, last sent today). Shipping it alone would break SMS sign-up
completion. Tracking as a coordinated code+config follow-up (block + re-tokenize the
WT 250 SMS link, deployed together). WorkflowType 361 (/connect/) is a sibling.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8637 ManageCommunicationLists: require impersonation token for anonymous resolution
Replace the guessable GUID-suffix match (Guid.EndsWith(param), length>=12) with
GetByImpersonationToken. The old code used the tail of a person's GUID as a bearer
credential; since a GUID is not secret (sent in plaintext SMS links, provider logs,
other Rock URLs), a leaked/forwarded suffix let an anonymous caller resolve that person
and overwrite their email/phone (account takeover via password reset).
COORDINATED with a config change (deploy together): WorkflowType 250
'* Communication List Signup' action 5380 builds secc.org/s/{keyword}?p={Guid|Slice:24,12}
and must instead emit a token via PersonTokenCreate. WorkflowType 361 '/connect/' is a sibling.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8650 Sync OnePoint search icon into repo
Mirror the live-on-prod SECC2019Portal header change that swaps the
plain-text "Search" trigger for a Font Awesome magnifying-glass icon
styled to match the news bell. Already applied and tested on prod via
Rock File Manager; this commit only brings the repo back in sync.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8654 Cap event promo image height on event detail page
The event detail page (SECC2024 theme) renders the promo image as a
full-width <img> with no height constraint, so tall/landscape photos
fill the viewport and push event copy below the fold on desktop.
Add a max-height: 45dvh + overflow: hidden cap on
.event-photo-container .event-photo. Scoped to the bare selector the
live DOM actually uses (no .event-details wrapper exists on the detail
page, so the pre-existing .event-details-scoped rule never matched).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* ROCK-8654 Scope event photo cap to the event-detail block; drop dvh
Scope the cap to .event-photo-cap (the CSS Class on the page-specific
Calendar Event Item Lava block 7325) so it only affects the event detail
page, not the shared .event-photo-container markup on the retired group
detail template. Use a single max-height: 45vh (dropped the dvh variant).
Requires the event-photo-cap CSS Class set on the block in each environment.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8654 Shorten comment
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8672: Remove vulnerable FFmpeg workflow action and xFFmpeg.NET dependency (#244)
The FFmpeg workflow action resolved the admin-configured Command attribute
through Lava (with {{file}} / {{outputPath}} merge fields) and passed the
result straight to the FFmpeg engine (argument/command injection), and called
Directory.CreateDirectory on an unsanitized OutputPath (path traversal).
Per the approved plan (JIRA ROCK-8672 #29957), this removes the action
outright rather than hardening it: only one inactive workflow referenced it
and no other code depends on it.
- Delete Plugins/org.secc.Workflow/Media/FFmpeg.cs
- org.secc.Workflow.csproj: remove the FFmpeg.NET reference, the
Media\FFmpeg.cs compile include, and the FFmpeg.NET.dll post-build xcopy
- packages.config: remove xFFmpeg.NET 3.4.0
- README.md: remove the FFmpeg action docs and the "Security (review)" note
that described this (now-fixed) vulnerability as a live finding
No database changes: the orphaned EntityType / WorkflowActionType rows are
left in place as inert metadata (the type can no longer be resolved, so it
cannot execute).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8675: send Messaging API function key via x-functions-key header (#241)
* ROCK-8675: send Messaging API function key via x-functions-key header
Move the decrypted Messaging API function key out of the ?code= query
string and into the x-functions-key request header across all
MessagingClient methods, so the secret no longer travels in request URLs
(captured in Function access logs, App Insights, and proxies). Behavior
is otherwise unchanged. Also fixes an Accept-header typo in
GetPhoneNumber (applicaiton/json -> application/json).
* ROCK-8675: update README security note to reflect x-functions-key header migration
---------
Co-authored-by: Stephen Lee <slee@secc.org>
* ROCK-8654 Vertically center the capped event photo
Add display:flex + justify-content/align-items center so the 45vh-capped
event promo image is cropped from its center (trimming top and bottom
equally) instead of clipping only the bottom.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8637 Clarify incrementUsage:false comment (address Copilot review)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Remove unused org.secc.GroupTrackerDemo plugin
Dead code: three anonymous api/GroupTracker/* REST endpoints (IDOR write + PII read)
with no post-build deploy step, not in RockWeb/bin, and no source/content/registration
references. The live 'Group Tracker' tool is a separate block (GroupMemberTracker.ascx).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8665: Fix note duplicating in PersonalGroupInformationPanel view (#240)
* ROCK-8665: Fix note duplicating in PersonalGroupInformationPanel view
Move the group member Note rendering out of the per-attribute foreach in
ShowView() so it renders once per member instead of once per attribute.
* Encode HTML in group member notes
* ROCK-8673: add in-memory rolling-window OTP request rate limiter (#242)
* ROCK-8673: add in-memory rolling-window OTP request rate limiter
Enforce a per-person/number cap on OTP code requests (default 5 per 15 min,
both configurable) before code regeneration, so regenerating a code no longer
resets the 5-guess brute-force budget. Implemented in-memory via the existing
SMSRecords helper; existing send-flood limiter and per-code guess budget unchanged.
* Removing configured 0's edge case for limits
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Fixed indentation mistake
* Reduce max code requests from 5 to 3
Also added explanatory comment on per-server max request cap
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* ROCK-8625: Add optional per-instance Lava Waiver Text to Volunteer Signup Form
* ROCK-8676 Updated instructions for documentation on Copilot review
* ROCK-8647: render MultiFile attribute media inline (native video/image)
Replace the plain-link display of the MultiFileFieldType with server-side
markup that embeds <video>/<img> for media files, keeping the download
link directly beneath each. Type detection uses BinaryFile.MimeType with a
filename-extension fallback. The condensed branch (grids/summaries) still
returns the 'N files' count, and FormatValueAsText still degrades to
filenames via the retained anchors. This removes the need for the
downstream page-level <script> that previously enhanced the links.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* ROCK-8647: address Copilot review feedback on PR #249
- Order display blocks via a Guid-keyed dictionary iterated over fileGuids
(matrix order) instead of OrderBy(IndexOf(...)); O(n), one materialization.
- Add aria-label, inner fallback text, and playsinline to the inline <video>
for accessibility and unsupported-browser handling.
- Add loading="lazy" decoding="async" to inline <img> thumbnails.
- Document the new inline-media display behavior in the org.secc.Attributes
README and add a 'Last updated' line per the repo documentation guideline.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* ROCK-8681 Reconcile Group App attendance confirm with kiosk check-in
PostGroupAttendance matched existing attendance by Occurrence.LocationId,
resolving it from group.GroupLocations.FirstOrDefault() when the caller sent no
locationId. Student small groups have no GroupLocation, so that resolved to
null and the lookup searched for a null-location occurrence -- never matching
the kiosk's room-specific occurrence. The confirm then created a duplicate row
on a separate null-location occurrence instead of flipping the check-in row.
Match the existing attendance on (GroupId, PersonAliasId, OccurrenceDate),
ignoring location (keeping the optional scheduleId filter), and prefer a
kiosk-origin occurrence (AttendanceCodeId set) when more than one matches. Only
create a new DidAttend=true row when none exists. Also return NotFound instead
of a 500 when groupMemberId is invalid, and read groupMember.PersonId directly
to avoid a null Person navigation in the query.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8647: address review — move media to FormatValueAsHtml; don't embed SVG
Per review: keep FormatValue link-only so exports/Lava merge fields don't
leak markup, and render <video>/<img> in FormatValueAsHtml (matches Rock's
MediaElementFieldType convention; the 6-arg entity overload delegates to
the 4-arg one, covering the ChangeManager block). Exclude SVG from inline
rendering (guarding both .svg and image/svg+xml) to avoid inline-script
XSS via GetFile.ashx.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* ROCK-8647: embed multi-file media in FormatValue (+AsHtml); keep exports clean via text path
Render <video>/<img> from both FormatValue and FormatValueAsHtml, mirroring
Rock core (ImageFieldType embeds via FormatValue; MediaElement/MediaWatch
override both). This restores inline media on Workflow Entry forms and Lava
output, which resolve attribute display through FormatValue rather than
FormatValueAsHtml.
The plan called for a FormatValueAsText override to keep exports/plain-text
merge markup-free. That override point does not exist in this Rock v13 base
(verified against the bundled Rock.dll: FormatValueAsText is absent;
GetTextValue/GetCondensedTextValue are its value-agnostic successors). So the
clean text channel is implemented via GetTextValue (comma-separated filename
list) and GetCondensedTextValue ("N files") instead. Condensed FormatValue
also returns "N files". SVG remains link-only (inline-script XSS guard).
Verified: full org.secc.Attributes assembly compiles clean (Roslyn,
langversion latest) against the in-repo reference assemblies.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* ROCK-8696: Remove Magick.NET/ImageMagick from org.secc.Workflow
The sole consumer was the ImageMontage workflow action, whose only referring workflow (#514 "Generate Trick Play Images") is dormant/inactive (last run 2024-03-10). Removes the vulnerable dependency and its dead code.
- Delete Media/ImageMontage.cs (only "using ImageMagick;" consumer in the repo)
- Remove both Magick.NET <Reference> blocks, the <Compile> entry, and the Magick* xcopy line from org.secc.Workflow.csproj (other PostBuildEvent xcopy lines kept)
- Remove both Magick entries from packages.config
- Remove Magick/ImageMontage references from README.md
No DB change; the orphaned EntityType + workflow #514 action-type rows are intentionally left in place.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8752: Filter inactive Defined Values out of VolunteerSignupWizard
VolunteerSignupWizard enumerated a configured Defined Type's DefinedValues
with no IsActive filter, so retired values still rendered (e.g. the Food Pack
wizard's 3 retired Oct-2023 slots in DefinedType 339 "2026 | Food Pack Times").
Added an IsActive filter at all four value-surfacing sites: the GetTree
fallback and saved-sub-values branches (public signup), the admin config
checkboxes, and the admin totals grid + column filter. Saved sub-values are
resolved via DefinedValueCache.Get with a null-guard so a deleted/retired GUID
in stored config is silently ignored. The DefinedType picker is intentionally
untouched (no inactive Defined Types exist site-wide).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8696: fix stale image-montage example in README flowchart
The workflow-engine mermaid diagram still cited 'create image montage' as an example action after the ImageMontage action was removed. Swap to a surviving example (remove a binary file).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8710: Move parent fields below waiver, relabel submit button
Change 1 - Volunteer Signup Form (Connections) block: render the editable
Form-key attribute controls (parent info) below the Waiver Text instead of
inline with the per-role display controls. Adds a phFormAttributes
PlaceHolder outside the repeater (after lWaiverText, before .actions) and
retargets both the write path (AddEditControls) and the read path
(GetEditValues) to it. Display-only non-URL attributes still render into the
per-role phAttributes.
Change 2 - primary submit button now reads "I Agree & Connect". The label is
set at runtime (was from the ConnectButtonText block setting); hardcoded per
stakeholder direction so every instance shows the waiver-acknowledgment
wording. The secondary "Connect and Add Another" button is unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* ROCK-8710: Note in README that Connect Button Text no longer applies to primary button
The primary submit button label is hardcoded to "I Agree & Connect" in code
(ROCK-8710), so the "Connect Button Text" block setting is a no-op for it.
Documented on that setting's README row so configurators aren't misled.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* ROCK-8710: Back out button hardcode + README note, keep layout move only
The primary submit label is already handled by the existing "Connect Button
Text" block setting, which is set to "I Agree & Connect" on the live food-pack
block instance (block 6192). Hardcoding btnConnect.Text overrode that working
per-instance setting for every copy of the block, so it is reverted here.
Restores to origin/master:
- .ascx.cs btnConnect.Text = GetAttributeValue( "ConnectButtonText" )
- .ascx primary button Text="Connect"
- README "Connect Button Text" row (removed the no-op note)
Remaining change vs master is layout-only: the phFormAttributes placeholder
(renders Form-key edit controls below the Waiver Text) plus the retargeted
AddEditControls/GetEditValues calls.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* GroupApp: enforce minor communication policy in Communicate endpoint
- Add IsMinor to GroupAppGroupMember DTO (set in GetGroupMembers)
- Individual emails to minors: CC parent/guardian emails via
Communication.CCEmails; reject with 400 if none on record
- Policy: communications to minors must include another adult
* Address review: cross-group guard, leader-gate IsMinor, README
- Communicate: 404 when GroupMemberId doesn't belong to {groupId}
- IsMinor only returned to leaders, consistent with other PII fields
- Document Communicate contract change + minor policy in README
* Remove stray tool markup from org.secc.Attributes README
* ROCK-8702 Add leader import support to Camp Placement Import (#256)
* ROCK-8702 Add leader import support to Camp Placement Import
Adds a Campers/Leaders toggle to the column mapping step. Leader imports
place people using the group type role marked IsLeader instead of the
default role, update existing non-leader members to the leader role, and
error when a target group type has no leader role. Preview validates
leader roles before processing.
* ROCK-8471 improved leader upgrade path
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* docs: document camp leader import behavior
* fix camp placement import labels for leader mode
* ROCK-8702 Reactivated dead archived groupmember path
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* Sl bugfix camp import leaders (#261)
* ROCK-8702 Add leader import support to Camp Placement Import
Adds a Campers/Leaders toggle to the column mapping step. Leader imports
place people using the group type role marked IsLeader instead of the
default role, update existing non-leader members to the leader role, and
error when a target group type has no leader role. Preview validates
leader roles before processing.
* ROCK-8471 improved leader upgrade path
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* docs: document camp leader import behavior
* fix camp placement import labels for leader mode
* ROCK-8702 Reactivated dead archived groupmember path
* ROCK-8702 Fixed bug in back button; added next button to file upload
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* ROCK-8756: Dev SMS capture transport (Papercut for SMS) (#258)
* ROCK-8756: add org.secc.SmsCapture project with CapturedSms entity and migration
New plugin project for the DEV-only SMS capture transport. Kept in its own
assembly (not org.secc.Communication) so the DLL never has to ship to prod.
CommunicationId/CommunicationRecipientId stored without FK constraints so
communication cleanup jobs are never blocked by capture rows.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* ROCK-8756: add SMS Capture transport component
Mirrors the v13 Twilio transport structure (both Send overloads, per-recipient
RockContext cadence, recipient status/History side effects) but writes a
capture row instead of calling the Twilio API. Recipients advance to Delivered
with a 'Captured by SMS Capture transport' status note. Sync-only on purpose:
MediumComponent falls back to the sync Send overloads for non-IAsyncTransport
components on every pipeline path. No Twilio SDK, no HttpClient, no outbound
calls. Enable Logging and Max Captured Messages component attributes; oldest
rows beyond the cap are trimmed after each send.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* ROCK-8756: add SMS Capture Inbox block
Papercut-style grid over the capture table: date range/to number/body filters,
detail modal with full pre-wrapped body and attachment links, per-row delete,
and a confirmed Clear All action.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* ROCK-8756: document SMS Capture wiring and post-clone checklist
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* ROCK-8756: reload component attributes per send so cap changes take effect
Component.GetAttributeValue reads the in-memory AttributeValues snapshot
loaded when the MEF singleton was constructed at app start. Editing Max
Captured Messages in the admin UI only updates the singleton on the node
that served the edit, so the node running the Send Communications job kept
reading the stale default (5000) and never trimmed. Reload attributes at
the start of both Send overloads (one cheap lookup per send call), and log
the trim result so the cap is observable in the Rock log.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* ROCK-8756: guard against null recipient on requery (Copilot review)
If the recipient row vanished between GetNextPending and the per-recipient
requery, the NRE in the try would be followed by a second NRE in the catch
(recipient.Status assignment), aborting the send loop and stranding the
remaining recipients as Pending. Return early instead. Pattern inherited
from core Twilio transport, which has the same latent issue.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* ROCK-8756: trim cap enforcement in SQL instead of EF (Copilot review)
Materializing excess rows pulled every over-cap nvarchar(max) body into
memory and deleted row-by-row; a ROW_NUMBER CTE delete does it in one
statement with no materialization. ExecuteSqlCommand's affected-row count
feeds the existing trim log line. Verified against LocalDB: 150 rows with
cap 100 deletes the 50 oldest.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* ROCK-8756: make inbox date filter end date inclusive
DateRangePicker is date-only and DateRange.FromDelimitedValues parses the
end date to midnight, so filtering CreatedDateTime < End excluded the
entire last selected day — a range ending today hid everything captured
today, which is the tool's primary workflow. Add a day and keep the
exclusive comparison.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Minor check: use AgeClassification == Child to catch no-DOB minors
Age.HasValue dropped child-role members without a birthdate; Rock
classifies them Child regardless. Businesses stay Unknown (classifier
skips them), so == Child never misfires on them. Applies to both the
Communicate guard and the roster IsMinor/parent-info logic.
* ROCK-8696: address review — README drift, one-time stale-DLL cleanup
- README Overview: "~29" -> "~27" actions, drop "media processing" (27
ActionComponent exports remain; the sole surviving Media action,
BinaryFileRemove, deletes files rather than processing media).
- README: add a "Last updated" marker per the org.secc.Attributes
convention.
- csproj PostBuildEvent: sweep stale Magick.NET/FFmpeg.NET DLLs out of
RockWeb/bin. xcopy never deletes, so the vulnerable binaries this
ticket and ROCK-8672 removed from source stay loaded on any
environment that built the prior code.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* ROCK-8752: Address Copilot review — null guards + reuse filtered list
- LoadSettings: materialize the active-values list once and reuse it for
both the filter dropdown and the grid rows (single predicate, no drift);
guard an unresolved defined type by skipping the partition.
- SetUpDefinedTypeDynamicControls: return early when no defined type is
selected ("Select One" passes Guid.Empty and DefinedTypeCache.Get
returns null).
- GetTree fallback: treat an unresolved defined type as zero values
instead of throwing on a misconfigured partition.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8706: narrow Attendance by occurrence before the member/employee OR in LoadHosts
LoadHosts() built hostsQry by GroupJoin-ing Attendance to the guest-count
subquery first, then applying the occurrence/DidAttend/EndDateTime/
member-or-employee/note Where clauses against the joined projection. EF6
generated a plan that ran the member-ConnectionStatus-OR-employee-subquery
predicate over the full ~9.4M-row Attendance table before narrowing by
occurrence, producing the reported 10-15s hang.
Move those five Where clauses onto attendanceService.Queryable() directly,
before the GroupJoin, so occurrence narrows the row set first. The GroupJoin/
DefaultIfEmpty guest-count join is unchanged (a correlated subquery there
re-runs the ValueAsPersonId scalar UDF per row and was measured at 58s).
DEV-verified on 2026-01-05 sample data: old and new shapes return identical
row sets (458/458, GuestSum 1159=1159). New shape measured at 310ms;
reproducing the pre-fix full-table-scan shape measured 16,454ms.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* ROCK-8752: Remove unreachable null guard in LoadSettings
The guard added in 371bffe3 duplicated a pre-existing check at the top of
the DefinedType partition block (L469-472), which already breaks out of
the partition loop when the type doesn't resolve — so the new guard could
never execute, and its comment misdescribed the actual behavior. The
materialized definedValues list (Copilot's actual ask) is kept.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8696: revert PostBuildEvent stale-DLL cleanup — doesn't reach prod
The Rock13 Azure DevOps release pipeline deploys via IIS Web App Deploy
with RemoveAdditionalFilesFlag=false (additive-only sync), and the build
runs on a disposable Microsoft-hosted agent. A del in the csproj's
PostBuildEvent only ever cleaned that throwaway agent's checkout — it
had no path to the actual Dev/Datably/Production bin folders, which
keep whatever a prior release deployed regardless of what's in the new
build. Dropping it; the stale Magick.NET/FFmpeg.NET DLLs need a manual
delete on each environment (see JIRA for steps).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* ROCK-8710: Clear phFormAttributes before re-add + guard Form-key controls to first role (fixes Add Another duplication)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8706: add AsNoTracking() to LoadHosts attendance query
The host query in LoadHosts() projects into SportsAndFitnessHost for
display and never saves changes, matching its three sibling queries in
the same method (attributeValueQry, hostsGuests, employees) which all
use AsNoTracking(). Adding it here avoids change-tracker overhead on the
result set, in line with the ROCK-8706 performance goal. (Copilot review
note on PR #262.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8706: decouple guest counts + batch occurrence lookup in LoadHosts
Fix B (guest-count tail): the guest-count aggregation was folded into the
attendance query via GroupJoin, which EF6 rewrote as a per-attendance-row
correlated subquery over the ValueAsPersonId scalar UDF — tail-spiking to
28.7s in Query Store. Run it as its own query materialized into a
Dictionary<int,int> (~87ms on DEV) and stitch GuestCount onto each host
after the attendance query materializes.
Fix A (occurrence N+1): the active-occurrence lookup made one
AttendanceOccurrenceService.Get() round trip per active (group, location,
schedule) triple. That Get(DateTime,int?,int?,int?) overload is read-only
(Queryable().FirstOrDefault(), no create/save) and
IX_GroupId_LocationID_ScheduleID_Date is UNIQUE on
(GroupId, LocationId, ScheduleId, OccurrenceDate), so the loop collapses to
a single query (three IN-lists) intersected against the exact triple set in
memory — provably identical result, one round trip instead of N.
Behavior-preserving: same attendance filters, same ordering, same per-host
guest counts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8706: decouple the employee lookup in LoadHosts (Fix C)
The member/employee OR left "employees.Contains(PersonId)" as an IQueryable
subquery, which EF6 compiled into a correlated EXISTS re-evaluated per
attendance row. AttributeValue.Value is nvarchar(max) (not indexable), so
each evaluation seeked attribute 740 and residual-filtered the string over
~11k rows -> ~2M row reads, the single largest operator in the Query Store
plan (subtree cost 2.42 of 3.01). The scalar UDF in the guest-count subquery
also forced the whole plan single-threaded (TSQLUserDefinedFunctionsNot-
Parallelizable) and the optimizer timed out.
Materialize the ~676 employee person ids ONCE into a list so the OR becomes a
constant IN-list evaluated a single time. Combined with Fix B (guest-count
decoupling, which removes the UDF and unlocks parallelism) the remaining
attendance query is a clean occurrence seek -> person lookup.
Behavior-preserving: same employee id set (same AttributeId=740 + Value
predicate), null EntityIds filtered (a null could never match a non-null
PersonId), OR unchanged -> identical host rows.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8790: Age-gate specialty roles — evaluate Group Requirements at signup
In VolunteerSignupFormConnections.btnConnect_Click, after the assigned
group + role are known for each ConnectionRoleRequest and before
rockContext.SaveChanges(), evaluate the role's Group Requirements for the
person via Group.PersonMeetsGroupRequirements(). If any requirement is
NotMet, surface the requirement's negative label inline (lResponseMessage),
keep the signup panel visible, and return before SaveChanges() so no
ConnectionRequest or downstream membership is created for an ineligible
volunteer. The person prospect record already exists (committed earlier)
and is intentionally left in place.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8790: address verifier review — add-time filter, fail-closed, use entered DOB
Verifier fixes on the signup Group Requirement gate:
- Fix 1: only block on requirements Rock enforces at add-time
(GroupRequirement.MustMeetRequirementToAddMember && RequirementCheckType
!= Manual), so a Manual/background-check requirement no longer
blanket-blocks every new signup.
- Fix 2: rockContext.SaveChanges() before evaluation so a matched existing
person's form-entered birthday (not their stale on-file DOB) is what the
requirement evaluates.
- Fix 3: fail closed — block on MeetsGroupRequirement.NotMet OR Error, not
just NotMet.
- Fix 4: wrap PersonMeetsGroupRequirements in try/catch; on throw, log via
ExceptionLogService and block with a generic message (fail closed) instead
of surfacing an unhandled error page.
- Fix 5: render the block message as alert-danger (red) instead of
alert-warning.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8790: prefer on-file birthday for age check, never overwrite from form
Stakeholder-approved change to the signup birthday rule:
- A matched person's existing on-file birthday is now PRESERVED (never
overwritten from the signup form); the age check evaluates it as-is.
- Only fill BirthDate from the form (bpBirthdate) when the person has none
on file — brand-new person, or matched person whose BirthDate is null —
as a fallback so the age check has a value.
- Guard changed from 'SelectedDate != person.BirthDate' to
'!person.BirthDate.HasValue'; the pre-evaluation SaveChanges persists a
newly-filled fallback birthday so PersonMeetsGroupRequirements reads it.
Shared-block behavior change: applies to ALL signups through this block
(Meal Packers, etc.), not just specialty. Accepted tradeoff (stakeholder):
the gate trusts the on-file birthday even if it is wrong.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8790: make signup requirement gate opt-in per requirement type
This block is shared across ~30+ signup instances, so an unconditional
gate would wrongly hard-block opportunities that only intend to pass their
Group Requirements through onto the connection request.
- New [TextField] block setting 'Signup-Blocking Requirement Types'
(key SignupBlockingRequirementTypes, order 15), declared in the same
style as the existing UrlKeys/FormKeys settings.
- Parse the setting (split on comma, Trim, drop blanks). Empty => skip the
gate entirely, so every other instance is unaffected by default and its
requirements still pass through onto the connection request.
- Gate now blocks only when GroupRequirementType.Name is in the configured
allowlist (case-insensitive) AND RequirementCheckType != Manual AND
(NotMet OR Error). Dropped the MustMeetRequirementToAddMember filter — the
explicit name allowlist is the intent control now; kept the non-Manual
guard so a mistakenly-listed Manual type can't blanket-block new signups.
Specialty instance must set the value to 'Over 18 Years Old, Over 14 Years
Old' (config/recipe, not code). Name-match means renaming a requirement type
requires updating the setting — same tradeoff as UrlKeys/FormKeys.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8790: move block-gate message above submit buttons and personalize it
UX tweaks to the signup requirement gate (message location + wording only;
gate logic, birthday rule, setting, and commit-before-evaluate unchanged):
- .ascx: add <asp:Literal ID="lRequirementBlockMessage"> inside pnlSignup,
just above the actions div, so the error shows directly above the Connect
buttons instead of at the top of the block.
- Render the block message on lRequirementBlockMessage instead of
lResponseMessage (top literal stays reserved for the normal connect
response flow).
- Personalize: "[NickName] isn't eligible for the [Role] role — [reason]."
Name = person.NickName (fallback FirstName); role resolved via
GroupTypeRoleService (fallback "this role"); reason = requirement type's
Negative Label (fallback Name). Multiple failing requirements lead with
"... isn't eligible for the [Role] role:" then a <br/>-joined list.
- Fail-closed evaluation-error message stays generic and renders in the same
spot: "We couldn't verify [Name]'s eligibility for this role...".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8706: fix CS8059 — RockWeb runtime-compiles this .ascx.cs as C# 6
The Fix B guest-count stitch loop used an inline out-variable declaration
(`out int guestCount`), a C# 7 feature. RockWeb compiles plugin .ascx.cs
code-behind at runtime under C# 6, which rejects it (CS8059: "Feature 'out
variable declaration' is not available in C# 6"), so the block failed to
load. Declare guestCount before the loop and pass `out guestCount`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8706: hand occurrence IN-lists to EF as List, not HashSet
Fix A built activeGroupIds/activeLocationIds/activeScheduleIds as HashSet<int>
and used .Contains() inside the EF6 IQueryable. EF6 translates List<T>.Contains
to SQL IN but does NOT reliably translate HashSet<T>.Contains (throws
NotSupportedException at query execution — a runtime page-load failure, not a
compile error). Materialize the three to List<int> before the query (dedup is
still done by the HashSet build). Consistent with the List used for employeeIds
in Fix C. The activeTriples HashSet is unaffected — it's used after ToList()
(in-memory LINQ), not translated to SQL.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8790: HTML-encode name/role/label in the eligibility-block message
Copilot follow-up: the eligibility-block message builds an alert-danger HTML
string with data-derived values interpolated in. HTML-encode each so a name,
role name, or requirement label containing &, ', or < can't break the markup:
- registrant name (person.NickName / FirstName) — covers both the not-met
message and the fail-closed catch message,
- the resolved role name,
- each requirement label (NegativeLabel / Name) in the blocking-messages list.
Uses Rock's null-safe .EncodeHtml() string extension (in scope via using
Rock;, already used elsewhere in this file); no using System.Web added.
Literal markup in the format strings (div wrapper, <br />) is left un-encoded.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8790: revert form DOB on block, clear stale message at handler top, note nothing submitted
Fix A (DOB re-entry trap): a birthday filled from the form this request was
committed by the pre-loop SaveChanges, so on a blocked submit a typo'd birth
year stuck (!person.BirthDate.HasValue was false on resubmit). Track whether
we filled BirthDate from the form this request (new-person SetBirthDate path;
matched-person fallback fill) and, on any blocking exit (not-met and the
fail-closed catch), revert it to null so a corrected resubmit re-reads the
form value. Revert runs on a SEPARATE RockContext (RevertFormBirthDate) so it
commits only the birthday clear — never the ConnectionRequests added earlier
in a multi-role loop on the main context, keeping a blocked submit
all-or-nothing. An existing on-file birthday is never reverted.
Fix B: clear lRequirementBlockMessage (Text/Visible) at the very top of
btnConnect_Click so a stale block message can't linger via ViewState on
'Connect and Add Another'. Done at the handler top, not in the gate (the gate
is skipped entirely when the setting is blank).
Fix C: append 'No signup was submitted.' to the single-, multi-requirement,
and fail-closed messages so it's clear the whole submit was cancelled.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ROCK-8700: Fix double check-in caused by redundant Add after AddOrUpdate (#264)
Rock core's AttendanceService.AddOrUpdate returns the existing tracked
attendance when one exists for the same person + occurrence. The extra
attendanceService.Add() flipped that entity to Added, so EF inserted a
duplicate row and dropped the update closing the old attendance, leaving
two active attendances (double check-in).
- SaveAttendance.cs: remove the redundant Add (AddOrUpdate already adds
new records); existing records now update in place.
- DataHelper.cs (RoomScanner): comment only. The identical-looking Add in
CloneAttendance is intentional clone-by-readd and must not be removed.
* ROCK-8801 Fix Mobile Check In Bug (#266)
* ROCK-8801 Rework MCRValidForKiosk to validate against persisted attendance data
QuickCheckin crashed with a NullReferenceException in MCRValidForKiosk when a
mobile-reserved room was closed (its GroupLocationSchedule detached) between the
reservation and the family's arrival at the kiosk. The occurrence could no longer
be resolved from OccurrenceCache, so occurrence.GroupTypeId threw and took down the
whole kiosk page.
Validate the reservation directly from the persisted Attendance -> Occurrence ->
Group records instead of OccurrenceCache. This matches the completion path
(btnCompleteMCRActual_Click), which already works off the attendance records and
never touches the cache, so a closed room no longer blocks or crashes check-in and
the family completes their original reservation without losing their spot.
* ROCK-8801 Add missing System.Data.Entity using for AsNoTracking
* ROCK-8801 Address review feedback on MCRValidForKiosk
- Collapse validation to a single SQL-side Any() query and skip the
DB round-trip entirely when the kiosk has no group types configured
- Trim the rationale comment to the essentials
- Document mobile-reservation validation behavior and the room-closure
edge case in the FamilyCheckin README
* Fix README Last updated formatting to match repo convention
* Update formatting
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* ROCK-8856 Event Pass: group registrations for the logged-in viewer (#267)
* ROCK-8856 Event Pass: add Include Family Registrations option
When enabled, the block expands the pass to include registrants from
other registrations in the same registration instance made by members
of the registrar's family, deduped by person. Defaults to off so
existing event passes are unchanged. Supports camps where each child
is registered individually but families expect one swipeable pass.
* ROCK-8856 Event Pass: hide carousel arrows when only one pass
* ROCK-8856 Event Pass: key registration grouping to the logged-in viewer
Replaces the registrar-family expansion with a viewer-keyed model:
anonymous visitors see exactly what the Registration/Registrant guid
names, and a logged-in viewer also sees registrants from other
registrations in the same registration instance made by the viewer or
their family. This removes the access-widening where one registrant
guid could unlock other people's pass QR codes (person-level check-in
credentials), plus:
- waitlist filter applies before anchoring, so a waitlist-only link
stays "Pass Not Found" instead of rendering family passes
- registrants without a person alias are excluded (previously crashed
the page in the per-registrant loop / DataBind)
- null-registrar registrations no longer throw during anchor
materialization (Registration.PersonAliasId is nullable)
- a ?Registrant= deep link orders that registrant first, so the
carousel opens on the requested pass
- one pass per person via order-preserving DistinctBy
- per-registrant PersonSearchKey N+1 replaced with one batched query
with a deterministic lowest-Id winner
- block setting renamed to "Include Registrar's Registrations"
(IncludeRegistrarRegistrations); environments that deployed the
earlier branch build should delete the orphaned
IncludeFamilyRegistrations attribute and re-enable the new setting
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* ROCK-8856 Event Pass: collapse registrar expansion into one parameterized query
Review cleanup on PR #267:
- Single registrant query replaces the anchor/family/main round trips;
eligibility filters (person alias, waitlist) now exist in one place.
- Family membership and anchor checks compose as EXISTS/subqueries instead
of literal IN lists, so every request reuses one cached SQL plan.
- Anchor rows outran…
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
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.
Rock core's AttendanceService.AddOrUpdate returns the existing tracked attendance when one exists for the same person + occurrence. The extra attendanceService.Add() flipped that entity to Added, so EF inserted a duplicate row and dropped the update closing the old attendance, leaving two active attendances (double check-in).
NOTE: Do not merge in before 7/13