Usagov 1685 fix site banner

[YaritzaGarcia]

Jira Task

https://cm-jira.usa.gov/browse/USAGOV-1685

Description

I added a variable to Drupal's state to check if the banner is already placed or not. If that variable is FALSE and the banner is supposed to be visible, it runs the "drupal_flush_all_caches();" function.

I changed all the code for how we place a site banner but the interface and the process in which a content admin adds it remains the same. These were the things I did:

• Update the readme
• Remove the old code from usa_twig_vars.module
• Create a new module for the site_banner called usa_site_banner
• Remove the twig template that I was using to place the block

Type of Changes

• New Feature
• Bugfix
• Frontend (Twig, Sass, JS)
  • Add screenshot showing what it should look like
• Drupal Config (requires "drush cim")
• New Modules (requires rebuild)
• Documentation
• Infrastructure
  • CMS
  • WAF
  • Egress
  • Tools
• Other

Testing Instructions

1. Run bin/init and bin/drush cim
2. Create a Content Administrator account and log in using that account
3. Go to the Content blocks page localhost/admin/content/block
4. Click + Add content block.
5. Fill in the required fields for the Site Banner.
   • Make sure you select the correct language for the banner.
6. Find the checkbox labeled Place above header.
   • Check this box if you want the banner to appear in the header_top region.
   • Uncheck this box if you want to remove the banner from the header_top region.
7. Click Save to create or update the Site Banner block.
8. Visit the site to ensure the banner appears or is removed from the region.
9. Log out from the account and check that the banner is visible for anonymous users.
10. Unplaced the block and check it was removed from anonymous users as well.
11. Repeat these steps for both languages and also add multiple site_banners
12. Keep the banners placed and go to Docker Desktop and find the cms container
13. Click the 3 dots in the Actions column and click "Open in terminal"
14. In the terminal, type ./scripts/tome-run.sh and wait for tome to run
15. Once tome is done, go to http://127.0.0.1 and check that the banners that you placed are visible in all the pages.

Change Requirements

• Requires New Documentation (Link: {docs/SiteBanner.md})
• Requires New Config
• Requires New Content

Validation Steps

Security Review

• Adds/updates software (including a library or Drupal module)
• Communication with external service
• Changes permissions or workflow
• Requires SSPP updates

Reviewer Reminders

• Reviewed code changes
• Reviewed functionality
• Security review complete or not required

Post PR Approval Instructions

Follow these steps as soon as you merge the new changes.

1. Go to the USAGov Circle CI project.
2. Find the commit of this pull request.
3. Build and deploy the changes.
4. Update the Jira ticket by changing the ticket status to Review in Test and add a comment. State whether the change is already visible on cms-dev.usa.gov and beta-dev.usa.gov, or if the deployment is still in process.

[akf]

I have some disappointing news: I see a problem. I'm testing this a little differently than you described, but I think the principle is the same:

• I'm editing the blocks in a "normal" browser window
• I opened an incognito window to view the site as the "anonymous" user
• Each time I change a block and save in the first window, I go back to my "incognito" window and refresh the page to see what Ms. Anonymous sees.

At first I was really confused because it seemed as if it was working sometimes and not others, and I could tell the cache was getting refreshed during the "working" page loads because they'd be really slow. Well, it turns out that was probably because I was clicking around and looking at other pages.

I turned on my debugger to see when this code gets executed, and if the CMS user saves the block and then loads a page to look at the header, your new code is executed then ... because a logged-in user gets the full page re-built.

But if the CMS user just saves the page and doesn't leave the admin area, and the anonymous user views the page, we don't hit preprocess_region for this header block (assuming that an anonymous user has looked at it before, of course).

