NEW show alert when subsite changes in another tab #443
Conversation
When a content editor is working in 2 tabs, and they change the active subsite in one, the second tab will now block further editing with a modal notifying them they cannot continue editing until they change the subsite back in the tab they changed it in :)
NEW react component to block editing when subsite changes
The subsite ID is updated in the PHP session, and the JavaScript had no way of reverting this back once if a user saw the modal on in their admin session. This could lead to confusion about what to do next. Now there is a revert button that will set the PHP session SubsiteID back to that of the tab beneath the modal, making it safe to edit once again. In order to support this, other tabs also need to update, and modals either displayed or hidden, which was already a feature. However these changes only happen on remote tabs (or Documents as per the API definition), so a new custom event has been introduced in order to be able to have the current tab/document update itself - but without coupling the React component to jQuery.entwine that handles the component lifecycle (externally from React).
Such as the subsite that the user is working on in that tab and the subsite that they are changing to
If using multiple subsites and more than two tabs you can create multiple react notices by changing the subsite more than once to different subsites in the same tab To fix this we check for an open notice and always remove it first
NEW revert button that matches active subsite to tab
Fix/411 multitab subsite
Previous attempts to update the stored Subsite ID in the localStorage when a page loaded for the first time would cause an infinite refresh loop. Reasons to set the ID on load are for hard refreshed tabs were a way to remove the modal blocking an edit, and could cause sync issues when multiple tabs are open - undermining the efforts recent commits have set to resolve. This also solves an issue where loading a edit form for a page that did not exist on any other subsite would set the SubsiteID in the PHP Session backend, based on the subsite the page in question. This would create multiple tab sync issues in the CMS silently (no way to tell the UI to block an edit). By setting the SubsiteID when the page loads fresh we can avoid these situations.
Not much functional change, mostly tidy up after testing that updates to the localStorage about the SubsiteID were made only after a successful switch from one subsite to the next (as opposed to assuming success, and possibly getting into the de-sync state that this work seeks to prevent).
IE 11 has limited support for Event and StorageEvents, and needs some help to create and fire the inforation at the correct timings. This commit sees changes made to ensure the correct information is always received by the handlers, triggering the notice appropriately.
Preventing the produciton build of front end assets from completing. This is largely white space changes, but also includes a few minor code adjustments around the JS to satisfy the linter - such as Object.keys(obj).foreach((val) => ...) instead of for(val in obj).
This lead to refactoring the SubsiteChangeAlert component to not rely on global state of the DOM, and instead take props in and reference them directly to perform its required tasks. Equally it accepts a callback for delegating the revert task. This is cleaner and more testable. Also fix small issue with adding divs for the modal as many times as it appears.
|
|
||
| $('.cms-edit-form').entwine({ | ||
| /** | ||
| * TODO: Fix with Entwine API extension. See https://github.com/silverstripe/silverstripe-subsites/pull/125 |
There was a problem hiding this comment.
is this TODO going to get done?
There was a problem hiding this comment.
No, this was existing code, moved in order to facilitate the new yarn build chain to support React components.
2 branch HEAD)
There was a problem hiding this comment.
So, what you're saying is this TODO is just copy pasted from existing code
The link points to a PR merged in 2013, which is not really helpful
If so, just remove the TODO? It's currently just confusing
client/src/components/SubsiteChangeAlert/tests/SubsiteChangeAlert-test.js
Outdated
Show resolved
Hide resolved
client/src/components/SubsiteChangeAlert/SubsiteChangeAlert.jsx
Outdated
Show resolved
Hide resolved
|
Hey @Cheddam you made a mention that you've been working on this recently. Do you have some local changes you could push up? |
|
@brynwhyman They're not quite ready to push, but I'll try and make time to get them polished off tomorrow. |
Such as putting in docblocks to inform future developers what the intent is behind a particular piece of behaviour. Linting has also taken place, and pre-existing minor bugs fixed in legacy code.
|
I've resolved (as in clicked 'resolve' on) all of the feedback items, as they were either addressed or refuted. I've also pushed some additional tidy-up and tweaks that I made while reviewing this (only took me 2 months to get them up, sorry team 😅). Final steps:
|
| @@ -0,0 +1,79 @@ | |||
| /* global window */ | |||
There was a problem hiding this comment.
This file extension needs to be changed to .js to match the rest of the codebase
|
|
||
| return i18n.inject( | ||
| i18n._t( | ||
| 'SubsiteChangeAlert.SUBSITE_CHANGED', |
There was a problem hiding this comment.
should this be added to en.yml?
| newSubsiteName, | ||
| currentSubsiteName | ||
| } | ||
| ).split('[SPLIT]'); |
There was a problem hiding this comment.
It would be cleaner to return an array with two translatable strings SUBSITE_CHANGED_LINE_ONE and SUBSITE_CHANGED_LINE_TWO
|
|
||
| $('.cms-edit-form').entwine({ | ||
| /** | ||
| * TODO: Fix with Entwine API extension. See https://github.com/silverstripe/silverstripe-subsites/pull/125 |
There was a problem hiding this comment.
So, what you're saying is this TODO is just copy pasted from existing code
The link points to a PR merged in 2013, which is not really helpful
If so, just remove the TODO? It's currently just confusing
|
Behat failure is the preview/split mode in using the main site domain instead of the subsite domain. Re-ran travis Have tried rebasing on top of the latest Different issue - got this when running
|
|
Yeah, I'm not feeling this one as it is I find the behat test failure to pretty concerning because it seems to show we've created a new layer in which to create werid bugs. For instance, why do we need to do things like this? https://github.com/silverstripe/silverstripe-subsites/pull/443/files#diff-cf163c29c1a03e751d8439e2f36b5206R176 It seems as though we now need to hunt around and find all the things that now need to be updated via javascript just so that it works with this, new way of doing things? I'm concerned that we're adding complexity and brittleness just to support a nice-to-have though not exactly essential feature. Subsites already has a reputation for having weird bugs. This PR will probably make things worse. As it is I am not confident merging this. I'd like to see a decent chunk of this PR removed and simplified. It may even need change of approach to get there. |
It was supposed to be the simplest way possible to fix the issue where the user is editing in two tabs between subsites and changes their php session subsite id from what is in their tab cache. I think it's ok to close this PR and I'll think about a better approach. |
|
@Mossman1215 I think the idea and general approach here is a good one and it's worth fixing. The issue is that this particuarly javascript implementation is overly complicated and that's creates a maintenance problem on a module that's already complicated. I'll close this one for now, though likely that we'll either reopen this in the future, or, start a new PR and reuse/reference the work done in this one |
Adds a new feature that alerts a user who is editing a site in the same browser but over multiple tabs, and changes the subsite in one tab. Normally nothing untoward would happen until the user then tries to save data for a subsite that is no longer the current one in session. Now a javascript powered modal will pop up alerting users to the fact that the subsite has changed, and includes a button to revert in order to continue editing (so changes aren't lost).
Resolves #314
Closes #411
Tested working in IE11.