VACMS-13852: Creates EntityEvent Subscriber. #14337

ndouglas · 2023-07-12T22:14:10Z

Description

Closes #14495 as well.

I removed the service declaration for the current entity event subscriber to see what (if anything) breaks.

I did a direct port of Cameron's tests for the current version of EntityEventSubscriber. After some work to adjust for different types, all of them pass... which makes me deeply suspicious. I've been careful, and have even done partial copies of these tests, but that still seems really unexpectedly fortunate.

I think I'm going to add a couple more test suites to test from a few different angles to see if I can tease out any weird corner cases. I might add an extended test suite for facility/banner updates etc because I'm not sure unit tests can really account for all of the weirdness in Drupal.

QA Steps (more probably forthcoming)

Followup Items

replace "exception occurred" with "N/A" in the log messages.

…Entity-Event-Subscriber

…ing messages.

…Entity-Event-Subscriber

ndouglas · 2023-07-26T16:52:18Z

docroot/modules/custom/va_gov_build_trigger/va_gov_build_trigger.services.yml

-  va_gov_build_trigger.entity_event_subscriber:
-    class: Drupal\va_gov_build_trigger\EventSubscriber\EntityEventSubscriber
-    arguments: ['@va_gov_build_trigger.build_requester', '@va_gov.build_trigger.environment_discovery']
-    tags:
-      - { name: event_subscriber }


This PR replaces the EntityEventSubscriber residing in va_gov_build_trigger.

ndouglas · 2023-07-26T16:59:11Z

...modules/custom/va_gov_content_release/src/EntityEvent/Strategy/Plugin/StrategyPluginBase.php

EntityEvent Strategy plugins encapsulate the logic for responding to entity events. This is so that we can switch out the "Tugboat" behavior for the "Prod" behavior, or insert a new behavior ("VerboseFalse") that will always refuse to trigger a content release, but will log what would have happened on prod.

ndouglas · 2023-07-26T17:08:07Z

docroot/modules/custom/va_gov_content_release/src/Plugin/EntityEventStrategy/VerboseFalse.php

This entity event strategy is active locally and on Tugboat. It never releases content (consistently with our desires), but performs the same calculations and logs the results of those calculations.

The intention is to resolve #14495.

ndouglas · 2023-07-26T17:11:37Z

docroot/modules/custom/va_gov_content_release/src/EventSubscriber/EntityEventSubscriber.php

+    $strategy = $this->strategyPluginManager->getStrategy($strategyId);
+    if ($strategy->shouldTriggerContentRelease($node)) {
+      $reason = $strategy->getReasonMessage($node);
+      $this->request->submitRequest($reason);


The Request service uses the same underlying machinery as the current system. See here: https://github.com/department-of-veterans-affairs/va.gov-cms/blob/main/docroot/modules/custom/va_gov_content_release/src/Request/Request.php

So if the strategy says to release, the system should release 🙂

This is all very readable and easy to follow .

ndouglas · 2023-07-26T17:26:09Z

docroot/modules/custom/va_gov_content_types/src/Traits/ContentReleaseTriggerTrait.php

A lot of the groundwork was laid already, but the effective change in this PR is to shift the content release logic into bundle classes and out of the entity event subscriber.

The main reason I want to do that is not because the existing code is bad, but because of all of the inline checks that are needed to make the code safe. For instance if ($node->original && $node->original instanceof NodeInterface && $node->original->hasField('...')) { ... }.

So I'm working on traits for bundle classes so that we can hopefully do all of the logic in one place, test it, and then use tested code elsewhere in the system, with the hope of lowering the "pucker factor" when we make content release-related changes.

Of course, the pucker factor for this PR is fairly high 😰

Yes the pucker factor is high, but I feel good that it is becoming easier to test and validate in all environs.

ndouglas · 2023-07-26T17:35:16Z

docroot/modules/custom/va_gov_environment/src/Environment/Environment.php

-    $environment = $settings->get('va_gov_environment')['environment'];
+    $environment = $settings->get('va_gov_environment')['environment'] ?? '(no value provided)';


The specific reason for this change is to prevent exceptions with messages like "Invalid Environment: ", which isn't immediately clear.

ndouglas · 2023-07-26T17:37:15Z

tests/phpunit/Service/VaGovUrlServiceTest.php

-    $this->config = ['hash_salt' => 'SCVSPZNSKKK5XCRJ1WLE'];
+    $this->config = [
+      'hash_salt' => 'SCVSPZNSKKK5XCRJ1WLE',
+      // This is temporary. See #14338.
+      'va_gov_environment' => [
+        'environment' => 'staging',
+      ],
+    ];


This indirectly triggers the Environment Discovery service, which needs this data to be set. If not, it'll throw an exception. #14338 involves updating this service to use the new system, which should make this test more concise, more reliable, less problematic, etc.

ndouglas · 2023-07-26T17:40:28Z

tests/phpunit/va_gov_content_release/unit/EventSubscriber/EntityEventSubscriberTest.php

This is an almost exact clone of the tests for the existing EntityEventSubscriber -- just modified slightly to account for the changes in API. They all pass. But that doesn't guarantee that the old tests are correct, or the new tests are sufficient.

Do we need to leave a human breadcrumb that changes to one needs to be made in the other so they remain clone-ish?

Hoping to get this merged before that becomes an issue 😁

ndouglas · 2023-07-26T18:00:13Z

tests/phpunit/va_gov_content_types/unit/ModuleTest.php

This is an ugly test and probably not that valuable.

swirtSJW

Code is looking good/amazing. Thanks for taking this all on. A couple questions and comments.

swirtSJW · 2023-07-26T21:25:15Z

...es/custom/va_gov_content_release/src/EntityEvent/Strategy/Plugin/StrategyPluginInterface.php

+   * @return string
+   *   The reason message.
+   */
+  public function getReasonMessage(VaNodeInterface $node) : string;


swirtSJW · 2023-07-26T21:31:31Z

docroot/modules/custom/va_gov_content_release/src/EntityEvent/Strategy/Resolver/Resolver.php

@@ -39,8 +39,8 @@ public function getStrategyId() : string {
    $environment = $this->environmentDiscovery->getEnvironment();
    return match (TRUE) {
      $environment->isBrd() => static::STRATEGY_ON_DEMAND,
-      $environment->isTugboat() => static::STRATEGY_NEVER,
-      $environment->isLocalDev() => static::STRATEGY_NEVER,
+      $environment->isTugboat() => static::STRATEGY_VERBOSE_NEVER,


Ooh this is a hard one. It is like one of those photos that you can see two ways

I read it one time as Strategy Verbose, Never
then read it again as Strategy, Verbose Never

unfortunately 'STRATEGY_NEVER_VERBOSE' suffers from the same problem and is no better. So I will have to get over it ;)

I was staring at those constant names earlier trying to think of something better, heh.

swirtSJW · 2023-07-26T21:35:49Z

docroot/modules/custom/va_gov_content_release/src/EventSubscriber/EntityEventSubscriber.php

+    $strategy = $this->strategyPluginManager->getStrategy($strategyId);
+    if ($strategy->shouldTriggerContentRelease($node)) {
+      $reason = $strategy->getReasonMessage($node);
+      $this->request->submitRequest($reason);


This is all very readable and easy to follow .

docroot/modules/custom/va_gov_content_release/src/Strategy/Plugin/StrategyPluginBase.php

swirtSJW · 2023-07-26T21:59:12Z

docroot/modules/custom/va_gov_content_types/src/Traits/ContentReleaseTriggerTrait.php

Yes the pucker factor is high, but I feel good that it is becoming easier to test and validate in all environs.

swirtSJW · 2023-07-26T22:07:57Z

tests/phpunit/va_gov_content_release/unit/EventSubscriber/EntityEventSubscriberTest.php

Do we need to leave a human breadcrumb that changes to one needs to be made in the other so they remain clone-ish?

tests/phpunit/va_gov_content_types/unit/Traits/ContentReleaseTriggerTraitTest.php

swirtSJW · 2023-07-27T14:23:58Z

Related comment from the other issue #14495 (comment)

…Entity-Event-Subscriber

swirtSJW · 2023-07-27T15:24:34Z

The code looks fantastic. I added some more QA steps, but have not run through them yet, but I plan to after I get back from a dentist apt.

swirtSJW · 2023-07-27T19:31:37Z

@ndouglas While testing the save of a system full width banner, I notices this in the log message. Do we want these showing as exceptions. They are really just fields or properties that can not be assessed on this particular content type.

ndouglas · 2023-07-27T19:35:26Z

@swirtSJW Yeah, that's intentional on my part (though debatable). I wanted to make the logic of retrieving and displaying these details as simple as possible, to avoid having the same logic (e.g. if ($isModerated) { $result['isDraft'] = $this->isDraft; }) in multiple places.

It might be better if I say "N/A" for each of those values. That's really what is implied there.

swirtSJW · 2023-07-27T19:44:11Z

It might be better if I say "N/A" for each of those values. That's really what is implied there.

That version seems less alarming and more informative. Or "Not available"

swirtSJW

Banner create: Pass

Banner publish: Pass

System banner : Pass

VAMC Facility: passed

This all tests out for me @ndouglas

Really nice work. Thank you for undertaking it.

…Entity-Event-Subscriber

…affairs#14337) * VACMS-13852: Creates EntityEvent Subscriber. * Tweaks to prevent test failures. * Yeah, no clue what I'm going to do here. * Test all event types. * Adds unit test coverage for LocalFilesystemBuildFileTest. * Test coverage. * Fix trait. * IsFacilityTraitTest. * Ignore coverage on plugin managers and plugin base classes. * Unit tests for the OnDemand entity event strategy plugin. * Make internal logic a little more consistent. * Disable current Entity Event Subscriber 👀 * Adds VerboseFalse entity event strategy plugin. * Oops, my plugins didn't have a logger 😧 * Adds mocks for OnDemand test. * Tests for VerboseFalse. * ContentReleaseTriggerTrait::hasTriggeringChanges() did the Bad Thing™ * D'oh. * Fixes test. * Fix tests. * VerboseFalse and tests. * Specify an exception occurred when getting a node detail threw. * Further tweaks. * Ugh, I'm an idiot. * Updates to VerboseFalse and VerboseFalseTest to support better debugging messages. * Removes bypass from test. * 'exception occurred' => 'Not Applicable'

VACMS-13852: Creates EntityEvent Subscriber.

cb6574b

va-cms-bot temporarily deployed to Tugboat July 12, 2023 22:14 Destroyed

Tweaks to prevent test failures.

93f5a22

va-cms-bot temporarily deployed to Tugboat July 13, 2023 13:43 Destroyed

Yeah, no clue what I'm going to do here.

eac26bb

va-cms-bot temporarily deployed to Tugboat July 13, 2023 14:24 Destroyed

Merge remote-tracking branch 'upstream/main' into VACMS-13852-Create-…

18c9f98

…Entity-Event-Subscriber

va-cms-bot temporarily deployed to Tugboat July 13, 2023 17:37 Destroyed

Merge remote-tracking branch 'upstream/main' into VACMS-13852-Create-…

c7ffab6

…Entity-Event-Subscriber

va-cms-bot temporarily deployed to Tugboat July 13, 2023 17:59 Destroyed

Test all event types.

c57c096

va-cms-bot temporarily deployed to Tugboat July 13, 2023 19:21 Destroyed

Adds unit test coverage for LocalFilesystemBuildFileTest.

b1a8c58

va-cms-bot temporarily deployed to Tugboat July 13, 2023 20:37 Destroyed

ndouglas added 2 commits July 14, 2023 09:09

Merge remote-tracking branch 'upstream/main' into VACMS-13852-Create-…

381a95a

…Entity-Event-Subscriber

Test coverage.

115f8fe

va-cms-bot temporarily deployed to Tugboat July 14, 2023 18:14 Destroyed

Fix trait.

4e4136c

va-cms-bot temporarily deployed to Tugboat July 14, 2023 18:20 Destroyed

IsFacilityTraitTest.

c936301

va-cms-bot temporarily deployed to Tugboat July 14, 2023 18:52 Destroyed

Ignore coverage on plugin managers and plugin base classes.

926a1f3

va-cms-bot temporarily deployed to Tugboat July 14, 2023 20:07 Destroyed

ndouglas added 2 commits July 17, 2023 09:16

Unit tests for the OnDemand entity event strategy plugin.

93db6c5

Make internal logic a little more consistent.

d72bb43

va-cms-bot temporarily deployed to Tugboat July 17, 2023 13:17 Destroyed

Merge remote-tracking branch 'upstream/main' into VACMS-13852-Create-…

a284bc3

…Entity-Event-Subscriber

va-cms-bot temporarily deployed to Tugboat July 17, 2023 13:32 Destroyed

Merge remote-tracking branch 'upstream/main' into VACMS-13852-Create-…

c803d86

…Entity-Event-Subscriber

va-cms-bot temporarily deployed to Tugboat July 17, 2023 16:31 Destroyed

Specify an exception occurred when getting a node detail threw.

e48c691

va-cms-bot temporarily deployed to Tugboat July 25, 2023 15:03 Destroyed

va-cms-bot temporarily deployed to Tugboat July 25, 2023 15:38 Destroyed

Further tweaks.

95744cb

va-cms-bot temporarily deployed to Tugboat July 25, 2023 18:33 Destroyed

Ugh, I'm an idiot.

5212125

va-cms-bot temporarily deployed to Tugboat July 25, 2023 19:59 Destroyed

Updates to VerboseFalse and VerboseFalseTest to support better debugg…

9948b83

…ing messages.

va-cms-bot temporarily deployed to Tugboat July 26, 2023 14:40 Destroyed

Merge remote-tracking branch 'upstream/main' into VACMS-13852-Create-…

4cd27d3

…Entity-Event-Subscriber

ndouglas commented Jul 26, 2023

View reviewed changes

swirtSJW reviewed Jul 26, 2023

View reviewed changes

Removes bypass from test.

4d7ba25

va-cms-bot temporarily deployed to Tugboat July 27, 2023 00:15 Destroyed

ndouglas marked this pull request as ready for review July 27, 2023 13:11

Merge remote-tracking branch 'upstream/main' into VACMS-13852-Create-…

8e9de71

…Entity-Event-Subscriber

va-cms-bot temporarily deployed to Tugboat July 27, 2023 14:35 Destroyed

swirtSJW previously approved these changes Jul 27, 2023

View reviewed changes

ndouglas added 2 commits July 27, 2023 17:17

Merge remote-tracking branch 'upstream/main' into VACMS-13852-Create-…

e4c218b

…Entity-Event-Subscriber

'exception occurred' => 'Not Applicable'

da113c1

ndouglas dismissed swirtSJW’s stale review via da113c1 July 27, 2023 21:19

va-cms-bot temporarily deployed to Tugboat July 27, 2023 21:19 Destroyed

swirtSJW approved these changes Jul 27, 2023

View reviewed changes

ndouglas merged commit 058a9ce into department-of-veterans-affairs:main Jul 27, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VACMS-13852: Creates EntityEvent Subscriber. #14337

VACMS-13852: Creates EntityEvent Subscriber. #14337

ndouglas commented Jul 12, 2023 •

edited by swirtSJW

Loading

ndouglas Jul 26, 2023

ndouglas Jul 26, 2023

ndouglas Jul 26, 2023

ndouglas Jul 26, 2023

swirtSJW Jul 26, 2023

ndouglas Jul 26, 2023

swirtSJW Jul 26, 2023

ndouglas Jul 26, 2023

ndouglas Jul 26, 2023

ndouglas Jul 26, 2023

swirtSJW Jul 26, 2023

ndouglas Jul 26, 2023

ndouglas Jul 26, 2023

swirtSJW left a comment

swirtSJW Jul 26, 2023

swirtSJW Jul 26, 2023

ndouglas Jul 27, 2023

swirtSJW Jul 26, 2023

swirtSJW Jul 26, 2023

swirtSJW Jul 26, 2023

swirtSJW commented Jul 27, 2023

swirtSJW commented Jul 27, 2023

swirtSJW commented Jul 27, 2023

ndouglas commented Jul 27, 2023 •

edited

Loading

swirtSJW commented Jul 27, 2023

swirtSJW left a comment

		$environment = $settings->get('va_gov_environment')['environment'];
		$environment = $settings->get('va_gov_environment')['environment'] ?? '(no value provided)';

VACMS-13852: Creates EntityEvent Subscriber. #14337

VACMS-13852: Creates EntityEvent Subscriber. #14337

Conversation

ndouglas commented Jul 12, 2023 • edited by swirtSJW Loading

Description

QA Steps (more probably forthcoming)

Followup Items

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

swirtSJW left a 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

swirtSJW commented Jul 27, 2023

swirtSJW commented Jul 27, 2023

swirtSJW commented Jul 27, 2023

ndouglas commented Jul 27, 2023 • edited Loading

swirtSJW commented Jul 27, 2023

swirtSJW left a comment

Choose a reason for hiding this comment

ndouglas commented Jul 12, 2023 •

edited by swirtSJW

Loading

ndouglas commented Jul 27, 2023 •

edited

Loading