Skip to content

Update form layouts after project policy reset#116

Open
unity-idm-agent wants to merge 2 commits into
unity-idm:devfrom
unity-idm-agent:fix/enquiry-policy-layout
Open

Update form layouts after project policy reset#116
unity-idm-agent wants to merge 2 commits into
unity-idm:devfrom
unity-idm-agent:fix/enquiry-policy-layout

Conversation

@unity-idm-agent
Copy link
Copy Markdown

@unity-idm-agent unity-idm-agent commented May 24, 2026

Summary

  • Update stored registration/enquiry form layouts when project policy documents are reset on a form.
  • Add a regression test for an enquiry form with a custom layout that initially lacks the policy-agreement element.

Root cause

  • Project policy reset updated the form's policyAgreements list but did not update stored custom form layouts.
  • If a custom enquiry layout was saved before/without the policy-agreement element, the policy stayed configured on the form but was not rendered to the user, so submitting the enquiry did not create sys:policy-agreement-state.

Fix

  • Call FormLayoutUtils.updateRegistrationFormLayout(...) and FormLayoutUtils.updateEnquiryLayout(...) after resetting policy agreements.

Tests

  • RED before fix: mvn -pl engine -Dtest=TestGroupDelegationConfigGenerator#shouldUpdateCustomEnquiryLayoutWhenProjectPoliciesAreReset test -Dgpg.skip=true -q failed because the custom layout contained only GROUP_0.
  • GREEN after fix: same targeted test passed.
  • Relevant tests: mvn -pl engine -Dtest=TestGroupDelegationConfigGenerator,TestEnquiryPolicyAgreements test -Dgpg.skip=true -q passed.
  • git diff --check passed.
  • CodeRabbit review against upstream/dev: no findings.

Manual verification

  • Source review confirmed generated project registration and join-enquiry forms include project policy agreements.
  • Source review confirmed policy rendering depends on the effective form layout containing POLICY_AGREEMENT_n elements.

Summary by CodeRabbit

  • Bug Fixes

    • Form layouts are now correctly refreshed when policy agreements are reset, ensuring proper structure in both registration and enquiry forms.
  • Tests

    • Added test coverage for enquiry form layout updates to verify that policy elements are properly included in form structures when policies are reset.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 24, 2026

Warning

Review limit reached

@unity-idm-agent, we couldn't start this review because you've used your available PR reviews for now.

Your plan currently allows 1 review/hour. Refill in 53 minutes and 26 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1a6cb36f-bc6a-4751-9922-aa08c44463ea

📥 Commits

Reviewing files that changed from the base of the PR and between 1c59365 and 81a5ffc.

📒 Files selected for processing (1)
  • engine/src/test/java/pl/edu/icm/unity/engine/project/TestGroupDelegationConfigGenerator.java
📝 Walkthrough

Walkthrough

The PR enhances form policy reset handling by integrating FormLayoutUtils to synchronize form layouts after clearing and rebuilding policy agreements. For both registration and enquiry form types, the implementation invokes the appropriate layout update utility before persisting forms, with a corresponding test validating the enquiry form layout update behavior.

Changes

Policy Reset with Form Layout Updates

Layer / File(s) Summary
Form layout update integration
engine/src/main/java/pl/edu/icm/unity/engine/utils/GroupDelegationConfigGeneratorImpl.java
Imports FormLayoutUtils and integrates calls to updateRegistrationFormLayout and updateEnquiryLayout within resetFormsPolicies for both form types, ensuring layouts reflect updated policy agreements before persistence.
Layout update validation test
engine/src/test/java/pl/edu/icm/unity/engine/project/TestGroupDelegationConfigGenerator.java
Adds a new test that verifies resetFormsPolicies correctly updates an enquiry form's policy agreements and layout elements, confirming both GROUP and POLICY_AGREEMENT elements are present in the final layout.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A rabbit hops through form layouts neat,
Where policies reset and designs compete!
Utilities sync the elements with care,
While tests verify the layout's there!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: updating form layouts after project policy reset, which aligns with the core modifications across both the implementation and test files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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
Copy Markdown

@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.

🧹 Nitpick comments (1)
engine/src/test/java/pl/edu/icm/unity/engine/project/TestGroupDelegationConfigGenerator.java (1)

266-272: ⚡ Quick win

Assert DB update invocation in the regression test.

At Line 266-272, the test only checks the mutated in-memory object. If enqFormDB.update(...) is removed, this can still pass. Add an explicit verify on the DAO update call.

Proposed test hardening
 import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.verify;
@@
 		generator.resetFormsPolicies("aenSuffix", FormType.ENQUIRY, List.of(2L));
+		verify(mockEnqFormDB).update(form);
 
 		assertThat(form.getPolicyAgreements()).hasSize(1);
🤖 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
`@engine/src/test/java/pl/edu/icm/unity/engine/project/TestGroupDelegationConfigGenerator.java`
around lines 266 - 272, The test in TestGroupDelegationConfigGenerator only
asserts in-memory changes after calling
generator.resetFormsPolicies(FormType.ENQUIRY, ...), so removal of the DAO
update would not fail the test; add an explicit Mockito verify that the DAO
update method was invoked (e.g., verify(enqFormDB).update(..) or
verify(enqFormDB).update(form)) after the assertions to ensure the persistence
call occurs; locate the call site around generator.resetFormsPolicies and add
the verify(enqFormDB) statement referencing the actual form variable used in the
test.
🤖 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.

Nitpick comments:
In
`@engine/src/test/java/pl/edu/icm/unity/engine/project/TestGroupDelegationConfigGenerator.java`:
- Around line 266-272: The test in TestGroupDelegationConfigGenerator only
asserts in-memory changes after calling
generator.resetFormsPolicies(FormType.ENQUIRY, ...), so removal of the DAO
update would not fail the test; add an explicit Mockito verify that the DAO
update method was invoked (e.g., verify(enqFormDB).update(..) or
verify(enqFormDB).update(form)) after the assertions to ensure the persistence
call occurs; locate the call site around generator.resetFormsPolicies and add
the verify(enqFormDB) statement referencing the actual form variable used in the
test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dcd0c6ed-2b62-487a-9973-31ea6d58f4cb

📥 Commits

Reviewing files that changed from the base of the PR and between efa25dd and 1c59365.

📒 Files selected for processing (2)
  • engine/src/main/java/pl/edu/icm/unity/engine/utils/GroupDelegationConfigGeneratorImpl.java
  • engine/src/test/java/pl/edu/icm/unity/engine/project/TestGroupDelegationConfigGenerator.java

@unity-idm-agent
Copy link
Copy Markdown
Author

unity-idm-agent commented May 24, 2026

Addressed the CodeRabbit test-hardening comment.

Change:

  • added verify(mockEnqFormDB).update(form) to the enquiry policy reset regression test, so the test now checks that the updated form is persisted.

Validation:

  • mvn -pl engine -Dtest=TestGroupDelegationConfigGenerator#shouldUpdateCustomEnquiryLayoutWhenProjectPoliciesAreReset test -Dgpg.skip=true — passed
  • git diff --check — passed
  • /root/.local/bin/coderabbit review --base upstream/dev --type uncommitted --plain before commit — no findings

Pushed follow-up commit: 81a5ffce3a.

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.

1 participant