Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

USAGOV-1937-clear-menu-link: #1923

Merged
merged 35 commits into from
Oct 25, 2024
Merged

USAGOV-1937-clear-menu-link: #1923

merged 35 commits into from
Oct 25, 2024

Conversation

omerida
Copy link
Contributor

@omerida omerida commented Sep 4, 2024

Jira Task

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

Description

Adds an event subscriber to clear menu link cache.

Needed when running a Tome export to build page navigation correctly when more than one path is exported by a process.

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 - includes patch, requires, at least, bin/drupal-update

Testing Instructions

Change Requirements

  • Requires New Documentation (Link: {})
  • Requires New Config
  • Requires New Content

Validation Steps

  • Make two exports for comparison
    • make a tome export of local using the dev branch, rename html/ to html-dev/ Also in the usagov_ssh_postprocessing module, renamed save the published pages CSV for the dev run to compare in the next step.
    • switch to this branch, make a tome export, rename to html-dev
  • Compare HTML(html-tidy config attached)
    • use html-tidy + find command to "normalize" the HTML output in each directory. Use the option to remove indentation. In each HTML folder run a command like:
      find . -name '*.html' -type f -print -exec tidy -config ../tidy-config.txt --warn-proprietary-attributes false -mq '{}' \;
    • use diff + grep to compare the contents of both directories to confirm the HTML is same between branches. Something like this where the first two grep -v commands ignore the cache busting query string drupla adds to CSS and JS files will give you a file to review in a text editor..:
      diff -brwd --strip-trailing-cr --exclude=_data -I js-view-dom-id --exclude=themes html-old/ html-new/ | grep -v sjpnwi | grep -v sjpja5 > html.diff
    • The diff file above should not have any lines starting with angle brackets.
  • Compare the published pages CSV
    • Should have the same number of lines
    • Sort the CSVs so they are in identical order, sorting by ID, full arl, and page title worked for me. Save sorted diffs
    • Diff the CSVs. Command line should work, a visual diff tool like WinMerge or Meld are useful too.
  • Compare number of redirects in each export directory to confirm its the same. Use this command inside each directory:
    grep -r 'http-equiv=\"refresh\"' . | wc -l

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.

@omerida omerida marked this pull request as draft September 4, 2024 17:59
@omerida omerida force-pushed the USAGOV-1773-clear-menu-link branch from 906a3d5 to 5f436d7 Compare September 6, 2024 14:49
@omerida omerida marked this pull request as ready for review September 6, 2024 14:50
…ink cache.

 Needed when running a Tome export to build page navigation correctly when more than one path is exported by a process.
@omerida omerida force-pushed the USAGOV-1773-clear-menu-link branch from 5f436d7 to 5fa0b27 Compare September 6, 2024 14:53
@akf akf self-requested a review September 9, 2024 22:30
omerida and others added 14 commits September 13, 2024 16:01
…out why higher path count saves redirects as the original page instead of a meta refresh.
…een page requests to fix how redirects are exported.
…GOV-1773-clear-menu-link' into USAGOV-1773-clear-menu-link
… Spanish homepage URL that can be treated as a redirect by Tome
…doesn't re-export pages as invoked paths when path_count is greater than 1.
@akf akf marked this pull request as draft September 27, 2024 01:18
@akf
Copy link
Member

akf commented Sep 27, 2024

@omerida I'm just reviewing things and I think this PR should be marked "draft" until you think it's good to go. So I did that.

@omerida omerida changed the title USAGOV-1773-clear-menu-link: USAGOV-1937-clear-menu-link: Sep 27, 2024
@akf
Copy link
Member

akf commented Oct 10, 2024

Yeah, I noticed that. We have bin/drupal-update which does that plus a bit more and I did that.

@omerida
Copy link
Contributor Author

omerida commented Oct 10, 2024

I found another cache that should be busted, for menu link content. I've updated the RequestPrepareSubscriber to reset it. I'm testing it now and will push the update shortly (waiting for one more tome run to finish)

@akf
Copy link
Member

akf commented Oct 10, 2024

Glad to hear it! I realized today that I probably, um, moved the html folder into an existing html-dev folder instead of renaming it as I'd intended, and that would account for a lot of the unexpected HTML changes was seeing. I realized I had nested folders yesterday when trying to figure out why the redirect count was so much higher in that folder, but I didn't make the connection with the other errors until today.
I have been using the mv command for well over 30 years, btw.

@omerida
Copy link
Contributor Author

omerida commented Oct 11, 2024

@akf Resetting the menu link content cache generates the same HTML but only if I run drush cr before I run the tome command. After the first run, a handful of pages will have the wrong datalayer, or navigation cards. I'll keep digging to see what other menu related cache might be involved.

@omerida
Copy link
Contributor Author

omerida commented Oct 11, 2024

I found another cache to reset and pushed an update to bust it. Multiple runs still eventually start showing the wrong breadcrumbs, menu, or navigation cards on occasion. I'm going to move this back to draft and keep testing

