Skip to content

Updating consent PUT to allow partial update#920

Merged
hasithakn merged 4 commits intowso2:mainfrom
Ashi1993:consent-manage-put
Mar 19, 2026
Merged

Updating consent PUT to allow partial update#920
hasithakn merged 4 commits intowso2:mainfrom
Ashi1993:consent-manage-put

Conversation

@Ashi1993
Copy link
Contributor

@Ashi1993 Ashi1993 commented Mar 18, 2026

Updating consent PUT to allow partial update

This PR updates consent PUT to allow partial update.

Issue link: #909

Doc Issue: Optional, link issue from documentation repository

Applicable Labels: Spec, product, version, type (specify requested labels)


Development Checklist

  1. Build complete solution with pull request in place.
  2. Ran checkstyle plugin with pull request in place.
  3. Ran Findbugs plugin with pull request in place.
  4. Ran FindSecurityBugs plugin and verified report.
  5. Formatted code according to WSO2 code style.
  6. Have you verified the PR doesn't commit any keys, passwords, tokens, usernames, or other secrets?
  7. Migration scripts written (if applicable).
  8. Have you followed secure coding standards in WSO2 Secure Engineering Guidelines?

Testing Checklist

  1. Written unit tests.
  2. Verified tests in multiple database environments (if applicable).
  3. Tested with BI enabled (if applicable).

Summary by CodeRabbit

  • Refactor
    • Consent update processing now treats several fields as optional/nullable and better handles partial updates; authorization resources and mappings are only applied when provided.
  • Behavior Change
    • Consent update payload no longer includes a consent ID field.
  • Tests
    • Update test payloads to remove consent ID from update requests.

Comment on lines +333 to +335
String receipt = updateRequestDTO.getReceipt() != null ? updateRequestDTO.getReceipt()
: storedConsent.getReceipt();
String status = updateRequestDTO.getStatus() != null ? updateRequestDTO.getStatus()
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 1

Suggested change
String receipt = updateRequestDTO.getReceipt() != null ? updateRequestDTO.getReceipt()
: storedConsent.getReceipt();
String status = updateRequestDTO.getStatus() != null ? updateRequestDTO.getStatus()
String receipt = updateRequestDTO.getReceipt() != null ? updateRequestDTO.getReceipt()
: storedConsent.getReceipt();
log.debug("Processing consent update for consentId: {}", consentId);

Comment on lines +347 to +349
ArrayList<ConsentMappingResource> consentMappingResources = null;
if (updateRequestDTO.getAuthorizationResources() != null) {
authorizationResources = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 2

Suggested change
ArrayList<ConsentMappingResource> consentMappingResources = null;
if (updateRequestDTO.getAuthorizationResources() != null) {
authorizationResources = new ArrayList<>();
if (updateRequestDTO.getAuthorizationResources() != null) {
log.debug("Updating authorization resources for consentId: {}", consentId);
authorizationResources = new ArrayList<>();

Copy link
Contributor

@wso2-engineering wso2-engineering bot left a comment

Choose a reason for hiding this comment

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

AI Agent Log Improvement Checklist

⚠️ Warning: AI-Generated Review Comments

  • The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
  • Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.

✅ Before merging this pull request:

  • Review all AI-generated comments for accuracy and relevance.
  • Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
Comment Accepted (Y/N) Reason
#### Log Improvement Suggestion No: 1
#### Log Improvement Suggestion No: 2

@coderabbitai
Copy link

coderabbitai bot commented Mar 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9faf03db-072e-47f4-8753-862dd83a679d

📥 Commits

Reviewing files that changed from the base of the PR and between 7e81e5c and ef31fe6.

📒 Files selected for processing (2)
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/InternalConsentUpdateRequestDTO.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.java
💤 Files with no reviewable changes (1)
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/InternalConsentUpdateRequestDTO.java

Walkthrough

Removed consentID from update payload and DTO; converted three primitive fields to boxed types in InternalConsentUpdateRequestDTO. Adjusted ConsentManageUtils to compute fields from the update payload when present and to instantiate authorization resources/consent mappings only if provided. Updated test constant payload accordingly.

Changes

Cohort / File(s) Summary
DTO changes
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/InternalConsentUpdateRequestDTO.java
Removed consentID field and its getter/setter. Converted consentFrequency:int → Integer, validityPeriod:long → Long, recurringIndicator:boolean → Boolean; updated getters/setters to use boxed (nullable) types.
Consent construction logic
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
Refactored construction of DetailedConsentResource to compute fields (receipt, status, frequency, validityPeriod, recurringIndicator, consentAttributes) from update payload when present, otherwise from stored consent. Authorization resources and consent mappings are now left null unless provided in the update request; removed prior fallback reuse of stored consent resources.
Test payload
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.java
Updated CONSENT_UPDATE_PAYLOAD JSON by removing the "consentID" field to match DTO changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibbled at fields, small and neat,
Pulled out a name, left others to be sweet,
Primitives boxed in a cozy heap,
Resources wake only when they leap,
A tiny hop—consent tidy and complete. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: converting primitive types to boxed types and removing the consentID field to enable partial updates in the consent PUT endpoint.
Description check ✅ Passed The PR description includes the required title, issue link, and a completed development checklist with most items checked. However, the testing checklist is incomplete and lacks detailed test documentation and automation test details as specified in the template.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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

@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: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java`:
- Around line 346-350: The current code initializes authorizationResources and
consentMappingResources to null and then passes them through to
DetailedConsentResource even when updateRequestDTO.getAuthorizationResources()
is absent, which can unintentionally clear existing associations; change the
logic to only construct and set authorizationResources and
consentMappingResources when updateRequestDTO.getAuthorizationResources() (and
the corresponding mapping list) is non-null—i.e., remove the null passthrough
and guard the assignment to DetailedConsentResource so you only call
setAuthorizationResources(...) and setConsentMappingResources(...) when you've
created non-null lists from updateRequestDTO, leaving existing fields untouched
otherwise (refer to updateRequestDTO, authorizationResources,
consentMappingResources, and DetailedConsentResource to locate the code).
- Around line 343-344: The code sets consentAttributes to null when
updateRequestDTO.getConsentAttributes() is absent, which causes stored
attributes to be erased; change the logic in ConsentManageUtils so that the
local variable consentAttributes keeps the existing stored attributes when
updateRequestDTO.getConsentAttributes() returns null (i.e., only overwrite
consentAttributes when updateRequestDTO.getConsentAttributes() != null),
referencing updateRequestDTO.getConsentAttributes() and the stored consent
attributes retrieval used elsewhere in the method to preserve prior values.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c39eec81-9237-4ca5-b861-e19a858e8e6b

📥 Commits

Reviewing files that changed from the base of the PR and between c5c19d8 and 7e81e5c.

📒 Files selected for processing (2)
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/InternalConsentUpdateRequestDTO.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java

@hasithakn hasithakn merged commit b1923e5 into wso2:main Mar 19, 2026
3 checks passed
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.

3 participants