[FORMS-8058] implement review component #1468

amit-mnnit · 2024-10-28T10:27:01Z

add css for now

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes and the overall coverage did not decrease.
All unit tests pass on CircleCi.
I ran all tests locally and they pass.

codecov · 2024-10-29T05:26:27Z

Codecov Report

Attention: Patch coverage is 84.44444% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.43%. Comparing base (aeed3f4) to head (96aa9d5).

Files with missing lines	Patch %	Lines
...nts/internal/servlets/ReviewDataSourceServlet.java	81.08%	0 Missing and 7 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                dev    #1468      +/-   ##
============================================
+ Coverage     82.35%   82.43%   +0.08%     
- Complexity      928      945      +17     
============================================
  Files           103      105       +2     
  Lines          2380     2425      +45     
  Branches        323      332       +9     
============================================
+ Hits           1960     1999      +39     
  Misses          258      258              
- Partials        162      168       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

adobe-bot · 2024-10-29T05:36:02Z

Accessibility Violations Found

Id	Impact
focus-order-semantics	minor
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-10-29T05:41:17Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-10-29T05:42:04Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-10-29T05:42:26Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	92	96	96	75

adobe-bot · 2024-10-29T05:42:26Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	96	75

adobe-bot · 2024-10-29T05:42:38Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-10-29T10:56:33Z

Accessibility Violations Found

Id	Impact
focus-order-semantics	minor
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-10-29T11:01:36Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	96	75

adobe-bot · 2024-10-29T11:01:36Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	93	96	96	75

adobe-bot · 2024-10-29T11:01:46Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-10-29T11:02:48Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-10-29T11:02:54Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

barshat7 · 2024-11-05T08:53:32Z

.../main/java/com/adobe/cq/forms/core/components/internal/servlets/ReviewDataSourceServlet.java

+        if (resourceResolver != null) {
+            Resource componentInstance = resourceResolver.getResource(componentInstancePath);
+            Resource formInstance = ComponentUtils.getFormContainer(componentInstance);
+            FormContainer formContainer = formInstance.adaptTo(FormContainer.class);


formInstance could be null, please add Null pointer check,

barshat7 · 2024-11-05T09:08:33Z

.../main/java/com/adobe/cq/forms/core/components/internal/servlets/ReviewDataSourceServlet.java

+            for (Base panel : panels) {
+                String name = panel != null ? panel.getName() : "";
+                String title = "";
+                if (panel != null) {


put the entire code under panel != null check rather than duplicating, also how are we preventing the review component itself being listed in the linked panel dropdown?

barshat7 · 2024-11-05T09:12:43Z

.../main/java/com/adobe/cq/forms/core/components/internal/servlets/ReviewDataSourceServlet.java

+        request.setAttribute(DataSource.class.getName(), actionTypeDataSource);
+    }
+
+    private List<? extends ComponentExporter> getPanels(FormComponent formContainer) {


It looks like this method doesn't just return Panels, it can return a field as well. The method name is misleading

Have we tested this with a composite component like TnC?

Changed, checked for all components

rismehta · 2024-11-08T05:48:20Z

...ore/src/main/java/com/adobe/cq/forms/core/components/internal/models/v1/form/ReviewImpl.java

+    }
+
+    @Override
+    public @NotNull Map<String, Object> getProperties() {


Let's not expose this in properties until we have a customer use-case to render review in headless implementation

We use these properties at runtime (reviewview.js) to render and display the editModeButton or the linked panel

rismehta · 2024-11-08T05:48:35Z

...ore/src/main/java/com/adobe/cq/forms/core/components/internal/models/v1/form/ReviewImpl.java

+    @JsonIgnore
+    @ValueMapValue(injectionStrategy = InjectionStrategy.OPTIONAL, name = "editAction")
+    @Nullable
+    private String editAction;


JsonIgnore should only be part of interface

rismehta · 2024-11-08T05:49:29Z

...ore/src/main/java/com/adobe/cq/forms/core/components/internal/models/v1/form/ReviewImpl.java

+    @JsonIgnore
+    @ValueMapValue(injectionStrategy = InjectionStrategy.OPTIONAL, name = "editAction")
+    @Nullable
+    private String editAction;


What do you mean by editAction ? Its not clear. IIUC, this property is to enable edit action on UI, shouldn't this be a boolean

rismehta · 2024-11-08T05:51:37Z

.../main/java/com/adobe/cq/forms/core/components/internal/servlets/ReviewDataSourceServlet.java

+        ResourceResolver resourceResolver = request.getResourceResolver();
+        String componentInstancePath = request.getRequestPathInfo().getSuffix();
+        List<Resource> resources = new ArrayList<>();
+        if (resourceResolver != null) {


We should change the implementation by using FormContainer#visit, like we do during form submission, that way the code would be more readable.

In the current implementation, we were walking twice which is not required.

We cannot use the FormContainer#visit method as it does not solve our use case. During the review, if a container has more than one child, we need to stop the iteration and return the children of that container(To create a list of panel).

rismehta · 2024-11-08T05:53:04Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/Review.java

+ */
+@ConsumerType
+public interface Review extends Base {
+    String[] getLinkedPanels();


The interface should be of provider type

please add java doc to all the methods. What does String[] array mean, if it is qualified name, then please link the QN documentation in the java doc

I checked for other components, We are using @ConsumerType not provider type.
Add Java doc, please check

rismehta · 2024-11-08T05:53:20Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/Review.java

+/**
+ * Defines the {@code Review} Sling Model used for the {@code /apps/core/fd/components/form/review} component.
+ *
+ * @since com.adobe.cq.forms.core.components.models 2.1.0


Please bump the version in minor version in package-info and use the same version here

rismehta · 2024-11-08T05:53:25Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/Review.java

@@ -0,0 +1,30 @@
+/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ ~ Copyright 2023 Adobe


rismehta · 2024-11-08T05:55:48Z

ui.af.apps/src/main/content/jcr_root/apps/core/fd/components/form/review/v1/review/README.md

+## BEM Description
+```
+BLOCK cmp-adaptiveform-review
+    ELEMENT cmp-adaptiveform-review__container


I don't think this BEM is correct should __panel be inside __container.

Also why is __field a sibling of container, shouldn't it be a child ?

Please check attached screenshots for batter understanding of BEM.

rismehta · 2024-11-08T05:57:32Z

ui.af.apps/src/main/content/jcr_root/apps/core/fd/components/form/review/v1/review/README.md

+2. `./hideTitle` - if set to `true`, the label of this field will be hidden
+3. `./name` - defines the name of the field, which will be submitted with the form data
+3. `./linkedPanels` - defines linked panels for reviewing the panel. If no panel is linked, the component will review the entire form.
+3. `./editAction` - defines the edit action for editing the fields, panels, field & panel, or none


Let's rename editAction to ./editModeAction : Determines the action or behavior for entering "edit mode"

rismehta · 2024-11-08T05:58:37Z

...pps/src/main/content/jcr_root/apps/core/fd/components/form/review/v1/review/_cq_template.xml

+<jcr:root xmlns:jcr="http://www.jcp.org/jcr/1.0" xmlns:nt="http://www.jcp.org/jcr/nt/1.0" xmlns:cq="http://www.day.com/jcr/cq/1.0"
+    jcr:primaryType="nt:unstructured"
+    jcr:title="Review"
+    fieldType="plain-text"/>


fieldType should be defaulted and it should not be mandatory for review. We should get rid of fieldType from JCR and add it in the interface directly as fallback like we have done for other components

adobe-bot · 2024-11-18T05:18:48Z

Accessibility Violations Found

Id	Impact
focus-order-semantics	minor
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-11-18T05:23:30Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	96	75

adobe-bot · 2024-11-18T05:23:30Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	93	96	96	75

adobe-bot · 2024-11-18T05:23:40Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-11-18T05:24:44Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-11-18T05:24:59Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-11-18T06:23:32Z

Accessibility Violations Found

Id	Impact
focus-order-semantics	minor
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-11-18T06:27:01Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	93	96	96	75

adobe-bot · 2024-11-18T06:27:01Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	96	75

adobe-bot · 2024-11-18T06:27:11Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-11-18T06:29:04Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-11-18T06:29:48Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

rismehta

added few more comments

rismehta · 2024-11-18T07:38:40Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/Review.java

+/**
+ * Defines the {@code Review} Sling Model used for the {@code /apps/core/fd/components/form/review} component.
+ *
+ * @since com.adobe.cq.forms.core.components.models 5.9.5


Please update version to 5.9.6

rismehta · 2024-11-18T07:38:47Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/Review.java

+    /**
+     * @return an array of linked panels to be reviewed on the review page. Each linked panel is the name of a panel that is linked to the
+     *         review page.
+     * @since com.adobe.cq.forms.core.components.models.form 5.9.5


even this should be 5.9.6

rismehta · 2024-11-18T07:39:31Z

.../main/java/com/adobe/cq/forms/core/components/internal/servlets/ReviewDataSourceServlet.java

@@ -0,0 +1,135 @@
+/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ ~ Copyright 2023 Adobe


this should be 2024

rismehta · 2024-11-18T07:41:02Z

ui.af.apps/src/main/content/jcr_root/apps/core/fd/components/form/review/v1/review/README.md

+    ELEMENT cmp-adaptiveform-review__container
+      ELEMENT cmp-adaptiveform-review__panel
+      MODIFIER cmp-adaptiveform-review__panel--repeatable
+        ELEMENT cmp-adaptiveform-review__label-containe


This should be -container

rismehta · 2024-11-18T07:42:46Z

ui.af.apps/src/main/content/jcr_root/apps/core/fd/components/form/review/v1/review/README.md

+        ELEMENT cmp-adaptiveform-review__label-containe
+          ELEMENT cmp-adaptiveform-review__label
+          ELEMENT cmp-adaptiveform-review__edit-button
+        ELEMENT cmp-adaptiveform-review__value


Please indent the BEM correctly, it is still incorrect

BLOCK cmp-adaptiveform-review ELEMENT cmp-adaptiveform-review__container ELEMENT cmp-adaptiveform-review__panel MODIFIER cmp-adaptiveform-review__panel--repeatable ELEMENT cmp-adaptiveform-review__label-container ELEMENT cmp-adaptiveform-review__label ELEMENT cmp-adaptiveform-review__edit-button ELEMENT cmp-adaptiveform-review__value ELEMENT cmp-adaptiveform-review__field ELEMENT cmp-adaptiveform-review__label ELEMENT cmp-adaptiveform-review__value ELEMENT cmp-adaptiveform-review__edit-button ELEMENT cmp-adaptiveform-review__text ELEMENT cmp-adaptiveform-review__label

rismehta · 2024-11-18T07:47:33Z

.../main/java/com/adobe/cq/forms/core/components/internal/servlets/ReviewDataSourceServlet.java

+            if (formInstance != null) {
+                FormContainer formContainer = formInstance.adaptTo(FormContainer.class);
+                List<Base> panelList = (List<Base>) getMultipleChildPanels(formContainer);
+                List<Base> panels = panelList.stream().filter(x -> "panel".equals(x.getFieldType())).collect(Collectors.toList());


Can we please re-use the existing constant or create a new constant if not present

rismehta · 2024-11-18T07:48:18Z

...tent/jcr_root/apps/core/fd/components/form/review/v1/review/clientlibs/site/js/reviewview.js

+
+    static addClassModifier(element, item) {
+      if(item.fieldType !== 'panel'){
+        element.querySelector('div').classList.add(Review.bemBlock + `__field--${item.fieldType}`);


Let's not have any hardcoded strings in the code, please create constants in the top and re-use the constant. That way code is more readable and easy to maintain

rismehta · 2024-11-18T07:48:41Z

...tent/jcr_root/apps/core/fd/components/form/review/v1/review/clientlibs/site/js/reviewview.js

+    }
+
+    static addClassModifier(element, item) {
+      if(item.fieldType !== 'panel'){


Please create constant for this as well. In fact, we should have fieldType constants in a common place and re-use them across

rismehta · 2024-11-18T07:49:22Z

...tent/jcr_root/apps/core/fd/components/form/review/v1/review/clientlibs/site/js/reviewview.js

+    static bemBlock = 'cmp-adaptiveform-review';
+    static templateAttribute = 'data-cmp-review';
+    static DATA_ATTRIBUTE_VISIBLE = 'data-cmp-visible';
+    static hideFieldFromReview = ['button', 'plain-text', 'captcha', 'image'];


Please expose them from af-core inside ui.frontend module. Core component should not own these constants

rismehta · 2024-11-18T07:51:03Z

...tent/jcr_root/apps/core/fd/components/form/review/v1/review/clientlibs/site/js/reviewview.js

+      const templates = this.getTemplates();
+      let mappings = {};
+      templates.forEach(template => {
+        const type = template.getAttribute(Review.templateAttribute + '-fieldType');


please have constant for this in this file

add css for now

adobe-bot · 2024-11-19T07:59:48Z

Accessibility Violations Found

Id	Impact
focus-order-semantics	minor
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-11-19T08:03:46Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	91	96	96	75

adobe-bot · 2024-11-19T08:03:46Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	96	75

adobe-bot · 2024-11-19T08:03:57Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-11-19T08:04:51Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-11-19T08:05:30Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

amit-mnnit force-pushed the review-FORMS-8058-v2 branch from 77ecd81 to 5ef355c Compare October 29, 2024 05:17

github-actions bot force-pushed the review-FORMS-8058-v2 branch from 5ef355c to 9c05348 Compare October 29, 2024 05:17

amit-mnnit force-pushed the review-FORMS-8058-v2 branch from 9c05348 to 041bdcf Compare October 29, 2024 10:36

github-actions bot force-pushed the review-FORMS-8058-v2 branch from 041bdcf to bc714a3 Compare October 29, 2024 10:37

barshat7 reviewed Nov 5, 2024

View reviewed changes

rismehta reviewed Nov 8, 2024

View reviewed changes

amit-mnnit force-pushed the review-FORMS-8058-v2 branch 5 times, most recently from 7c4dae9 to 13098cf Compare November 18, 2024 05:01

rismehta requested changes Nov 18, 2024

View reviewed changes

Amit Kumar added 5 commits November 19, 2024 07:41

[FORMS-8058] implement review component

654bbf5

add css for now

add test cases

d7e4646

fix review points

f48b0e3

add test case

5169d48

fix review points #2

96aa9d5

github-actions bot force-pushed the review-FORMS-8058-v2 branch from 005945e to 96aa9d5 Compare November 19, 2024 07:41

		@@ -0,0 +1,30 @@
		/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
		~ Copyright 2023 Adobe

		@@ -0,0 +1,135 @@
		/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
		~ Copyright 2023 Adobe

[FORMS-8058] implement review component #1468

Are you sure you want to change the base?

[FORMS-8058] implement review component #1468

Conversation

amit-mnnit commented Oct 28, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

adobe-bot commented Oct 29, 2024

Accessibility Violations Found

adobe-bot commented Oct 29, 2024

Accessibility Violations Found

adobe-bot commented Oct 29, 2024

Accessibility Violations Found

adobe-bot commented Oct 29, 2024

Lighthouse scores (mobile)

adobe-bot commented Oct 29, 2024

Lighthouse scores (desktop)

adobe-bot commented Oct 29, 2024

Accessibility Violations Found

adobe-bot commented Oct 29, 2024

Accessibility Violations Found

adobe-bot commented Oct 29, 2024

Lighthouse scores (desktop)

adobe-bot commented Oct 29, 2024

Lighthouse scores (mobile)

adobe-bot commented Oct 29, 2024

Accessibility Violations Found

adobe-bot commented Oct 29, 2024

Accessibility Violations Found

adobe-bot commented Oct 29, 2024

Accessibility Violations Found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshat7 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rismehta Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Nov 18, 2024

Accessibility Violations Found

adobe-bot commented Nov 18, 2024

Lighthouse scores (desktop)

adobe-bot commented Nov 18, 2024

Lighthouse scores (mobile)

adobe-bot commented Nov 18, 2024

Accessibility Violations Found

adobe-bot commented Nov 18, 2024

Accessibility Violations Found

adobe-bot commented Nov 18, 2024

Accessibility Violations Found

adobe-bot commented Nov 18, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

barshat7 Nov 5, 2024 •

edited

Loading

rismehta Nov 8, 2024 •

edited

Loading