Have you tried to get some code like this to run when the block change is saved in the first place? (I would assume yes, and that it wasn't so easy.)

[YaritzaGarcia]

Thanks for reviewing! I'm going to take a look at this and see what's going on.

[YaritzaGarcia]

@akf Good news! I think I found a new way to add the site banner (I found it last week but I've been doing some testing)

[akf]

This is working beautifully, but again I have frustrating news. :-(

After testing this out as you describe, I decided to look at the Configuration Sync report at http://localhost/admin/config/development/configuration (log in as a Site Admin for that) and I saw that 2 blocks, named block.block.site_banner_32 and block.block.site_banner_34, would be deleted on next import. (I deleted one myself; it must have been number 33.)

So, in other words, they would be deleted the next time we deploy code!

So while this is cleaner in a lot of ways, your initial implementation, where someone needed to clear the cache or maybe just visit some pages while logged in to get the banner to show up, was preferable.

I still think there should be a way to get this to work properly, possibly combining ideas from the two approaches. The underlying issue here is that any changes made in the CMS, need to be saved as content in the database (not as exportable configuration). I forgot that blocks fall into the "exportable configuration" category!

I hate to bring nothing but disappointment, so I looked around a bit and I found this: https://www.reddit.com/r/drupal/comments/ut42uf/comment/i97egf6/

They're suggesting using config_ignore, which is a project that still appears to be going strong.

I would say check in with @mdranove but he's just started vacation (perhaps when he gets back?) because he has experience with config_split. It looks like they are entirely compatible: https://sitestudiodocs.acquia.com/8.0/user-guide/setup-config-ignore-and-config-split

[akf]

Actually I can explain this one! The checkbox is part of the block content, so the block will render it, and it will fall back on the default theme if nothing is supplied. This is a fine way to suppress it; it still shows up in the admin area (where we want to see it!) because the admin area has a different theme.

[cwacht]

I'm curious what all of this means for the Benefits Redirect Modal. Should I move forward with the implementation that mirrors how the gov banner is currently done on the prod? Or should I change it?

[akf]

Just closing the loop for @YaritzaGarcia, Chris and I DM'd about this. He'll move forward with the existing implementation and we'll expect to update both when we have a better approach (probably based on this newer work).

[YaritzaGarcia]

Thanks for the update and for reviewing! I will take a look.

[akf]

Hi @YaritzaGarcia! We're all super-busy and since this is not a show-stopper, I'm changing the PR to a "draft" (so someone doesn't merge it by accident) for now.

[omerida]

@akf and @YaritzaGarcia:

Chiming in with my 2cents on this task, and a lot of it is helping me get up to speed with the issue

Without this PR, there is a block type for adding banners using the USWDS banner component. This works for me locally and I haven't noticed any caching issues/incognito window issues. However, only admins can place blocks into the block layout due to Drupal permissions. Content team would like to add/update banners on their own.

This PR implements some hooks to place banner blocks when added/editted into the header region for users that can't use the block layout screen. It sort-of-works but there are caching issues/tome export issues.

A couple of things come to mind:

1. We've fixed many tome caching issues (by disabling some caches during export...), I wonder if that conflated things with this issue.
2. Since it seems to work for users with the proper permissions, could we extend the "Administer Blocks" permission the the Content Admin role? With the usual "With great power, comes great responsibility" reminder...though that screen does let you configure ALL the blocks, which is risky.
3. Are we sure that SELECT count(*) as c from block_content_field_data where changed > (UNIX_TIMESTAMP(now())-(1800)) is the right way to check for updated block content? Since block display also depends on configuration ... is there some other table(s) to look at? Is this why updating a node triggered the rebuild which included the banner?

Given blocks are a mix of content and config ... what if we created a node type for "Banners" These would be a separate implementation from the existing banners as blocks, that are editable by the content team and not mixed into config sync. There would be a few trade-offs, you'd have to re-implement the logic for displaying a banner only on certain pages (not terribly difficult, but not trivial), and you'd need some way to sort the display order (we could reuse the sorting view widget used for benefit category search weights). Without more work, these banner nodes would have to display in one spot within a region (then again, there could be a select menu for picking which region to show them in...) We'd also want to use the rabbit hole module so these nodes aren't treated as pages or show up in sitemap.xml

[akf]

• We've fixed many tome caching issues (by disabling some caches during export...), I wonder if that conflated things with this issue.

Maybe! This is definitely worth a try. My recollection is that just visiting a page that should have the banner as a logged-in user fixed the display for anonymous users (I am not sure whether that was for just that page), so the best test would include making sure to avoid clicking around after adding the banner.

At the same time, this behavior wasn't limited to Tome; you could also see it (that is, see that you could not see the banner) if you logged out and viewed CMS pages.

• Since it seems to work for users with the proper permissions, could we extend the "Administer Blocks" permission the the Content Admin role? With the usual "With great power, comes great responsibility" reminder...though that screen does let you configure ALL the blocks, which is risky.

We considered and rejected that. Way too many people have the Content Admin role right now.

• Are we sure that SELECT count(*) as c from block_content_field_data where changed > (UNIX_TIMESTAMP(now())-(1800)) is the right way to check for updated block content? Since block display also depends on configuration ... is there some other table(s) to look at? Is this why updating a node triggered the rebuild which included the banner?

I would say we're sure that's not sufficient for every change. For example, we already know that re-ordering menus does not trigger a rebuild. In this case, I think we'd need to test again to be sure. I am fairly certain that we had cases where rebuilds were triggered and yet the new block did not appear in the rendered site. (The rebuild might have been triggered because the editor had actually authored the block content as well as placing it, which probably would make that SELECT statement return non-zero, or it might have been because other content was updated.) But, I'm not confident that every page was actually re-rendered in that case!

... which kind of brings us back to the first question. Maybe tome-sync correctly detected that tome should run, but tome itself didn't detect that the pages would have changed.