FIX: add X-Robots-Tag header for check_xhr-covered GET actions, too (PR #9868)

see https://meta.discourse.org/t/missing-x-robots-tag/152593/3 for context

GitHub

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/missing-x-robots-tag/152593/3

Can you add a test for this? We rarely accept fixes without tests.

Can you add a test for this? We rarely accept fixes without tests.

It would be easy if there was an existing non-json GET action in Discourse that

  • allowed non-logged visitors
  • didn’t skip check_xhr filter

Do you know such action? Otherwise I can add such action in spec code similarly to what plugins do but this sounds hacky. Or fine?

Why would a GET for html not skip xhr? I don’t understand now that I’ve read in more detail.

Why would a GET for html not skip xhr? I don’t understand now that I’ve read in more detail.

Initially my plugin’ action didn’t skip check_xhr, and it was happily rendering empty layout then loaded page contents with XHR - the same way e.g. /admin does (its HTML version doesn’t skip check_xhr), but in my case for not logged in users. This doesn’t look wrong to me by itself. But the add_noindex_header not working in this case does look wrong.

@artemv I am still super confused here. Where does /admin render HTML without skipping xhr?

@artemv I am still super confused here. Where does /admin render HTML without skipping xhr?

At standard Discourse. Here, I’ve added logging to check_xhr and requested localhost:3000/admin: bundle exec rails server 2020-05-26 17-43-46

Aha I understand now, sorry what confused me is check_xhr does not ensure that XHR is present, it bootstraps the app for any GET if nothing special is supposed to happen.

It looks like the /groups path will respond to anonymous requests and doesn’t skip xhr, so your test can use that path.

@eviltrout I’m glad you figured it out - I was like, ugh, let’s annotate check_xhr filter to give that guy more context about it. Wait, he’s the author? Eh. :slight_smile: It was long ago though, understood!

@eviltrout just in case - I’ve added a test, please review

Awesome, thanks for explaining and working through it with me.