FORMS-22032 Added support for reading associate properties container #1758
Conversation
Accessibility Violations Found
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
rismehta
left a comment
rismehta
left a comment
There was a problem hiding this comment.
Please add test cases and validate the schema changes, you can check the existing test suite
| if (FormConstants.CHANNEL_PRINT.equals(this.channel) && resource != null) { | ||
| Resource associatePropertiesResource = resource.getChild(CUSTOM_ASSOCIATE_PROPERTY_WRAPPER); | ||
| if (associatePropertiesResource != null) { | ||
| AssociateProperties associateProperties = associatePropertiesResource.adaptTo(AssociateProperties.class); |
There was a problem hiding this comment.
Please add an NPE check here. It shouldn’t occur due to continuous upgrade, but having a safeguard in place would still be helpful.
For example, if someone is using a very old add-on on cloud, this could potentially happen—even though the chances are quite low.
There was a problem hiding this comment.
@rismehta I am not sure where to put the NPE check? We already have a NPE check for "associateProperties" at line 618 before it's usage.
There was a problem hiding this comment.
This import will be undefined
import com.adobe.cq.forms.core.components.models.form.print.associate.AssociateProperties
in case of old addon, you would have to add a safe check if this class is not present
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
@rismehta I have added a feasible test case covering majority part of the added code, but coverage is still failing. Can we take an exception here, since only 2 lines are remaining to be covered and there is no straightforward approach to cover because: cc: @sufyanharoon |
rismehta
left a comment
rismehta
left a comment
There was a problem hiding this comment.
RTC. Approved. Spec changes are already reviewed
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Description
As per the concluded associate UI feature, this PR reflects the required changes to be able to read the associate properties container, "fd:associate".
Related Issue
FORMS-22032
How Has This Been Tested?
This has been verified e2e on a local cloud based setup, apart from the validation by existing tests and pipelines.
Types of changes
Checklist: