VACMS 6172: Adds validator and constraint to fail Drupal URLs.

[ndouglas]

Description

Relates to #6172. Wrote two tests: one for rich text fields and one for plain text fields. I don't think we need tests to prove it doesn't trigger falsely -- the normal functional tests should prove that. Assuming I didn't mess something up.

To Do

• Figure out why the error message is showing up twice. I think this could be related to this Paragraphs issue, but it'll take some work to confirm that and it might just be a red herring. EDIT: It was a red herring. The TextareaWidget doesn't consult the violation path, and there is no other way to target the value specifically. This was probably overlooked because TextareaWithSummaryWidget does consult the violation path.
• Figure out why the error link isn't working. The link points to e.g. <node path alias>#<subform, etc>-field-wysiwyg-0, whereas it should point to <node path alias>#<subform, etc>-field-wysiwyg-0-value. I played quite a bit yesterday with ConstraintViolationBuilder::atPath(), setting it to (string) $delta . '.value', etc, but I couldn't get it to retain the full path. I'm gonna have to do some deep diving to figure out where the path is getting mangled.
• Unit tests for the validator's methods:
  • ::validate()
  • ::validateHtml()
  • ::validateText()
• Possibly shift to validating the entire entity and looping through the fields and validating each one rather than validating each field separately. I don't know if that would cause or avert performance issues.
• Revalidate all existing nodes to see what sort of things show up. There's a possibility that some legitimate uses of these hostnames might be in existing content that would lead us to fine-tune the approach. Also, this would be a solid indicator that everything works as expected.
• Improve functional tests to confirm that: This was a core issue, not a failing in my code AFAICT.
  • the error links work (moving the user to the appropriate field)
  • the error message is only displayed a single time.
• Post some screenshots.
• ~Update formats of messages in line with feedback received from @rachel-kauff and @xiongjaneg ~ will address in followup
• Update screenshots after ^^ will address in followup

Testing done

Added two Behat tests. Was gonna add PHPUnit tests to test mechanically, and still might depending on my mood and dictatorial disposition of my reviewer.

Screenshots

QA steps

As user uid with user_role

1. Do this
2. Then that
3. Then validate Acceptance Criteria from issue

• This
• That
• The other thing

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.

Select Team for PR review

• Core Application Team
• Product Support Team

Is this PR blocked by another PR?

• DO NOT MERGE

Does this PR need review from a Product Owner

• Needs PO review

CMS user-facing annoucement

Is an announcement needed to let editors know of this change?

• Yes, and it's written in issue ____ and queued for publication.
  • Merge and ping @ rachel-kauff so she's ready to publish after deployment
• Yes, but it hasn't yet been written
  • Don't merge yet -- ping @ rachel-kauff to prompt her to write and queue content
• No announcement is needed for this code change.
  • Merge & carry on unburdened by announcements

[acrollet]

still might depending on my mood and dictatorial disposition of my reviewer.

it me!

[ndouglas]

(I was planning on adding unit tests, I just wanted to get this PR opened before the end of the day, and it's getting close for me)

[swirtSJW]

Nate this is very exciting,
I found 2 issues that you probably are already tracking but just in case:

The link to the error doesn't work. That's a 508 thing. The is probably a disconnect on the id it is referencing.
#edit-field-content-block-0-subform-field-wysiwyg

Also the error shows twice... which is probably why the link to it does not work.

[swirtSJW]

The other issue, which increases the risk but I don't think enough to not do it is right now the links are correctly flagged as bad if they have a *// but do not hit if there is no //
good fail - https://pr6199-gaktbhyes0fyr2yat6aed8p3eazzhgwp.ci.cms.va.gov/node/34054
good fail - //pr6199-gaktbhyes0fyr2yat6aed8p3eazzhgwp.ci.cms.va.gov/node/34054
bad pass - pr6199-gaktbhyes0fyr2yat6aed8p3eazzhgwp.ci.cms.va.gov/node/34054

I think the risk of .ci.cms.va.gov appearing somewhere else in the string and needing the added security of the // is super small.

We also want to double check with Jane and Rachel that this is not going to interfere with any linking to knowledge base content where they may want to intentionally be sending people to prod.cms.va.gov

[swirtSJW]

Last comment I promise :) It would be good to pull in @rachel-kauff to get the error message to be more in line with plainlanguage feedback
The link "bad place" contains an absolute CMS URL ( //pr6199-gaktbhyes0fyr2yat6aed8p3eazzhgwp.ci.cms.va.gov/node/34054 ).

[ndouglas]

Thanks, Steve!

I found 2 issues that you probably are already tracking but just in case:

Yeah, I think I'm setting the constraint in a weird way or something -- I haven't figured that out. The nonworking link is referring to the same thing as a working link does (e.g. if the field were empty) but minus a -value on the end. I'm not sure why that's happening, but presumably because I'm doing something unorthodox or just plain dumb. I think that's the same reason that the error message is doubled. I'm sure it's a quick fix once I figure out what I'm doing wrong 😃

I think the risk of .ci.cms.va.gov appearing somewhere else in the string and needing the added security of the // is super small.

EDIT: Ugh, just accidentally closed this and sent the comment prematurely. Awesome. #friday

Sure, I could take the // requirement out too. You're right, it's probably overly defensive.

We also want to double check with Jane and Rachel that this is not going to interfere with any linking to knowledge base content where they may want to intentionally be sending people to prod.cms.va.gov

You're absolutely right, of course. @xiongjaneg , @rachel-kauff, do you see any issues with angrily rejecting <anything>.cms.va.gov from all plain text and rich text fields?

[ndouglas]

I think I fixed this with a patch. Let's see what the automated tests say!

EDIT: Issue

[ndouglas]

I wrote a revalidation script and revalidated all however-many-thousand nodes, and this was the output.

According to this, four nodes have an absolute CMS URL:

[18214] => The Way Forward - Budgeting and Building an Emergency Savings Fund
	- field_body.0.value: The link "View or download the flyer" contains an absolute CMS URL ( https://prod.cms.va.gov/sites/default/files/2021-04/Budgeting_Building_Emergency_Fund_Seminar_04212021.pdf ).
[29658] => Audiology and speech at VA San Diego health care
	- field_body.0.value: The link "Learn more about VA hearing aids" contains an absolute CMS URL ( https://prod.cms.va.gov/health-care/order-hearing-aid-batteries-and-accessories ).
[29555] => Audiology and speech at VA Phoenix health care
	- field_body.0.value: The link "Learn more about VA hearing aids" contains an absolute CMS URL ( https://prod.cms.va.gov/health-care/order-hearing-aid-batteries-and-accessories ).
[29670] => Military sexual trauma care at VA San Diego health care
	- field_body.0.value: The link "Learn more about military sexual trauma" contains an absolute CMS URL ( https://prod.cms.va.gov/health-care/health-needs-conditions/military-sexual-trauma ).

It's likely worth going through to see what the other validation issues are. From a casual glance, they seem to be of a very small variety.

[ndouglas]

Followup for refining text in #6233.

[ndouglas]

@swirtSJW In order to get this merged, I opened up a followup issue to address how the error message is phrased: #6233.

[va-cms-bot]

va/tests/status-error:

+--------------------------------+----------+----------------------------------+

| Title                          | Severity | Summary                          |

+--------------------------------+----------+----------------------------------+

| Module and theme update status | Error    | Not secure! [1]                  |

|                                |          |                                  |

|                                |          | [1]                              |

|                                |          | http://default/admin/reports/upd |

|                                |          | ates                             |

|                                |          |                                  |

+--------------------------------+----------+----------------------------------+

[acrollet]

awesome tests!