VACMS-14700: Adds banner to VBA Facility #16076

omahane · 2023-11-09T14:15:45Z

Description

Relates to #14700

Testing done

Manually

QA Steps

Node:edit when banner is not enabled

As [email protected], edit Cheyenne VA Regional Benefit Office.

Confirm that Display a banner alert on this facility is not checked
Confirm that the Display a banner alert on this facility is the only form element showing
Save the node

Node:view when banner is not enabled

Confirm that "Display a banner alert on this facility" is set to "Off"

Node:edit when banner is enabled

Edit Cheyenne VA Regional Benefit Office
Check "Display a banner alert on this facility"
Confirm that the following fields show as required
- Banner type
- Dismissible by user?
- Banner title
- Banner content

Set no fields

Save the node
Confirm that Banner title throws a validation error "Fill out this field" before saving

Set the Banner title only

Add "Test title" to Banner title
Save the node
Confirm that the following message appears on the top of the page: "1 error has been found: Display a banner alert on this facility"
Confirm that the following message appears under Display a banner alert on this facility: "Banner type, Dismissible by user?, Banner content must not be empty when 'Display a banner alert on this facility' is checked."

Set the Banner type

Select "Information" in the banner type
Save the node
Confirm that the following message appears on the top of the page: "1 error has been found: Display a banner alert on this facility"
Confirm that the following message appears under Display a banner alert on this facility: "Dismissible by user?, Banner content must not be empty when 'Display a banner alert on this facility' is checked."

Set Dismissible by user?

Select "Allow site visitors to dismiss banner" in set Dismissible by user?
Save the node
Confirm that the following message appears on the top of the page: " 1 error has been found: Banner content"
Confirm that the following message appears under Banner content: "Banner content must not be empty when 'Display a banner alert on this facility' is checked."

Set Banner content

Add "Test content" to Banner content
Save the node
Confirm that the node saves

Node:view when banner is enabled

Confirm that "Display a banner alert on this facility" is on
Confirm that all banner fields are displaying

Node:view when banner is turned off again

Edit the node
Uncheck Display a banner alert on this facility
Save the node
Confirm that confirm that "Display a banner alert on this facility" is set to "Off" in node:view
Confirm that no banner fields show

Node:edit after banner has been turned off

Edit the node
Confirm that Display a banner alert on this facility is unchecked
Check Display a banner alert on this facility
Confirm that the other banner fields are not set

Definition of Done

Documentation has been updated, if applicable.
Tests have been added if necessary.
Automated tests have passed.
Code Quality Tests have passed.
Acceptance Criteria in related issue are met.
Manual Code Review Approved.
If there are field changes, front end output has been thoroughly checked.

Select Team for PR review

docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php

...a_facility/src/Plugin/Validation/Constraint/VbaFacilityRequiredFieldsConstraintValidator.php

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

PHP_CodeSniffer

docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php|174 col 4| Expected 4 space(s) before asterisk; 3 found
docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php|175 col 4| Expected 4 space(s) before asterisk; 3 found
docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php|176 col 4| Expected 4 space(s) before asterisk; 3 found
docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php|177 col 4| Expected 4 space(s) before asterisk; 3 found
docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php|244 col 1| More than 2 empty lines are not allowed
docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php|246 col 6| Line exceeds 80 characters; contains 85 characters
docroot/modules/custom/va_gov_vba_facility/src/Plugin/Validation/Constraint/VbaFacilityRequiredFieldsConstraintValidator.php|23 col 9| Line exceeds 80 characters; contains 81 characters

docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php

docroot/modules/custom/va_gov_vba_facility/js/set_banner_content_to_required.es6.js

docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php

...a_facility/src/Plugin/Validation/Constraint/VbaFacilityRequiredFieldsConstraintValidator.php

docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php

omahane · 2023-11-09T18:24:03Z

@davidmpickett Could you take a look at this and see what I might want to change before really tightening down the code? I created a user [email protected] who should best imitate an editor.

swirtSJW

This is looking quite good. I am still testing, but here are thoughts and questions from code review.

swirtSJW · 2023-11-15T15:02:17Z

config/sync/core.entity_form_display.node.vba_facility.default.yml

+      region: content
+      parent_name: ''
+      weight: 2
+      format_type: detailswithimage


Are we going to do an image?

I am not sure. We use this format_type in the CLP. I imagine that it would be useful. I wan't going to worry about putting one in now, though, but this way we can post-MVP.

Makes sense. Might want to spin up a ticket to reminder us.

config/sync/core.entity_view_display.node.vba_facility.default.yml

docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php

swirtSJW · 2023-11-15T22:32:56Z

...a_facility/src/Plugin/Validation/Constraint/VbaFacilityRequiredFieldsConstraintValidator.php