@omerida omerida marked this pull request as draft October 11, 2024 21:16
@omerida
Copy link
Contributor Author

omerida commented Oct 18, 2024

Found there's a cache related issue with menu_breacrumb module that has a patch. I've updated with the patch locally and am not running into random taxonomy datalayer / breadcrumb / nav card page weirdness when running tome. I've pushed that update here. On Monday I will do some more testing and (hopefully) can switch this away from draft.

@omerida omerida marked this pull request as ready for review October 21, 2024 20:28
@omerida
Copy link
Contributor Author

omerida commented Oct 21, 2024

OK, it looks like the 'menu' entity manager needs to have its cache cleared between requests via Tome's subscriber. I've tested exports and I'm not seeing the random datalayer or navigation card pages after sequential exports.

The process is 2x as fast with a path count of 10 versus 1. Going up to 20 didn't seem to make a big difference in my limited testing

With path count = 1:

real    13m 15.41s
user    38m 18.74s
sys     7m 12.38s

With path count = 10:

real    6m 10.38s
user    16m 12.63s
sys     1m 31.80s

I also spoke to Dale and his PR for wizard datalayer issues addresses the duplicated rows I was seeing in published pages.csv.

@omerida omerida marked this pull request as draft October 21, 2024 20:34
@omerida
Copy link
Contributor Author

omerida commented Oct 21, 2024

Just found that the export static asset pass also needs to track init_paths to prevent exporting the same path multiple times. I need re-roll the patch and test some more.

@omerida omerida marked this pull request as ready for review October 22, 2024 14:30
Copy link
Member

@akf akf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran one test on my local and it passed everything in your description except (sorry, there's an "except") ... there is a difference in the mobile menus on the agency index "letter" pages. It's just the agency index pages, both English and Spanish. But it does seem to be all of the agency index pages (21 English, 15 Spanish).

After doing the diff, I viewed my site as an anonymous user and I could see the incorrect menu, so I can give you a screen shot!

Here's what the text diff looks like for a single page.

diff -brwd --strip-trailing-cr --exclude=_data -I js-view-dom-id --exclude=themes tidy-html-dev/agency-index/b/index.html tidy-html-1773/agency-index/b/index.htm\
l                                                                                                                                                                 
17,43c17,43                                                                                                                                                       
---                                                                                                                                                               
171c171                                                                                                                                                           
< <li class="navigation__item">                                                                                                                                   
---                                                                                                                                                               
> <li class="navigation__item"><a href="/agency-index">A-Z index of U.S. government departments and agencies</a>                                                  
173,174d172                                                                                                                                                       
< <li class="navigation__item sibling"><a href="/buy-from-government">Buying from the U.S. government</a></li>                                                    
< <li class="navigation__item sibling"><a href="/facts-figures">U.S. facts and figures</a></li>                                                                   
176,181d173                                                                                                                                                       
< <li class="navigation__item sibling"><a href="/state-local-governments">State and local governments</a></li>                                                    
< <li class="navigation__item sibling"><a href="/branches-of-government">Branches of government</a></li>                                                          
< <li class="navigation__item sibling"><a href="/elected-officials">Elected officials</a></li>                                                                    
< <li class="navigation__item sibling"><a href="/laws-and-regulations">Federal laws and regulations</a></li>                                                      
< <li class="navigation__item sibling"><a href="/tribes">Indian tribes and resources for Native Americans</a></li>                                                
< <li class="navigation__item sibling"><a href="/courts">Federal, state, territory, county, and municipal courts</a></li>          

And here's a screen shot of a page showing what that looks like on a rendered (cms) page:
bad-a-z-mobile-menu

I am keenly aware that this might be hard to reproduce.

if ($changes) {
// Render it as HTML5:
$modifiedHtml = $html5->saveHTML($document);
$event->setHtml($modifiedHtml);
}
}

/**
* Prevent exporting paths Tome might discover after the collect paths event.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, specifically "prevent exporting node-based paths" ...

This is probably a separate can of worms, but I look at this simple function and wonder whether it might not be a better approach than what we currently have for excludeDirectories? That is, go ahead and let the all the paths be collected, and then mark the ones that match our exclusion rules as invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be somewhat faster if you don't have to load the node entities too, no? Though I bet those are cached already...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect it to be. I'm certain that when I wrote that, I didn't think there was a way to get at the path alias without loading the node entity.

@omerida
Copy link
Contributor Author

omerida commented Oct 23, 2024

I ran one test on my local and it passed everything in your description except (sorry, there's an "except") ... there is a difference in the mobile menus on the agency index "letter" pages. It's just the agency index pages, both English and Spanish. But it does seem to be all of the agency index pages (21 English, 15 Spanish).

We chatted about this in Jira

Copy link
Member

@akf akf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all checks out. The only differences I found were those darn agency index mobile menus, which we've agreed are a separate problem.

@akf akf merged commit 7a3701d into dev Oct 25, 2024
5 of 6 checks passed
@akf akf deleted the USAGOV-1773-clear-menu-link branch October 25, 2024 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants