Skip to content

Fix sharing.test.ts for discriminated union schema#130

Merged
hotlong merged 2 commits intocopilot/release-new-version-please-workfrom
copilot/check-action-run-status
Jan 25, 2026
Merged

Fix sharing.test.ts for discriminated union schema#130
hotlong merged 2 commits intocopilot/release-new-version-please-workfrom
copilot/check-action-run-status

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jan 25, 2026

Tests in sharing.test.ts were failing due to schema changes that introduced a discriminated union between CriteriaSharingRuleSchema and OwnerSharingRuleSchema.

Changes

Updated test data structure:

  • sharedWith: string → { type: ShareRecipientType, value: string }
  • criteria field renamed to condition (criteria-based rules)
  • Added ownedBy field (owner-based rules)
  • Removed 'manual' and 'guest' as valid rule types
  • Added 'full' as valid access level

Before:

const rule = {
  name: 'sales_access',
  object: 'opportunity',
  sharedWith: 'group_sales_team',
  criteria: 'status = "Open"',
};

After:

const rule = {
  name: 'sales_access',
  object: 'opportunity',
  type: 'criteria',
  condition: 'status = "Open"',
  sharedWith: { type: 'group', value: 'sales_team' },
};

All 24 sharing tests now pass.

Original prompt

引用: https://github.com/objectstack-ai/spec/actions/runs/21325211801/job/61381141562#step:8:1


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@vercel
Copy link
Copy Markdown

vercel bot commented Jan 25, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
spec Ready Ready Preview, Comment Jan 25, 2026 2:12am

Request Review

Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Copilot AI changed the title [WIP] Check action run status in GitHub CI Fix sharing.test.ts for discriminated union schema Jan 25, 2026
@hotlong hotlong marked this pull request as ready for review January 25, 2026 02:13
Copilot AI review requested due to automatic review settings January 25, 2026 02:13
Copilot AI requested a review from hotlong January 25, 2026 02:13
@hotlong hotlong merged commit f475c88 into copilot/release-new-version-please-work Jan 25, 2026
5 checks passed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the test suite in sharing.test.ts to align with schema changes that introduced a discriminated union between CriteriaSharingRuleSchema and OwnerSharingRuleSchema. The changes ensure all 24 tests pass with the new schema structure.

Changes:

  • Updated sharedWith from simple string to structured object with type and value fields
  • Renamed criteria field to condition for criteria-based rules
  • Added ownedBy field for owner-based rules
  • Removed 'manual' and 'guest' as valid rule types (now only 'owner' and 'criteria')
  • Added 'full' as a valid access level alongside 'read' and 'edit'

});

it('should accept manual sharing rule', () => {
it('should accept user-specific sharing rule', () => {
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name "should accept user-specific sharing rule" is misleading. This test is actually validating a criteria-based sharing rule that happens to share with a user recipient. The test name suggests it's testing a different rule type, but 'user-specific' is not a rule type in the schema - it's just using 'user' as a recipient type. Consider renaming to something like "should accept criteria rule with user recipient" to more accurately describe what's being tested.

Suggested change
it('should accept user-specific sharing rule', () => {
it('should accept criteria rule with user recipient', () => {

Copilot uses AI. Check for mistakes.
expect(rule.sharedWith.type).toBe('user');
});

it('should accept guest sharing rule', () => {
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name "should accept guest sharing rule" is misleading. After the schema changes, 'guest' is no longer a sharing rule type - it's now just a recipient type within the ShareRecipientType enum. This test is actually validating a criteria-based sharing rule that shares with guest recipients. Consider renaming to something like "should accept criteria rule with guest recipient" to accurately reflect the current schema structure.

Suggested change
it('should accept guest sharing rule', () => {
it('should accept criteria rule with guest recipient', () => {

Copilot uses AI. Check for mistakes.
Comment on lines 288 to 309
it('should reject sharing rule without required fields', () => {
expect(() => SharingRuleSchema.parse({
object: 'account',
sharedWith: 'group_id',
type: 'criteria',
condition: 'status = "Active"',
sharedWith: { type: 'group', value: 'group_id' },
})).toThrow();

expect(() => SharingRuleSchema.parse({
name: 'test_rule',
sharedWith: 'group_id',
type: 'criteria',
condition: 'status = "Active"',
sharedWith: { type: 'group', value: 'group_id' },
})).toThrow();

expect(() => SharingRuleSchema.parse({
name: 'test_rule',
object: 'account',
type: 'criteria',
condition: 'status = "Active"',
})).toThrow();
});
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test coverage for required fields should include validating that discriminated union requirements are enforced. Specifically, missing tests for:

  1. Owner-based rule without ownedBy field should be rejected
  2. Criteria-based rule without condition field should be rejected

These are critical validations for the discriminated union schema. Consider adding test cases like:

  • Testing that { name: 'test', object: 'account', type: 'owner', sharedWith: {...} } (missing ownedBy) throws an error
  • Testing that { name: 'test', object: 'account', type: 'criteria', sharedWith: {...} } (missing condition) throws an error

Copilot uses AI. Check for mistakes.
Comment on lines +196 to 220
it('should accept user-specific sharing rule', () => {
const rule = SharingRuleSchema.parse({
name: 'manual_share',
name: 'user_specific_share',
object: 'opportunity',
type: 'manual',
type: 'criteria',
condition: 'stage != "Closed Won"',
accessLevel: 'edit',
sharedWith: 'user_john_doe',
sharedWith: { type: 'user', value: 'john_doe' },
});

expect(rule.type).toBe('manual');
expect(rule.sharedWith.type).toBe('user');
});

it('should accept guest sharing rule', () => {
const rule = SharingRuleSchema.parse({
name: 'public_access',
object: 'knowledge_article',
type: 'guest',
type: 'criteria',
condition: 'published = true',
accessLevel: 'read',
sharedWith: 'guest_users',
sharedWith: { type: 'guest', value: 'guest_users' },
});

expect(rule.type).toBe('guest');
expect(rule.sharedWith.type).toBe('guest');
});
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests don't cover the 'role_and_subordinates' recipient type, which is defined in ShareRecipientType enum (sharing.zod.ts:41). Consider adding a test case to validate that sharing rules can use this recipient type, e.g., sharedWith: { type: 'role_and_subordinates', value: 'sales_manager' }

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants