TT-7681 Add bibleId ownership validation - #602
Open
gtryus wants to merge 2 commits into
Open
Conversation
…isting Bible ID handling
gtryus
changed the title
In [TeamDialog.tsx:471-475](vscode-webview://1vvl62agp8nmso4u3onasscp49436tio9t463rhlr0pga78gsjbi/src/renderer/src/components/Team/TeamDialog.tsx#L471-L475), `ownerName` collapses to `undefined` whenever `owner` is truthy (a different team) but that team's `organization` record isn't in the local cache — `organizations.find(...)` just returns `undefined`. My earlier fix used `!ownerName` as a stand-in for "not foreign," so exactly that case (foreign owner, but org record not yet synced) bypassed validation again. **Fix applied:** - [TeamDialog.tsx:471](vscode-webview://1vvl62agp8nmso4u3onasscp49436tio9t463rhlr0pga78gsjbi/src/renderer/src/components/Team/TeamDialog.tsx#L471): now passes a new `foreignOwner` boolean, `Boolean(owner && owner !== values?.team.id)` — derived only from the already-available `owner` org id (via `getBibleOwner`, which reads `organizationbible` records directly and doesn't need the `organization` record itself), independent of the display-name lookup. - [PublishExpansion.tsx:228](vscode-webview://1vvl62agp8nmso4u3onasscp49436tio9t463rhlr0pga78gsjbi/src/renderer/src/components/PublishExpansion.tsx#L228): `bibleIdIsValid` now gates on `!foreignOwner` instead of `!ownerName`; `ownerName` is used only for the alert text as recommended. - [PublishExpansion.tsx:236-242](vscode-webview://1vvl62agp8nmso4u3onasscp49436tio9t463rhlr0pga78gsjbi/src/renderer/src/components/PublishExpansion.tsx#L236-L242): added an effect that revalidates the already-typed id whenever `foreignOwner` changes, since `owner`/`foreignOwner` resolve one render after `bible` (TeamDialog computes it in a separate effect) — closing the "later arrival doesn't re-run validation" gap too. **Tests** ([PublishExpansion.test.tsx](vscode-webview://1vvl62agp8nmso4u3onasscp49436tio9t463rhlr0pga78gsjbi/src/renderer/src/components/PublishExpansion.test.tsx#L132-L216)): extended the TT-7681 describe block with a new case, `keeps the existing-bible error when the foreign owner name has not loaded`, rendering with `foreignOwner: true, ownerName: undefined`. Verified red against the `!ownerName` version (error incorrectly cleared to `''`) and green with the `foreignOwner`-based fix; the original TT-7681 test and `npm run typecheck` still pass.
gtryus
marked this pull request as draft
September 12, 2026 02:33
gtryus
marked this pull request as ready for review
September 12, 2026 02:45
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
An unused import may fail linting, and the delayed ownership transition lacks direct test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds robust Bible ID ownership validation to prevent saving IDs owned by another team.
Changes:
- Introduces an explicit foreign-owner flag and revalidation.
- Adds regression tests for missing or loaded owner names.
File summaries
| File | Description |
|---|---|
TeamDialog.tsx |
Supplies foreign ownership state. |
PublishExpansion.tsx |
Prevents foreign Bible IDs from passing validation. |
PublishExpansion.test.tsx |
Adds ownership regression coverage. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| import React from 'react'; | ||
| import { act, render, waitFor } from '@testing-library/react'; | ||
| import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; |
Comment on lines
+236
to
+240
| useEffect(() => { | ||
| // `foreignOwner` can resolve one render after `bible` (TeamDialog looks | ||
| // up the owning org in a separate effect). Re-check the id already | ||
| // typed so a stale '' error doesn't leave Save wrongly enabled. | ||
| if (bibleId) setBibleIdError(bibleIdIsValid(bibleId)); |
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.
Root cause (TT-7681): In
PublishExpansion.tsx,bibleIdIsValidshort-circuited to "no error" whenever the typed value matchedbible?.attributes?.bibleId— regardless of whether that loaded bible belonged to another team. When a user typed another team's Bible ID,TeamDialogloaded that foreign bible into the bible prop and flaggedownerName. Deleting and retyping the last character re-triggered validation while bible still referenced the foreign record, sonewName === bible.attributes.bibleIdwas true and the function returned '', clearing the error and re-enabling Save — even though the ID still belonged to another team.Fix:
PublishExpansion.tsx:210 now only treats "typed value equals loaded bible's id" as "unchanged" when!ownerName(i.e., we actually own that bible). Otherwise it falls through to the existing "already exists" check, reusing the existingbibleidexistsstring — no new localization strings were needed.Test: Added
PublishExpansion bibleIdownership validation (TT-7681) inPublishExpansion.test.tsx, following this repo's existing RTL pattern for this component. It renders with abible/ownerNameshape matching another team's Bible ID, mirrors the exact repro (delete last char, retype it), and asserts the error stays non-empty and the field stays aria-invalid.Verified red on the original code (error incorrectly went to '') and green after the fix, plus npm run typecheck and the existing
resolveBibleForSavetests still pass.