Skip to content

Conversation

@thephez
Copy link
Collaborator

@thephez thephez commented Oct 30, 2025

Related to dashpay/platform#2842

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Corrected field naming convention for checkbox configurations to ensure proper default value handling.
  • Tests

    • Added end-to-end test coverage for data contract creation with history tracking enabled.

thephez and others added 2 commits October 30, 2025 12:06
Change form field property naming in API definitions to use 'default'
instead of 'defaultValue' for consistency with standard conventions.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

Walkthrough

Updated API schema field naming from defaultValue to default for two checkbox definitions, and added an end-to-end test case for data contract creation with history tracking enabled.

Changes

Cohort / File(s) Summary
API Schema Configuration
public/api-definitions.json
Updated field naming for Documents Mutable (Default) and Documents Can Be Deleted (Default) checkbox definitions, replacing defaultValue: true with default: true
Test Coverage
tests/e2e/transitions/state-transitions.spec.js
Added new E2E test "should create data contract with history enabled" that validates data contract creation with keepsHistory: true parameter override and verifies contractId in parsed result

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify consistency of field naming changes across the schema configuration
  • Confirm test case follows existing E2E test patterns and assertions are correct
  • Check that executeStateTransitionWithCustomParams method supports the new parameter usage

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "test: add test for contract create with history" directly and accurately describes the primary change in the changeset. The title clearly references the addition of a new end-to-end test for contract creation with history enabled, which is the main substantive change reflected in the test file addition. While the changeset also includes a supporting update to API definitions (changing field naming from defaultValue to default), this appears to be a configuration change that facilitates the main test addition rather than the primary objective. The title is concise, specific, and would allow a teammate scanning the repository history to quickly understand the change's purpose.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch test-contract-create

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
public/api-definitions.json (1)

1666-1671: Same field name change as the previous checkbox.

This change is consistent with the previous checkbox update.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 736c936 and 0dad29f.

📒 Files selected for processing (2)
  • public/api-definitions.json (1 hunks)
  • tests/e2e/transitions/state-transitions.spec.js (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/transitions/state-transitions.spec.js (1)
tests/e2e/queries/query-execution.spec.js (4)
  • result (39-39)
  • evoSdkPage (325-325)
  • parameterInjector (326-326)
  • contractData (172-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Run SDK Site UI Tests
🔇 Additional comments (1)
public/api-definitions.json (1)

1659-1664: No breaking change—frontend code already uses the default field.

The verification shows that public/app.js line 1248 explicitly reads from fieldSchema.default, not defaultValue. The field name change aligns with the frontend's actual consumption pattern and does not introduce any breaking changes.

Likely an incorrect or invalid review comment.

Comment on lines +504 to +525
test('should create data contract with history enabled', async () => {
// Execute the data contract create transition with keepsHistory: true
const result = await executeStateTransitionWithCustomParams(
evoSdkPage,
parameterInjector,
'dataContract',
'dataContractCreate',
'testnet',
{ keepsHistory: true } // Override to enable history
);

// Validate basic result structure
validateBasicStateTransitionResult(result);

// Validate data contract creation specific result
validateDataContractResult(result.result, false);

// Parse result and verify keepsHistory is set to true
const contractData = JSON.parse(result.result);
expect(contractData).toBeDefined();
expect(contractData.contractId).toBeDefined();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Add assertion to verify that keepsHistory is actually set in the created contract.

The test comment on Line 521 states "Parse result and verify keepsHistory is set to true", but the actual implementation only checks that contractId is defined. This means the test doesn't verify that the keepsHistory: true parameter was actually applied to the created contract.

Apply this diff to add the missing verification:

       // Parse result and verify keepsHistory is set to true
       const contractData = JSON.parse(result.result);
       expect(contractData).toBeDefined();
       expect(contractData.contractId).toBeDefined();
+      expect(contractData.keepsHistory).toBe(true);
🤖 Prompt for AI Agents
In tests/e2e/transitions/state-transitions.spec.js around lines 504 to 525, the
test claims to verify keepsHistory is set to true but only asserts contractId is
defined; update the test to parse the created contract object and add an
assertion that contractData.keepsHistory (or
contractData.definition?.keepsHistory depending on shape) strictly equals true
so the keepsHistory override is actually validated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants