#4353 Plugin for auto-generated sitemaps #4413

dancastellon · 2018-07-09T22:19:15Z

Resolves #4353
Impact: minor
Type: feature

Issue

Reaction currently has no sitemap.xml - a file that allows search engines visibility into a website's pages. Without it, search engines must manually crawl and find pages through links.

Solution

A sitemap generator plugin that creates and stores XML for a sitemap index, as well as sitemaps for tag pages, PDPs, and arbitrary URLs that can be added via an event hook.

There is a recurring job that runs every 24 hours (can be changed) that generates the sitemaps. There is also a button to manually trigger a refresh, at Dashboard -> Shop -> Options - along with a notification that appears when it's completed.

GraphQL TBD (separate issue)

Breaking changes

None

Testing

Reset the db (translations updated) and use reaction-data-loader to load the dev dataset
Create some products and tags - some archived/non-visible, some visible/published
Login as admin and visit Dashboard -> Shop Settings -> Options
Click "Refresh sitemap"
Confirm you get a notification (bell icon) as well as a Toast alert.
See Sitemaps collection and confirm correct XML by handle: sitemap.xml, sitemap-pages-1.xml, sitemap-tags-1.xml, sitemap-products-1.xml
Visit /sitemap.xml and confirm it loads and displays sitemap index XML
Visit each of the individual sitemap URLs and confirm they load/display valid XML
In the DB, confirm you see a Sitemaps collection with a document for each sitemap.
Confirm you see "BASE_URL" in place of any actual URLs in DB sitemaps
Without updating a tag or product, click the sitemap refresh button again and confirm that the sitemaps for tags and products did not regenerate, but that the sitemap index (sitemap.xml) and the one for pages, has regenerated.
Update a tag's title (I did it by editing the main tag nav), refresh sitemap, and confirm the tag sitemaps did regenerate, but not product sitemaps
Update a product field (such as the title), refresh sitemap, and confirm product sitemaps did regenerate
Run against retailer and miderprise databases - confirm job runs in background (doesn't stop app UI from functioning), and creates the correct number of sitemaps (1k URLs per sitemap max).
At Shop -> Options, change the "Sitemap refresh period" value to something really short, like "every 30 seconds"
Restart the server for the job schedule to update (this appears to be the same as other refresh period settings, like open exchange rates)
Confirm the job runs as scheduled.

brent-hoover · 2018-07-10T06:02:38Z

@dancastellon You've got some lint problems here. Make sure it passes npm run lint

brent-hoover

This is looking pretty good. Couple of questions and that's all.

brent-hoover · 2018-07-10T06:04:37Z

imports/plugins/included/sitemap-generator/server/lib/generateSitemaps.js

+import { Meteor } from "meteor/meteor";
+import { check, Match } from "meteor/check";
+import Hooks from "@reactioncommerce/hooks";
+import Reaction from "/imports/plugins/core/core/server/Reaction";


This should be imported from /server/api

@zenweasel Release 1.14 changes that. It looks like all references to /server/api have been moved to /imports/plugins/core/core/server/Reaction.

as @dancastellon mentioned, there is no more /server/api in release 1.14

brent-hoover · 2018-07-10T06:08:13Z

imports/plugins/included/sitemap-generator/server/lib/generateSitemaps.js

+        <lastmod>LAST_MOD</lastmod>
+        <changefreq>daily</changefreq>
+        <priority>0.8</priority>
+      </url>`.replace("URL", url);


Are we replacing LAST_MOD somewhere?

@zenweasel Yes, see getSitemapXML below.

brent-hoover · 2018-07-10T06:22:10Z

imports/plugins/included/sitemap-generator/server/lib/generateSitemaps.js

+ * @param {Array} [shopIds] - _id of shops to generate sitemaps for. Defaults to primary shop _id
+ * @param {Number} [urlsPerSitemap] - Max # of URLs per sitemap
+ */
+export default function generateSitemaps (shopIds = [], urlsPerSitemap = DEFAULT_URLS_PER_SITEMAP) {


These methods could definitely use some tests

…or manual regeneration method

dancastellon · 2018-07-13T15:27:00Z

@zenweasel, @spencern This is ready for a review, but let's not merge yet - I need to test against the miderprise dataset, which Grigg will be setting up on a hosted DB, likely next week.

dancastellon · 2018-07-17T19:01:41Z

@zenweasel Today I was able to test the sitemap generator with the miderprise dataset. It took roughly 3.5 minutes, but the app was useable while it was generating. I believe this one's ready for review.

brent-hoover

Just some style things to take care of. Have not tested yet.

brent-hoover · 2018-07-17T21:25:40Z

imports/collections/schemas/registry.js

    type: String,
-    label: "Open Exchange Rates App Id"
+    label: "Open Exchange Rates App Id",
+    optional: true


Can you explain why you needed to make this change since it doesn't appear related to this PR?

@zenweasel Without making this field optional I'm unable to save the shop options form, unless I add an App ID for Open Exchange Rates. Shop options is where I added the sitemap refresh setting & regenerate button. I just noticed that on a fresh install, I also had to make settings.cart in the same schema optional. Does it make sense to have the sitemap settings be in a separate form? Or perhaps somewhere else, like the Shop -> General form?

brent-hoover · 2018-07-17T21:28:05Z

imports/plugins/included/sitemap-generator/client/components/GenerateSitemapsButton.js

@@ -0,0 +1,32 @@
+import { Meteor } from "meteor/meteor";
+import React, { Component } from "react";


React import should always go before Meteor import: https://docs.reactioncommerce.com/docs/styleguide#syntax-and-style-conventions

brent-hoover · 2018-07-17T22:21:16Z

imports/plugins/included/sitemap-generator/server/lib/getSitemapXml.app-test.js

@@ -0,0 +1,29 @@
+import Random from "@reactioncommerce/random";


Filename should use hyphens rather than camel-casing

Thanks @zenweasel - updated all filenames to be hyphenated

brent-hoover · 2018-07-17T22:23:19Z

imports/plugins/included/sitemap-generator/server/methods/index.js

@@ -0,0 +1,23 @@
+import { Meteor } from "meteor/meteor";


Is this a pattern you see elsewhere as I am not a big fan of putting much besides imports into index.js

@zenweasel I updated this to follow the core plugin's pattern for defining methods. Method itself in a separate file, that is simply imported in methods/index.js and exported.

brent-hoover · 2018-07-17T22:26:26Z

@dancastellon Also, you still have lint errors

brent-hoover · 2018-07-17T22:30:09Z

@dancastellon Following up on the notes from the call, if I click on "Refresh Sitemaps Now" I don't see any visual feedback that anything has started, much less completed in either the server console or the UI. I should get at least some sort of "started regenerating sitemap" message or something.

brent-hoover · 2018-07-17T22:33:57Z

Shouldn't I be seeing these value substituted in the actual XML?

…reaction into feat-4353-dancastellon-sitemap-generator

dancastellon · 2018-07-25T15:34:14Z

Thanks @rymorgan, I've made the changes you suggested, see below:

dancastellon · 2018-07-25T15:36:38Z

@rymorgan Also, the notification. Let me know if I should change the text:

rymorgan · 2018-07-25T20:02:42Z

@dancastellon I think the notification should say 'Sitemap refresh is complete' to match the language in the toast

Other than that it LGTM. The toast alert was coming up for me but could very well be something wrong with my local reaction instance so as long as @zenweasel can confirm that is working then I'm good with this.

dancastellon · 2018-07-25T20:13:30Z

@rymorgan Done, thanks. I have seen times previously where the toast doesn't show up sometimes. @zenweasel Let me know if you still don't see the toast. Might be a core bug.

brent-hoover · 2018-07-27T05:56:42Z

If I put in an invalid interval I don't get any validation but it throws this error in the server console:
No valid available later.js times in schedule after Fri Jul 27 2018 13:55:06 GMT+0800 (+08)

I don't know that we do a good job validating any of those values in that screen. Maybe it might make sense to just do a dropdown there? Every 24 hours/Every 12 hours/Every 1 hour? Or maybe the later library gives you some way of validating it.

brent-hoover · 2018-07-27T06:13:43Z

When I click on "Refresh sitemap" I get the translation key instead of the message.

brent-hoover · 2018-07-27T06:17:26Z

This is a nitpick probably but: If I click on "View All" for notifications, each of the notifications is a link but that link just takes you to the home page. Maybe it should take you to the XML file instead?

brent-hoover · 2018-07-27T06:36:58Z

@dancastellon Found a few little things to fix but it's getting pretty close

Issues addressed

brent-hoover

A few issues left to address

…reaction into feat-4353-dancastellon-sitemap-generator

…ts lib

dancastellon · 2018-07-27T17:36:26Z

@zenweasel I've made the changes you suggested. Tagging @rymorgan as I've switched out some form fields with their new Reaction Components counterparts

Used a select box to let the admin choose from 3 options - every 24 hours, 12 hours, 1 hour
Linked the notification to sitemap.xml
Added default values to translations so that Toast alert never shows the i18nkey

rymorgan · 2018-07-27T22:50:56Z

@dancastellon Is this still under shops/options? I'm not seeing it anymore on your branch (might be user error ?).

From your screenshot though I think this approach is good. And every place where we convert to components in our component library is a win. 👍

dancastellon · 2018-07-30T13:44:22Z

@rymorgan Yes, it's still under Shop -> Options. I did rename the React component behind the sitemap settings, so maybe try a hard refresh? Are you seeing any browser console errors?

rymorgan · 2018-07-30T18:01:56Z

Ok, I got it working. From a UI/UX perspective, it looks good to me 👍

brent-hoover

Looks good. 👍

(feat): generate sitemaps on demand and store in collection

c7a0d51

dancastellon requested a review from brent-hoover July 9, 2018 22:19

brent-hoover requested changes Jul 10, 2018

View reviewed changes

brent-hoover reviewed Jul 10, 2018

View reviewed changes

dancastellon added 3 commits July 10, 2018 09:16

(style): correct linting issues

f671b6c

(feat): Routes for auto-generated sitemap files

59226ad

(feat): Add compound index for Sitemaps queries

db2d035

dancastellon force-pushed the feat-4353-dancastellon-sitemap-generator branch from 3afad21 to db2d035 Compare July 10, 2018 15:47

dancastellon changed the title ~~WIP: Feat 4353 dancastellon sitemap generator~~ [WIP]: #4353 Plugin for auto-generated sitemaps Jul 11, 2018

dancastellon added 10 commits July 11, 2018 09:54

(refactor): Cleanup generateSitemaps function

56de04e

(style): corrent linting issues

b423901

(feat): sitemap refresh period setting + translations

a7a11c2

(refactor): rename sitemap route handler to middleware

84fae7f

(feat): recurring job that regenerates sitemaps

4ea11c8

(style): corrent linting issues

6612677

(perf): limit default # URLs per sitemap to 1k + use job-collection f…

720b3bd

…or manual regeneration method

(test): tests for generateSitemaps function

28485ce

(test): additional tests for generateSitemaps

0e56dbf

(test): app test for getSitemapXML

a7b7d4a

dancastellon requested a review from brent-hoover July 13, 2018 15:25

dancastellon requested a review from spencern July 13, 2018 15:27

spencern removed their request for review July 13, 2018 16:42

dancastellon changed the title ~~[WIP]: #4353 Plugin for auto-generated sitemaps~~ #4353 Plugin for auto-generated sitemaps Jul 17, 2018

brent-hoover previously requested changes Jul 17, 2018

View reviewed changes

dancastellon added 3 commits July 24, 2018 20:36

Merge branch 'release-1.14.0' of https://github.com/reactioncommerce/…

6c88323

…reaction into feat-4353-dancastellon-sitemap-generator

(fix) update sitemap generation translations

7b55ac4

(feat): use notifications UI when manually refreshing sitemap

20adb5c

(style): correct linting issues

db0f013

(fix): change sitemap refresh notification text

743aea3

brent-hoover requested changes Jul 27, 2018

View reviewed changes

dancastellon added 4 commits July 27, 2018 09:59

Merge branch 'release-1.14.0' of https://github.com/reactioncommerce/…

a9595b4

…reaction into feat-4353-dancastellon-sitemap-generator

(fix): provide default values for toast alerts

7abfef3

(feat): sitemap refresh notification brings admin to sitemap.xml

8d5fe24

(feat): select box for sitemap refresh period + use Reaction componen…

1c51a91

…ts lib

dancastellon requested a review from brent-hoover July 27, 2018 17:37

brent-hoover approved these changes Jul 31, 2018

View reviewed changes

aldeed changed the base branch from release-1.14.0 to release-1.15.0 August 2, 2018 03:12

aldeed merged commit c8d2400 into release-1.15.0 Aug 16, 2018

aldeed deleted the feat-4353-dancastellon-sitemap-generator branch August 16, 2018 23:02

spencern mentioned this pull request Aug 22, 2018

Release 1.15.0 #4549

Merged

brent-hoover mentioned this pull request Nov 8, 2018

Implement Google sitemap functionality reactioncommerce/example-storefront#422

Closed

		@@ -0,0 +1,32 @@
		import { Meteor } from "meteor/meteor";
		import React, { Component } from "react";

		@@ -0,0 +1,29 @@
		import Random from "@reactioncommerce/random";

#4353 Plugin for auto-generated sitemaps #4413

#4353 Plugin for auto-generated sitemaps #4413

Uh oh!

Conversation

dancastellon commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Solution

Breaking changes

Testing

Uh oh!

brent-hoover commented Jul 10, 2018

Uh oh!

brent-hoover left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dancastellon Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dancastellon commented Jul 13, 2018

Uh oh!

dancastellon commented Jul 17, 2018

Uh oh!

brent-hoover left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brent-hoover commented Jul 17, 2018

Uh oh!

brent-hoover commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brent-hoover commented Jul 17, 2018

Uh oh!

dancastellon commented Jul 25, 2018

Uh oh!

dancastellon commented Jul 25, 2018

Uh oh!

rymorgan commented Jul 25, 2018

Uh oh!

dancastellon commented Jul 25, 2018

Uh oh!

brent-hoover commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brent-hoover commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brent-hoover commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brent-hoover commented Jul 27, 2018

Uh oh!

brent-hoover left a comment

Choose a reason for hiding this comment

Uh oh!

dancastellon commented Jul 9, 2018 •

edited

Loading

dancastellon Jul 11, 2018 •

edited

Loading

brent-hoover commented Jul 17, 2018 •

edited

Loading

brent-hoover commented Jul 27, 2018 •

edited

Loading

brent-hoover commented Jul 27, 2018 •

edited

Loading

brent-hoover commented Jul 27, 2018 •

edited

Loading

rymorgan commented Jul 27, 2018 •

edited

Loading

dancastellon commented Jul 30, 2018 •

edited

Loading