Preserve Workbench business rule payloads for clearer validation errors - #8048
Open
acwhite211 wants to merge 29 commits into
Open
Preserve Workbench business rule payloads for clearer validation errors#8048acwhite211 wants to merge 29 commits into
acwhite211 wants to merge 29 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR preserves structured business-rule exception payloads through WorkBench uploads and resolves them into localized frontend messages. It adds payload sanitization, typed upload-result contracts, resolver registries, conflicting-record formatting, and backend/frontend coverage. ChangesBusiness Rule Payload Localization
Sequence Diagram(s)sequenceDiagram
participant WorkbenchUpload
participant BackendUploadHandler
participant to_failed_business_rule
participant resolveValidationMessage
participant resultMessageResolvers
WorkbenchUpload->>BackendUploadHandler: upload record
BackendUploadHandler->>to_failed_business_rule: exception and ReportInfo
to_failed_business_rule-->>WorkbenchUpload: FailedBusinessRule with payload
WorkbenchUpload->>resolveValidationMessage: validation key and payload
resolveValidationMessage->>resultMessageResolvers: resolve parsing or business-rule message
resultMessageResolvers-->>WorkbenchUpload: localized validation message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 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 |
acwhite211
marked this pull request as ready for review
May 18, 2026 21:47
emenslin
requested changes
May 19, 2026
emenslin
left a comment
emenslin
left a comment
Collaborator
There was a problem hiding this comment.
- Confirm the message reads like a normal validation error, for example
Collectionobject must have unique catalognumber in collection. - Confirm the message includes conflicting record IDs when provided, for example
Conflicting record IDs: 3347460. - Confirm the tooltip no longer shows the raw Python tuple/dict payload.
Although this looks a lot better I do have one change I think is needed. I think that the error should show either the schema caption set by the user (e.g. CIDA Number instead of catalognumber) or the schema config field name (e.g. catalogNumber instead of catalognumber)
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
specifyweb/backend/workbench/upload/tests/test_upload_results_json.py (1)
41-70: ⚡ Quick winExtend this test to cover JSON round-trip of preserved payload.
This asserts conversion correctness, but not serialization/schema compatibility for the preserved payload. Add a small
UploadResult(...).to_json() -> json.dumps/json.loads -> from_jsonassertion in the same test.🤖 Prompt for 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. In `@specifyweb/backend/workbench/upload/tests/test_upload_results_json.py` around lines 41 - 70, Extend the testBusinessRuleExceptionPayload to also verify JSON round-trip: create the FailedBusinessRule via to_failed_business_rule(BusinessRuleException(...), info), wrap it in an UploadResult (use the same FailedBusinessRule instance), call UploadResult.to_json() then json.loads and pass that into UploadResult.from_json() (or the project equivalent) and assert the deserialized UploadResult still contains the original FailedBusinessRule payload (compare the preserved payload dict and message). Ensure you use the existing symbols: BusinessRuleException, ReportInfo, to_failed_business_rule, FailedBusinessRule, UploadResult.to_json and UploadResult.from_json for locating the code.
🤖 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 `@specifyweb/backend/workbench/upload/upload_result.py`:
- Around line 253-261: The helper is_business_rule_exception_with_payload
currently accepts any dict as the payload (exception.args[1]) which can contain
non-serializable or invalid values; update the logic in
is_business_rule_exception_with_payload (and the similar check around lines
264-268) to validate and/or sanitize the payload shape before treating it as a
business-rule payload: ensure exception.args[1] is a dict whose keys are strings
and whose values are only JSON-serializable scalar types (str, int, float, bool,
None) or plain lists/dicts that recursively satisfy the same constraint, or else
reject it (or replace with a safe fallback like an empty dict or
{"unserializable": true}) so that constructing/encoding FailedBusinessRule will
not fail at runtime. Include references to
is_business_rule_exception_with_payload and the code path that constructs
FailedBusinessRule when applying this validation.
In `@specifyweb/frontend/js_src/lib/components/WorkBench/resultsParser.ts`:
- Around line 305-336: In resolveBackendBusinessRuleMessage, guard against
missing/empty table by checking the result of getStringPayload(payload, 'table')
(assigned to tableName) and return undefined immediately if it's falsy; this
prevents calling getSchemaTableLabel('') and producing empty localized table
labels. Keep the existing branches that use tableName (fieldNotUnique,
childFieldNotUnique) but only execute them when tableName is non-empty so
callers can fall back to the default message.
- Around line 259-269: The message suffix is hardcoded in English inside
withConflictingRecordIds, causing mixed locales; update withConflictingRecordIds
to fetch a localized prefix from backEndText (e.g., add a conflictingRecordIds
key to the backEndText dictionary) and use that localized string (via
localized/backEndText lookup) instead of the hardcoded "Conflicting record IDs:"
before joining payload.conflicting; ensure IR payload handling and existing
localized(message) wrapping remain unchanged so the full tooltip is entirely
localized.
---
Nitpick comments:
In `@specifyweb/backend/workbench/upload/tests/test_upload_results_json.py`:
- Around line 41-70: Extend the testBusinessRuleExceptionPayload to also verify
JSON round-trip: create the FailedBusinessRule via
to_failed_business_rule(BusinessRuleException(...), info), wrap it in an
UploadResult (use the same FailedBusinessRule instance), call
UploadResult.to_json() then json.loads and pass that into
UploadResult.from_json() (or the project equivalent) and assert the deserialized
UploadResult still contains the original FailedBusinessRule payload (compare the
preserved payload dict and message). Ensure you use the existing symbols:
BusinessRuleException, ReportInfo, to_failed_business_rule, FailedBusinessRule,
UploadResult.to_json and UploadResult.from_json for locating the code.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f43e892-a787-4b63-a34a-af5fa0eda045
📒 Files selected for processing (5)
specifyweb/backend/workbench/upload/tests/test_upload_results_json.pyspecifyweb/backend/workbench/upload/treerecord.pyspecifyweb/backend/workbench/upload/upload_result.pyspecifyweb/backend/workbench/upload/upload_table.pyspecifyweb/frontend/js_src/lib/components/WorkBench/resultsParser.ts
emenslin
approved these changes
May 22, 2026
emenslin
left a comment
emenslin
left a comment
Collaborator
There was a problem hiding this comment.
- Confirm the message reads like a normal validation error, for example
Collectionobject must have unique catalognumber in collection. - Confirm the message includes conflicting record IDs when provided, for example
Conflicting record IDs: 3347460. - Confirm the tooltip no longer shows the raw Python tuple/dict payload.
Looks good, I confirmed with CO and a few other tables and all errors seemed to be formatted correctly.
bhumikaguptaa
approved these changes
May 22, 2026
bhumikaguptaa
left a comment
bhumikaguptaa
left a comment
Collaborator
There was a problem hiding this comment.
- Confirm the message reads like a normal validation error, for example
Collectionobject must have unique catalognumber in collection. - Confirm the message includes conflicting record IDs when provided, for example
Conflicting record IDs: 3347460. - Confirm the tooltip no longer shows the raw Python tuple/dict payload.
Works as expected, ran into no errors.
added 6 commits
July 27, 2026 14:00
added 2 commits
July 27, 2026 14:37
|
Warning One or more dependencies are approaching or past End-of-Life. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@specifyweb/backend/workbench/upload/upload_result.py`:
- Around line 285-317: Update _sanitize_business_rule_payload_value and
_sanitize_business_rule_payload so a valid top-level None is distinguished from
a failed sanitization. Use a dedicated sentinel or equivalent success indicator
for invalid values, preserve None in sanitized payloads, and continue skipping
unsupported values and non-string keys.
- Around line 252-282: Restrict the wrapper fallback in
is_business_rule_exception_with_payload to a canonical business-rule marker,
such as localizationKey, instead of accepting generic keys like table or
fieldName. Preserve the direct exception class identity check, but ensure
unrelated two-argument exceptions with database-style payloads are not
classified as business-rule exceptions.
In `@specifyweb/frontend/js_src/lib/components/WorkBench/resultsParser.ts`:
- Around line 37-42: Update the fallback around localized in resultsParser so
unknown business-rule payloads with payload.localizationKey return the readable
backend key without JSON.stringify(payload). Preserve the JSON payload fallback
for non-business-rule validation payloads, and keep the existing empty-payload
behavior unchanged.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 756a5fcb-1f93-456f-bacf-746e06e73177
📒 Files selected for processing (8)
specifyweb/backend/workbench/upload/tests/test_upload_results_json.pyspecifyweb/backend/workbench/upload/upload_result.pyspecifyweb/backend/workbench/upload/upload_table.pyspecifyweb/frontend/js_src/lib/components/WorkBench/__tests__/resultsParser.test.tsspecifyweb/frontend/js_src/lib/components/WorkBench/resultMessageResolvers.tsspecifyweb/frontend/js_src/lib/components/WorkBench/resultsParser.tsspecifyweb/frontend/js_src/lib/components/WorkBench/uploadResultTypes.tsspecifyweb/frontend/js_src/lib/localization/backEnd.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- specifyweb/backend/workbench/upload/upload_table.py
Contributor
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 50 minutes. |
CarolineDenis
force-pushed
the
issue-8045
branch
from
July 27, 2026 13:21
3a837da to
3de6ff5
Compare
Contributor
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@specifyweb/frontend/js_src/lib/components/WorkBench/resultMessageResolvers.ts`:
- Around line 146-177: Update fieldNotUnique and childFieldNotUnique to validate
the getStringPayload result for fieldName before resolving localized field
labels, falling back to the safe raw key when it is empty or missing. Apply the
same guard to parentField in childFieldNotUnique, while preserving the existing
table guard and normal localized-label behavior for non-empty values.
In `@specifyweb/frontend/js_src/lib/components/WorkBench/uploadResultTypes.ts`:
- Around line 169-189: Update the WbRecordResult type so it becomes a union of
single-variant objects: wrap each mapped recordResultType member in its
discriminator key, then index the mapped type by RecordResultTypes['type']. Keep
the existing Extract and Omit logic, ensuring record_result requires only the
one runtime variant rather than every possible discriminator key.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 94b978d5-866f-40a7-8838-b65088c496d8
📒 Files selected for processing (10)
specifyweb/backend/workbench/upload/tests/test_upload_results_json.pyspecifyweb/backend/workbench/upload/treerecord.pyspecifyweb/backend/workbench/upload/upload_result.pyspecifyweb/backend/workbench/upload/upload_table.pyspecifyweb/frontend/js_src/lib/components/LocalityUpdate/utils.tsspecifyweb/frontend/js_src/lib/components/WorkBench/__tests__/resultsParser.test.tsspecifyweb/frontend/js_src/lib/components/WorkBench/resultMessageResolvers.tsspecifyweb/frontend/js_src/lib/components/WorkBench/resultsParser.tsspecifyweb/frontend/js_src/lib/components/WorkBench/uploadResultTypes.tsspecifyweb/frontend/js_src/lib/localization/backEnd.ts
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
5 participants
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.
Fixes #8045
Improve Workbench handling of back-end business rule validation errors. The back-end now preserves structured
BusinessRuleExceptionpayloads inFailedBusinessRuleupload results instead of stringifying and discarding them. The Workbench front-end uses those payloads to show uniqueness-rule failures and appends the conflicting record IDs when available. This makes duplicate catalog number errors clearer and avoids exposing raw Python exception tuples in validation tooltips.Checklist
self-explanatory (or properly documented)
Testing instructions
Summary by CodeRabbit