new feature: subsite change detector

[lekoala]

I've recently played around with a little js snippet and I think it would be a worthwhile addition to the module

the goal is to detect when a subsite change has occurred in another tab. This can help preventing many odd situations where you would click and suddenly see a whole new page due to the context change

the solution works like this, it's a really simple js snippet

• store the currently selected subsite in local storage
• detect changes in storage. if subsite has changed, prompt for reload

what do you think?

  // Subsite change detector
  var subsiteSessionKey = "admin_subsite_id";
  var subsiteSelectedId = null;
  window.addEventListener("storage", () => {
    var tabId = localStorage.getItem(subsiteSessionKey);
    if (tabId && subsiteSelectedId != tabId) {
      if (confirm("The current subsite has changed. Reload the page ?")) {
        window.location.reload();
      }
    }
  });
  $("#SubsitesSelect").entwine({
    onmatch: function () {
      subsiteSelectedId = this.find("option[selected]").attr("value");
      localStorage.setItem(subsiteSessionKey, subsiteSelectedId);
    },
  });

Acceptance criteria

• Users are warned when their session is changed to target a different subsite.
• Validate what currently happens when the user tries to make a change when looking at subsite that doesn't match what is in their session.
  • Clarify what should happen when the user chooses to ignore your warnings.
• Need to make sure the Subsite ID in the PHP session is always respected even if it doesn't match the local storage Susbsite ID.

PR

• Add subsite change detection #516

[michalkleiner]

That looks nice and we would certainly have a use for it. The message could be more descriptive in terms of how the current subsite changed, mentioning another tab or other window, so editors don't think something changed it magically or they got hacked.
Not sure if the JS needs any more safeguards, could local storage be unavailable? Is the onmatch event reliable way how to track the change? Does the window always fully reload when changing subsites? Could/should it leverage onclick handler as well?

[lekoala]

@michalkleiner yes even if the following snippet is working fine for my use case, it needs to be tweaked a bit before being added

• For the message itself, maybe something like "You've changed subsite in another tab, do you want to reload the page?"
• Make it translatable with ss.i18n
• Add a try catch block for the set operation
• As far as I'm aware, there is a full reload when changing subsite, and therefore the onmatch works fine. I'm not sure using onclick would be a good idea: you can try to change subsite and then it might not actually work. with onmatch, you have the guarantee of the current subsite

[michalkleiner]

I would support this enhancement if you want to open a PR for it.
There might be parts of the CMS where a reload might not be required, just trying to thing where I would actually not want it within the CMS. Probably make it an opt-in enhancement that can be turned on through config. Then we can discuss what the default should be.

[lekoala]

@michalkleiner done, PR created with the little changes mentioned. I did try this locally on my project and it seems fine, but I made the edit directly on github because it's much more convenient :)
it would be great if someone from the team can give this a try and see if everything works as expected

[michalkleiner]

I'll try to test it locally on one of our multi-subsite builds.

[NightJar]

This has already been done.
The PR was closed because the reviewer wasn't in the mood.

I'm not feeling this

[NightJar]

The critical thing that's missing @lekoala is that giving the user the chance to dismiss the message brings in their ability to click the save button again. This causes huge issues (saving data to the wrong site, potentially moving it between sites) that have afflicted users for years. Although the notice is good, it doesn't prevent the underlying issues it tries to avoid :)

[lekoala]

@NightJar yes, I agree it's not ideal, but it's an improvement. A better situation is always a good thing, I believe.
Yes, you can ignore and save regardless, "most of the times" it works fine, due to subsiteID being passed along when you save a form. But then, since you ignored the warning, at least you are not surprised that the subsite change when you navigate.

anyway, that's my take, it's an easy snippet to add for anyone who wants to add this
maybe don't make it a default and let this as an opt in option for developers that prefer this way of thinking

[NightJar]

@lekoala I agree, improvement that does no harm (side effect) is good. It's not always a shared opinion unfortunately.

Please don't take my feedback as denial of approach - I mention as it's relevant feedback... but it depends on your goals with this development. We tried a full solution that helped people unaware of the side effects of multi-tab editing avoid disrupting their site. The solution here achieves a very similar outcome with much less code, which is perfectly acceptable I think. If you wanted to take the solution further, the feedback is relevant, if not, then that's fine :)

[lekoala]

@NightJar no worries, i fully agree with you
anyway i'm letting silverstripe's team decide, my goal was simply to share something find useful for myself

[maxime-rainville]

There's probably a lot of side questions about how we should be handling this type of situation. We don't really have the time to ask or answer those questions. Given that the attach PR seems like a good incremental improvements, we're inclined to review it as is for now.

[lekoala]

@maxime-rainville maybe the one change that is still missing to make it 100% is to make it configurable, ie: add a setting to make it opt in for example