+          $empty_fields[] = $entity->get('field_dismissible_option')->getFieldDefinition()->getLabel();
+          $field_error_path[] = 'field_dismissible_option';
+        }
+        if (empty(trim($entity->get('field_banner_title')->value))) {


Nicely done adding the trim().

swirtSJW · 2023-11-15T22:34:22Z

...a_facility/src/Plugin/Validation/Constraint/VbaFacilityRequiredFieldsConstraintValidator.php

+
+        if (!empty($empty_fields)) {
+          if (count($field_error_path) > 1) {
+            // Multiple errors should go on the checkbox.


swirtSJW · 2023-11-15T23:00:02Z

When there is an error that points to the checkbox, because there are multiple, the jump link does not work

The link points to
#edit-field-vba-banner-panel
but the id on the field is
id="edit-field-vba-banner-panel-value"

Edit: this is not the issue Nate has tried to fix. This is an actual issue that might be possible to correct by setting the error path differently.

swirtSJW · 2023-11-15T23:13:49Z

The jumplink when the error is in the CKeditor is broken. This one is definitely a known issue and I created an upstream issue for it. I will find it an link it here when I do

Edit: here is the upstream issue We are using patch 6 currently, but it seems to not be working.

…rors in UI and testing.

… per PHPStan.

swirtSJW · 2023-11-19T22:05:02Z

I just resolved the conflicts

swirtSJW · 2023-11-19T23:16:07Z

I just created a drupal core issue for the error message jump link to the checkbox not working.
https://www.drupal.org/project/drupal/issues/3402684 which is separate from the existing ckeditor jumplink issue.

swirtSJW · 2023-11-19T23:58:13Z

PHP unit failure was unrelated. Tied to Post API endpoint not found. (an outtage of LH sandbox)

swirtSJW · 2023-11-20T00:11:02Z

I ran them by hand

All good.

* VACMS-14700: Adds config for banner fields * VACMS-14700: Adds config for persistence * VACMS-14700: Adds hiding and a little resetting on presave * VACMS-14700: Hides and reveals fields. Adds constraint. Clears fields. * VACMS-14700: Expands banner fieldset when checkbox is checked. * VACMS-14700: Hides banner fields on node:view when not checked * VACMS-14700: Adds config changes. * VACMS-14700: Changes config and select list option * VACMS-14700: Code cleanup * VACMS-14700: Fixes a couple typos. * VACMS-14700: Updates language after review. Updates code to remove errors in UI and testing. * VACMS-14700: Removes isset() as it always exists and is not nullable, per PHPStan. * VACMS-14700: Adds full content validation. * VACMS-14700: Removes N/A from dismissible, makes it show as required. * VACMS-14700: Orders methods * VACMS-14700: Makes dismissible not required. * VACMS-14700: Updates name, per PR review * VACMS-14700: Updates UI per Figma

omahane requested a review from a team as a code owner November 9, 2023 14:15

va-cms-bot temporarily deployed to Tugboat November 9, 2023 14:15 Destroyed

github-actions bot changed the title ~~Vacms 14700 vba banner~~ VACMS-14700: vba banner Nov 9, 2023

github-actions bot reviewed Nov 9, 2023

View reviewed changes

va-cms-bot temporarily deployed to Tugboat November 9, 2023 16:34 Destroyed

github-actions bot reviewed Nov 9, 2023

View reviewed changes

va-cms-bot temporarily deployed to Tugboat November 9, 2023 16:56 Destroyed

omahane force-pushed the VACMS-14700-vba-banner branch from b724d4f to b3eae90 Compare November 9, 2023 18:06

va-cms-bot temporarily deployed to Tugboat November 9, 2023 18:07 Destroyed

github-actions bot reviewed Nov 9, 2023

View reviewed changes

docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php Outdated Show resolved Hide resolved

docroot/modules/custom/va_gov_vba_facility/src/EventSubscriber/VbaFacilitySubscriber.php Outdated Show resolved Hide resolved

va-cms-bot temporarily deployed to Tugboat November 9, 2023 19:17 Destroyed

va-cms-bot temporarily deployed to Tugboat November 13, 2023 14:01 Destroyed

omahane force-pushed the VACMS-14700-vba-banner branch from 4d1e93d to 0273ee7 Compare November 13, 2023 21:15

va-cms-bot temporarily deployed to Tugboat November 13, 2023 21:15 Destroyed

va-cms-bot temporarily deployed to Tugboat November 13, 2023 21:46 Destroyed

omahane changed the title ~~VACMS-14700: vba banner~~ VACMS-14700: Adds banner to VBA Facility Nov 13, 2023

va-cms-bot temporarily deployed to Tugboat November 14, 2023 21:33 Destroyed

va-cms-bot temporarily deployed to Tugboat November 15, 2023 07:28 Destroyed

github-actions bot added the Facilities Facilities products (VAMC, Vet Center, etc) label Nov 15, 2023

va-cms-bot temporarily deployed to Tugboat November 15, 2023 13:47 Destroyed

omahane force-pushed the VACMS-14700-vba-banner branch from e3cf721 to 5e56050 Compare November 15, 2023 13:51

va-cms-bot temporarily deployed to Tugboat November 15, 2023 13:51 Destroyed

omahane requested a review from swirtSJW November 15, 2023 14:57

swirtSJW reviewed Nov 15, 2023

View reviewed changes

omahane force-pushed the VACMS-14700-vba-banner branch from 5e56050 to 33c2d32 Compare November 16, 2023 21:14

omahane force-pushed the VACMS-14700-vba-banner branch from 7b8ac99 to a43b906 Compare November 17, 2023 16:18

va-cms-bot temporarily deployed to Tugboat November 17, 2023 16:19 Destroyed

omahane added 18 commits November 19, 2023 16:51

VACMS-14700: Adds config for banner fields

514b8e9

VACMS-14700: Adds config for persistence

79053c7

VACMS-14700: Adds hiding and a little resetting on presave

ea93c2f

VACMS-14700: Hides and reveals fields. Adds constraint. Clears fields.

ec7b608

VACMS-14700: Expands banner fieldset when checkbox is checked.

54730b3

VACMS-14700: Hides banner fields on node:view when not checked

afae37e

VACMS-14700: Adds config changes.

99b025e

VACMS-14700: Changes config and select list option

28fd680

VACMS-14700: Code cleanup

1ebf258

VACMS-14700: Fixes a couple typos.

bf57acd

VACMS-14700: Updates language after review. Updates code to remove er…

ea32edb

…rors in UI and testing.

VACMS-14700: Removes isset() as it always exists and is not nullable,…

d62d9cb

… per PHPStan.

VACMS-14700: Adds full content validation.

d07ef82

VACMS-14700: Removes N/A from dismissible, makes it show as required.

47b9cae

VACMS-14700: Orders methods

24beb27

VACMS-14700: Makes dismissible not required.

2bd9370

VACMS-14700: Updates name, per PR review

6e206ab

VACMS-14700: Updates UI per Figma

ca22eaf

swirtSJW force-pushed the VACMS-14700-vba-banner branch from a43b906 to ca22eaf Compare November 19, 2023 22:04

va-cms-bot temporarily deployed to Tugboat November 19, 2023 22:04 Destroyed

swirtSJW merged commit 1c8f7f6 into main Nov 20, 2023
14 checks passed

swirtSJW deleted the VACMS-14700-vba-banner branch November 20, 2023 00:11

omahane mentioned this pull request Sep 19, 2024

Drupal: Add Banner fields to Vet Center content type #18924

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VACMS-14700: Adds banner to VBA Facility #16076

VACMS-14700: Adds banner to VBA Facility #16076

omahane commented Nov 9, 2023 •

edited by swirtSJW

Loading

github-actions bot left a comment

omahane commented Nov 9, 2023

swirtSJW left a comment

swirtSJW Nov 15, 2023

omahane Nov 16, 2023

swirtSJW Nov 17, 2023

swirtSJW Nov 15, 2023

swirtSJW Nov 15, 2023

swirtSJW commented Nov 15, 2023 •

edited

Loading

swirtSJW commented Nov 15, 2023 •

edited

Loading

swirtSJW commented Nov 19, 2023

swirtSJW commented Nov 19, 2023 •

edited

Loading

swirtSJW commented Nov 19, 2023

swirtSJW commented Nov 20, 2023

VACMS-14700: Adds banner to VBA Facility #16076

VACMS-14700: Adds banner to VBA Facility #16076

Conversation

omahane commented Nov 9, 2023 • edited by swirtSJW Loading

Description

Testing done

QA Steps

Node:edit when banner is not enabled

Node:view when banner is not enabled

Node:edit when banner is enabled

Set no fields

Set the Banner title only

Set the Banner type

Set Dismissible by user?

Set Banner content

Node:view when banner is enabled

Node:view when banner is turned off again

Node:edit after banner has been turned off

Definition of Done

Select Team for PR review

github-actions bot left a comment

Choose a reason for hiding this comment

omahane commented Nov 9, 2023

swirtSJW left a comment

Choose a reason for hiding this comment

swirtSJW Nov 15, 2023

Choose a reason for hiding this comment

omahane Nov 16, 2023

Choose a reason for hiding this comment

swirtSJW Nov 17, 2023

Choose a reason for hiding this comment

swirtSJW Nov 15, 2023

Choose a reason for hiding this comment

swirtSJW Nov 15, 2023

Choose a reason for hiding this comment

swirtSJW commented Nov 15, 2023 • edited Loading

swirtSJW commented Nov 15, 2023 • edited Loading

swirtSJW commented Nov 19, 2023

swirtSJW commented Nov 19, 2023 • edited Loading

swirtSJW commented Nov 19, 2023

swirtSJW commented Nov 20, 2023

omahane commented Nov 9, 2023 •

edited by swirtSJW

Loading

swirtSJW commented Nov 15, 2023 •

edited

Loading

swirtSJW commented Nov 15, 2023 •

edited

Loading

swirtSJW commented Nov 19, 2023 •

edited

Loading