fix: correct preview view when using subsites

wilr commented 4 months ago

Description

Preference should be given to the page CMSPreviewLink so to align to the functionality on the main view. Link() should be as a fallback if PreviewLink() is not defined on the parent object.

Manual testing steps

This will manifest itself in a few ways the the easiest way to reproduce is

When using subsites and editing a subsite from the main site domain (i.e *.cwp.govt.nz)
The block is non-inline editable

The calculated preview link would be /page/url which will display the main site 404.

Expected behaviour is it matches the pages PreviewLink (with the relevant SubsiteID argument present).

Issues

https://github.com/silverstripe/silverstripe-elemental/issues/1182

Pull request checklist

[x] The target branch is correct
- See picking the right version
[x] All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
- Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
[x] The commit messages follow our commit message guidelines
[x] The PR follows our contribution guidelines
[x] Code changes follow our coding conventions
[x] This change is covered with tests (or tests aren't necessary for this change)
[ ] Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
[x] CI is green

GuySartorelli commented 4 months ago

Can you please create an issue for this, link to the issue, and include more detail in the issue about what the actual problem is and some clearer steps to reproduce it? It's not clear to me from what you've written what the problem actually is.

I've also added the PR template back in, which should have been there when you created the PR initially. Please tick all of the boxes that apply, and make any changes needed (if any) as you go through the list.

wilr commented 4 months ago

Hi @GuySartorelli

The problem can be seen in the 'Manual steps' above.

The CMS Preview when using subsites with Elemental always shows the main website and not the relevant subsite.

This is because on the Element, the CMSPreviewLink uses the 'Link' method of the page (which may return mainsite.cwp.govt.nz/page/url) not the correct preview link (mainsite.cwp.govt.nz/page/url?SubsiteID=1)

GuySartorelli commented 4 months ago

The problem can be seen in the 'Manual steps' above.

Unfortunately I'm not able to follow those steps, which is why I've asked for more details steps I can follow easily.

Please also create an issue and link to it - this is important because the CMS Squad tracks issues, not PRs. It will be hard to ensure this gets the attention it deserves if there's no issue we can track.

wilr commented 4 months ago

Issue at https://github.com/silverstripe/silverstripe-elemental/issues/1182 if that helps at all.

GuySartorelli commented 4 months ago

Based on the description in the linked issue, it seems to me that the API for previewLink() needs to be updated to allow subsites to alter it. I don't think calling Link() directly when trying to get the preview link is correct.

At the very least, we need to retain the instanceof CMSPreviewable check, because if the parent record isn't previewable we shouldn't be providing a preview link to the child record, which can only be previewed in the context of its parent.

wilr commented 4 months ago

I don't think you read the diff correctly - I didn't add the Link() method, I just switched it around so PreviewLink is prefered. The instance of check is still very much present!

GuySartorelli commented 4 months ago

Ahh, yes, apologies I did misread it. I'm gonna have to git blame to see why it was doing the Link() check first originally to ensure this PR doesn't cause a regression - if I can't find anything or the reason is no longer relevant though then this change probably seems okay.

GuySartorelli commented 4 months ago

Looking back at https://github.com/silverstripe/silverstripe-elemental/pull/982 there wasn't any discussion of whether the call to PreviewLink() should be before or after the call to Link().

I think I just had it as a fallback because the call to Link() was there already. So use existing logic, and fall back to new logic.

I'll try this PR out with and without subsites just to make sure it works as expected locally, but the code looks okay now that I've refreshed my memory about the logic.

GuySartorelli commented 4 months ago

Actually... I think there might be an underlying concern here that isn't being addressed. Why is PreviewLink() giving the link for the correct subsite, but Link() isn't? Are we sure subsites is doing the right thing there?

wilr commented 3 months ago

@GuySartorelli Mentioned in my original post Link is designed to be relative. So if you call Link on a page it'll be /page-url (therefore site.cwp.govt.nz/page-url) which will be a 404 on previewing without a SubsiteID.

We could change to AbsoluteLink (which uses the subsite domain) and that would 'fix' the issue as well but I imagine the semantics of PreviewLink is correcter :)

GuySartorelli commented 3 months ago

Ohh right, interesting. Sorry, I must have missed that in your initial post.

I just gave this a quick test with the intention so merge it, but I can't reproduce the original bug locally even without this PR.

Just looking at the code for subsites... there is a BaseElementSubsites class that updates the preview link for elemental blocks. It's added automatically when the elemental module is installed, albeit using classexists instead of moduleexists.

The preview URL is still relative, but it adds the subsite ID as a get variable which works fine. Can you please double check the version of subsites you tested against has that extension?

silverstripe / silverstripe-elemental