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

new feature: subsite change detector #515

Open
lekoala opened this issue Mar 17, 2023 · 12 comments
Open

new feature: subsite change detector #515

lekoala opened this issue Mar 17, 2023 · 12 comments

Comments

@lekoala
Copy link
Contributor

lekoala commented Mar 17, 2023

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

@michalkleiner
Copy link
Contributor

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
Copy link
Contributor Author

lekoala commented Mar 17, 2023

@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
Copy link
Contributor

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 added a commit to lekoala/silverstripe-subsites that referenced this issue Mar 17, 2023
@lekoala
Copy link
Contributor Author

lekoala commented Mar 17, 2023

@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
Copy link
Contributor

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

@NightJar
Copy link
Contributor

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

I'm not feeling this

@NightJar
Copy link
Contributor

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
Copy link
Contributor Author

lekoala commented Mar 28, 2023

@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
Copy link
Contributor

@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
Copy link
Contributor Author

lekoala commented Mar 29, 2023

@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
Copy link

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
Copy link
Contributor Author

lekoala commented Apr 17, 2023

@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

@GuySartorelli GuySartorelli self-assigned this Apr 18, 2023
GuySartorelli pushed a commit to lekoala/silverstripe-subsites that referenced this issue Apr 18, 2023
GuySartorelli pushed a commit to lekoala/silverstripe-subsites that referenced this issue Apr 18, 2023
@GuySartorelli GuySartorelli removed their assignment Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants