Facility banner endpoints needs additional data to fully render

[timcosgrove]

Requirements

'Facility banners' (full_width_banner_alert nodes) can be returned by the Banner Alerts endpoint, but the data returned is missing some needed information which is required to fully render the banner.

Acceptance criteria

Preview Give feedback

1. The Banner Alerts API endpoint is updated so that Facility banners include situation update information
2. That situation update information populates appropriately when the banner is included in Next Build

Background & implementation details

The endpoint is reached at the following URL structure:

/jsonapi/banner-alerts?item-path=/housing-assistance/

where the item-path value should contain both leading and trailing slashes.

That endpoint is largely defined in this class: https://github.com/department-of-veterans-affairs/va.gov-cms/blob/main/docroot/modules/custom/va_gov_api/src/Resources/BannerAlerts.php

The full_width_banner_alert entity contains a field field_situation_updates that is a Paragraph field and therefore is not fully included, since entity references are not expanded normally.

We can't use the normal ?include=field_situation_update mechanism in JSON:API requests because multiple resource types are returned. jsonapi module (core) DOES contain mechanisms for resolving includes and we might want to take advantage of that here.

[alexfinnarn]

I'm going to start with the "cheap" solution since that might be good enough. It isn't technically sexy, but it certainly works and requires no code changes...well, a small code change it turns out.

It is true that trying to ?include= on a field with multiple resource types in a collection fails when one returned resource does not contain the field. However, in this case, we know that there are three resource types in play and only one resource type currently has the field_situation_updates field on it. So, I thought, why not just add the field to the other two content types, disable the field on forms, and call it a day?

To test:

1. Find a full_width_banner_alert node and trace it to an active page. I used https://www.va.gov/north-florida-health-care/operating-status/ since that was active, at least yesterday it was. I was able to add a situation update and re-publish it here: /node/64695/edit
2. Go to /jsonapi/banner-alerts?item-path=/north-florida-health-care/operating-status/&include=field_situation_updates and confirm that the included key has data in it that matches the situation update you just added.
3. Re-use the field_situation_updates field on the promo_banner and banner content type configuration. Do not alter the form display, which will be explained later. You can disable the field on the entity view display settings.
4. Edit current nodes for promo_banner and banner and add /north-florida-health-care/operating-status/ to their list of paths. Make sure the nodes are published.
5. Go to /jsonapi/banner-alerts?item-path=/north-florida-health-care/operating-status/&include=field_situation_updates and confirm that the included key has data in it that matches the situation update you added in step 1 as well as the two other banners.

I thought this would be a breeze, but after adding the situation updates field to the banner content type and making the field disabled in form settings, the revision log field "disappeared". So, when I clicked to save nothing happened with no error messages.

Turns out some code was moving the revision log text area to the right-hand sidebar where "Section Settings" is. I can tell in browser dev tools that the error CSS class was being applied but it didn't show up to me...so it took me a minute to figure out that was the issue.

I was able to find code already altering the banner forms here: https://github.com/department-of-veterans-affairs/va.gov-cms/blob/main/docroot/modules/custom/va_gov_banner/src/EventSubscriber/EntityEventSubscriber.php#L40 And it was simple to unset($form['field_situation_updates']); with no noticeable regressions in saving the banner and promo banner nodes.

Proposed changes:

1. Re-use field_situation_updates on the banner and promo_banner content types.
2. Add unset($form['field_situation_updates']); to hide that field on forms.

Questions:

1. Should all banner types have the situation update field? Seems like a useful field to have for any type of banner and can be ignored and left empty. I assume some theming work would be necessary but it might make sense to explore or re-visit usage and intent of the situation update field.

Other attempts:

1. I tried adding data in BannerAlerts::collectFullWidthBannerAlertData but it never stuck. I think you might have to alter how those entities are loaded and/or look more into refactoring that class to allow for greater manipulation of data. I can look into altering data in this way, if it makes sense.

[alexfinnarn]

Since attaching field_situation_updates to the other banner types wasn't received highly, I spent some time trying other methods.

Attempts:

• Adding the data in BannerAlerts::collectFullWidthBannerAlertData. I reported on this in my last comment, but I tried a few more ways of inserting the data in that method.
• Adding a computed field dynamically. This never seemed to stick, and I think the schema stored in config is used for JSON:API resources. You can probably extend a class and override a method just for the full width banner entity, but I didn't look into it that deeply.
• Adding a computed field to the schema and config export. I started to do this, and I think it could work...I was getting database errors trying to not store any value...but I stopped since it's the same type tech debt as adding fields to other banners, except it's worse with code to remove later.
• Creating a field instance and jamming it into the JSON:API response in the BannerAlerts class. This may have worked but it seemed wrong, and I figured the data would be stripped from the response like in other attempts.

So, I decided to try the nuclear option of modifying the response at the last minute, which always works. In this case, I don't think the code is much better or worse than modifying how JSON:API loads resources, normalizes, and then serializes them for responses. Plus, the data can be in whatever format is requested since the response doesn't go through any validation afterward. Beautiful ;P

Something like:

  public function onKernelResponse(ResponseEvent $event): void {
    // Only modify the "/jsonapi/banner-alerts" route.
    $request = $event->getRequest();
    if ($request->get('_route') !== 'va_gov_api.banner_alerts') {
      return;
    }

    // Get paragraph data and put into the includes section...
    $response = $event->getResponse();
    $response->setContent($this->setParagraphData($response->getContent()));
}

The main caveat is updating the OpenAPI schema generator for the VA.gov JSON:API to reflect how the data is added for discoverability. I think this can be done, but I paused to make this comment and will go look into that afterward.

An alternative is to split up the queries so that the banner and promo_banner can be one query and the full_width_banner_alert another using the include query parameter. Banner and Promo Banner have 20 total entities whereas Full Width Banner Alert has 931 entities.

[alexfinnarn]

I did try looking at what the computed breadcrumbs module does for a computed field on all node types. While hook_entity_base_field_info(EntityTypeInterface $entity_type) can add fields to entity types that are respected by JSON:API nomalization, that adds the field to the whole entity type, and there is no way to filter by bundle.

I did find another hook that defines bundle field definitions, but it doesn't have an event subscriber, which was a problem before...I'm fine with adding hooks that have no deprecation notices.

Three solutions so far:

1. Attach field_situation_updates to the banner and promo_banner bundles so that ?include=field_situation_updates works as expected.

• Pros: Only modifies a couple of configuration files. The resource collection functions like standard JSON:API response.
• Cons: Situation updates aren't needed on the other two content types at this time so the fields are unnecessary in that regard.

2. Use hook_entity_bundle_field_info() to dynamically add a computed field.

• Pros: The most structured solution by adding to the JSON:API schema and response at the same time.
• Cons: The field data already exists as a relationship, so this is duplicating data. Normally, the data would be in the "includes" section of the response.

3. Use kernelResponse event to modify the response.

• Pros: The most configurable option and can place data in "includes" mimicking default JSON:API behavior.
• Cons: The code relies on a certain shape for the response and would break if the BannerAlerts class' code changes.

I added a draft PR to #16883 . However, there appears to be a graphQL check error I need to review. Not sure if it's because of the added field definition...

It's beyond the scope of this issue, but I'm not sure why the banner alerts endpoint has to conform to the JSON:API spec since the collection is not a standard collection. It's also not in the JSON:API schema that I could see. With a custom REST endpoint, the data could be shaped however is wanted.

[alexfinnarn]

After commenting out the hook_entity_bundle_field_info() the "va/tests/content-build-gql" check passed so the additional field might affect the GraphQL queries. However, Cypress tests are failing whether that computed field solution is commented out or not. I'm not sure how to look at the failures, but maybe adding an integration test would shed light on the failures.

[tjheffner]

I was initially thinking maybe it would happen in this loop in BannerAlerts.php:

    // Add the banners to the response.
    foreach ($facility_banners as $entity) {
      $this->addEntityToResponse($resource_type, $entity);
    }

If it isn't feasible to add the paragraph data we want as a field on the entity for our particular endpoint there, I think I found something that may help.

This issue seems to describe a similar problem as ours (?include= breaks if all entity types do not have the field being included). Testing the patch in this comment locally with some dummy banner data I created (One facility banner with a situation update added, one promo banner)... things appeared to resolve correctly.

It may be worth pulling on that thread a little farther... it seems potentially simpler than some of the other approaches. I don't want to add "unused" hidden fields to content types that don't need them. Duplicating data on our endpoint doesn't bother me (as you note, it is a custom REST endpoint, we can shape it however we want) as long as it isn't duplicating the data on the actual JSON:API endpoint for a banner node (ie jsonapi/node/full_width_banner_alert/ vs the custom jsonapi/banner-alerts?item-path=)

[alexfinnarn]

I checked out that patch to test, but now I am seeing some weird behavior locally that I've replicated. Even with no code changes to the main branch, I still get different behavior than what I saw on the branch in the PR.

1. Pull in the latest on main and ddev composer install to make sure dependencies are up-to-date.
2. Run ddev pull va --skip-files to set up the latest database.
3. Clear caches for good measure.
4. Edit https://va-gov-cms.ddev.site/node/64695/edit to be published and have a situation update included.
5. Go here https://va-gov-cms.ddev.site/jsonapi/banner-alerts?item-path=/north-florida-health-care/operating-status?include=field_situation_updates but don't see an includes section.

I'll try checking out the branch I made a PR on, but I'm not sure what's going on with my local right now...

As far as the patch issue, I made a comment on that about the JSON:API spec and how that type of change would affect Drupal's interpretation of it. I think most devs want more information included, but I could see the issue linger for a years...maybe not. However, adding a patch to how relationships work, in general, just for this issue seems riskier than other approaches.

What about making the banner alerts code run at an endpoint like /api/v1/banner-alerts?item-path= and forgo JSON:API alltogher? My interpretation of the problem here is the classic want of a UI needing certain data and wanting that returned in one response. The current implementation of the JSON:API spec is getting in the way and people could argue for years over resolving the issue. So, why not simplify things a bit with a different endpoint? Otherwise, it seems like we are jamming things into a JSON:API collection where a work-around is always needed.

It should be rather trivial to take what's in the process method and attach that to a controller that returns a response. Then, adding the paragraphs in the loop you referenced would work, be easy to understand for future devs, and make it rather easy to modify in case that is needed in the future. That would be my ultimate preference, but I kind of just want to close this issue and move on...so whatever works is fine by me.

[tjheffner]

i am not tied to the /jsonapi/banner-alerts route for our endpoint. i think it would be clearer to place it under something else, considering it is a custom endpoint. I think we placed it under /jsonapi/ to begin with because the rest of the endpoints we target are also under there (and other than this one paragraph type, it has mostly "just worked"), but nothing says it has to be there.

My interpretation of the problem here is the classic want of a UI needing certain data and wanting that returned in one response.

this is exactly it, yes.

[alexfinnarn]

Okay, I added a custom controller to the example code in the attached draft PR so that a request to "api/v1/banner-alerts?path="

    // Add the banners to the response.
    $full_width_banner_alert_data = [];
    foreach ($facility_banners as $entity) {
      $full_width_banner_alert_data[] = $this->serializer->normalize($entity);

      // Override field_situation_updates with referenced paragraph data.
      $situation_updates = $entity->get('field_situation_updates')->referencedEntities();
      $situation_update_data = [];
      foreach ($situation_updates as $situation_update) {
        $situation_update_data[] = $this->serializer->normalize($situation_update);
      }
      $full_width_banner_alert_data['field_situation_updates'] = $situation_update_data;
    }

includes the paragraph data in the response. I mainly copy/pasted the code in BannerAlerts and used the default normalization of the serializer service. I don't have a preference for the particular route used and the code can be cleaned up further, but is this a good solution to continue with?

And what is the preferred shape of data the frontend wants to receive? It might be possible to normalize like JSON:API and then override the field, but the default output for entity normalization isn't similar to the current banner alerts response.

[tjheffner]

I am comfortable with this approach.