USAGOV-1870 - Published Page Report to handle taxonomy terms #1860
Conversation
There was a problem hiding this comment.
I caught a subtle bug here -- see below.
Btw, it might not have been clear -- we're referring back to the contents of the file not because we think the same page might be rendered twice, but because Tome doesn't always render all the pages. It's great that Tome can just update the pages that need an update, but we need the report to contain an entry for every page. So we build on the existing CSV file.
| @@ -118,7 +119,7 @@ public function modifyHtml(ModifyHtmlEvent $event) { | |||
| $nodeIDElement = array_search("Page ID", $csv[0]); | |||
| $languageElement = array_search("Taxonomy Level 1", $csv[0]); | |||
| foreach ($csv as $key => $line) { | |||
| if ($line[$nodeIDElement] == $decoded["nodeID"]) { | |||
| if (!empty($line[$nodeIDElement]) && $line[$nodeIDElement] == $decoded["nodeID"]) { | |||
There was a problem hiding this comment.
We need to repeat this test for taxonomyIDs, or otherwise expand this clause. (And maybe it would be good to change the name of that variable from $nodeIDElement to $pageIDElement or something.)
What's missing is a test for the case where there's a taxonomyID, say "60," and there's already an entry in the PPR for it -- that would have "t_60" in the Page ID position.
I was able to reproduce the problem by first, having a wizard taxonomy term on my local and setting its path alias (because otherwise I think it gets omitted, due to the /taxonomy/term path). Run tome and get the published_pages.csv file. The edit and re-save the wizard term, run tome again (happily, it goes pretty fast when you've just updated one term like that). Now I have two entries for that term in my published_pages.csv.
There was a problem hiding this comment.
oh! I see this now - in fact, every tome-run was adding a new taxonomy-term to a new line..
I have updated, and this issue is now fixed with the latest commit.
There was a problem hiding this comment.
Oh, hmm, I may not be as familiar with our workflow here. I previously clicked the "Resolve conversation button", but now I am assuming that perhaps that is something that Amy does and I instead am supposed to request a re-review? Let me know if I am doing this right.
There was a problem hiding this comment.
You did the right thing, and I just got bogged down and didn't have time to do the re-review until today.
…e added to a new line to the PPR on every tome-run
|
@DaleMHFrey just a heads up, there's a silly lint error on this PR and that's why I haven't reviewed it again; could you clear that up please? |
|
There was a problem hiding this comment.
I realize it's been awhile since I reviewed this and since you made changes, and there might have been some drift in the dev branch.
I'm getting big batches of warnings from line 128 (to which I attached comments), and then this warning:
[warning] Attempt to read property "nodeValue" on null PublishedPagesSubscriber.php:143
And this error:
[drupal "base_url":"http://localhost","severity":"ERROR","type":"php","date":"2024-11-07T12:34:45","uid":"0","request_uri":"http://localhost/where-report-scams/identity-scam","refer":"","ip":"127.0.0.1","link":"","message":"Error: Call to a member function toUrl() on null in
Drupal\usagov_ssg_postprocessing\EventSubscriber\PublishedPagesSubscriber->modifyHtml() (line 204 of
/var/www/web/modules/custom/usagov_ssg_postprocessing/src/EventSubscriber/PublishedPagesSubscriber.php) #0
[internal function]:
... and then "drush terminated abnormally."
I do see some entries in the report with Page IDs like t_67. But my html directory is short by 91 pages (I had a previous run saved off to the side for comparison). I didn't try to get into what's causing these problems. I will if you find that you don't see the same on your system.
| @@ -118,7 +125,7 @@ public function modifyHtml(ModifyHtmlEvent $event) { | |||
| $nodeIDElement = array_search("Page ID", $csv[0]); | |||
| $languageElement = array_search("Taxonomy Level 1", $csv[0]); | |||
| foreach ($csv as $key => $line) { | |||
| if ($line[$nodeIDElement] == $decoded["nodeID"]) { | |||
| if (!empty($line[$nodeIDElement]) && ($line[$nodeIDElement] == $decoded["nodeID"] || $line[$nodeIDElement] == 't_' . $decoded["taxonomyID"])) { | |||
There was a problem hiding this comment.
I am getting a whole lot of warnings about this line. Big batches of:
[warning] Undefined array key "taxonomyID" PublishedPagesSubscriber.php:128
... and
[warning] Undefined array key "nodeID" PublishedPagesSubscriber.php:128
There was a problem hiding this comment.
Ah I was able to reproduce and re-run. I believe I have fixed the issue, though, I am seeing one and only one potential "duplicate" in the report for the home-page - one home page in English and one in Spanish. Can you refresh my memory if this is expected?
I have updated this branch.
There was a problem hiding this comment.
I ran tome on the dev branch and then on this one and compared the resulting reports, and I found some weird differences -- entries with matching titles that seemed they shouldn't be wizard steps, but with /taxonomy/term/ paths (which also shouldn't be showing up I think). I figured that probably meant that node IDs were being treated as taxonomy IDs (or vice versa) somewhere, and I think I see where!
I changed the line I've called out here locally and the diffs look pretty good now, so I think that's all that needs to change!
| $nodeEntity = \Drupal::entityTypeManager()->getStorage('node')->load($nid); | ||
| $url = $nodeEntity->toUrl()->toString(); | ||
| if (!empty($nid)) { | ||
| if (substr($nid, 0, 2) !== 't_') { |
There was a problem hiding this comment.
I believe this conditional is backward (should be === instead of !==)
USAGOV-1870 - Published Page Report to handle taxonomy terms
https://cm-jira.usa.gov/browse/USAGOV-1870
Description
Type of Changes
Testing Instructions
Run Tome, and expect "t_" to exist under the "Page ID" column for taxonomy-terms.
Change Requirements
Validation Steps
Security Review
Reviewer Reminders
Post PR Approval Instructions
Follow these steps as soon as you merge the new changes.
Review in Testand 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.