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

Add sitemap generator #183

Merged
merged 16 commits into from
Dec 27, 2021
Merged

Conversation

hungmi
Copy link
Contributor

@hungmi hungmi commented Dec 24, 2021

This PR adds sitemap_generator and add some links mentioned in #40.

I also add /developers/new & /businesses/new since they are part of sign up process. I can remove them If that is not needed.

I have tested it locally and the sitemap.xml seems to be ok.
But I don't know how to write test for it 😅. Does anyone have any suggestion?

And the next step is set rake "-s sitemap:refresh"(according to the doc) on Heroku scheduler.

Pull request checklist

  • Your code contains tests relevant for code you modified
  • You have linted and tested the project with bin/check

Copy link
Owner

@joemasilotti joemasilotti left a comment

Choose a reason for hiding this comment

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

Thanks for kicking this off! I made a few small change requests, other than that it looks good.

We will have to host the sitemap somewhere else, like S3, as Heroku doesn't have persistent storage. But I can take care of that.

Gemfile Outdated Show resolved Hide resolved
config/sitemap.rb Outdated Show resolved Hide resolved
config/sitemap.rb Outdated Show resolved Hide resolved
@joemasilotti
Copy link
Owner

I'm fine with a single smoke test if it doesn't take too long. Running the command and verifying that the XML file is created without any raised errors should be good enough!

@hungmi
Copy link
Contributor Author

hungmi commented Dec 25, 2021

I have added a test to run rails sitemap:refresh:no_ping.
But I can't find a way to add -s when I invoke it, so I followed this open issue to prevent unneccessarily outputs.
Thanks for your review!

@joemasilotti
Copy link
Owner

This is looking great. I'm just figuring out the S3 hosting and will merge in.

@joemasilotti
Copy link
Owner

OK, I think we're good! Turned out I need to add a bunch of stuff to get the S3 upload working and Google Search Console pointing to the sitemap correctly. I'm going to document all of the non-code stuff on the Wiki soon.

Thanks again for the PR!

@joemasilotti joemasilotti merged commit 11d62e7 into joemasilotti:main Dec 27, 2021
@joemasilotti
Copy link
Owner

And we're good! Thanks again 🎉

image

@hungmi hungmi deleted the sitemap-generator branch December 28, 2021 02:42
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.

None yet

2 participants