Implement one user concurrent access for storylines

[mohsin-r]

Changes

• Only one user is now permitted to load, edit, and save a storyline at a time. This is achieved through "locking" a UUID when a user initially creates or loads it. When the user's session ends (either through a 30 minute timeout, closing the tab, navigating to another route), the storyline is again "unlocked" for other users. Note that 30 seconds before the session timeout, the user will be prompted to continue the session, if they wish.

Notes

• To help people test the PR, the session timeout has been set to 40 seconds (or else you'd have to wait forever for the modal to show up). This will be changed before the PR is merged. You can also see the countdown printing in the console to track when the modal shows and the session expires.

Testing

Unfortunately this will need to be tested locally (or on the dev server once that is deployed) as there are server changes. The best way to test this is:

1. Open the app in two separate tabs.
2. Try and break concurrency in as many ways as you can think of (e.g. loading the same product at once, creating two new products with the same UUID, ending the session of one and then loading the other, confirming/cancelling the session timeout, etc.)
3. Grouse if anything does not work.

---

This change is 

[github-actions]

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/sockets

[mohsin-r]

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions

---

src/stores/lockStore.ts line 77 at r1 (raw file):

        // Resets the current session back to a full 30 minutes.
        resetSession() {
            this.timeRemaining = 40; // TODO: Change to 1800 (30 minutes) when testing is done.

blocker

---

src/stores/lockStore.ts line 84 at r1 (raw file):

            this.timeInterval = setInterval(() => {
                // TODO: Remove later. Countdown for debugging.
                console.log(this.timeRemaining);

blocker

[mohsin-r]

Reviewable status: 0 of 17 files reviewed, all discussions resolved

---

src/stores/lockStore.ts line 77 at r1 (raw file):

Previously, mohsin-r (Mohsin Reza) wrote…

blocker

Removed

---

src/stores/lockStore.ts line 84 at r1 (raw file):

Previously, mohsin-r (Mohsin Reza) wrote…

blocker

Removed

[mohsin-r]

Rebased and updated. Ready for review now. Changes can be tested on dev site.

Reviewable status: 0 of 17 files reviewed, all discussions resolved

[szczz]

Reviewed 17 of 17 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mohsin-r)

---

server/index.js line 208 at r2 (raw file):
Can we reword this message? Something like

Aborted: The secret key is missing or does not correspond to this storyline's lock.

---

src/components/landing.vue line 46 at r2 (raw file):

        const socket = ref(null);
        socket.value = new WebSocket(socketUrl + '/Storylines-Editor-STB-Server/');

This test code can be 💥

---

server/index.js line 560 at r2 (raw file):

    // { uuid: <uuid>, lock: true }
    // { uuid: <uuid>, lock: false }
    // TODO: Do we need this stuff in the logs?

Wouldn't hurt. Could help us debug locking issues in the future.

---

server/package.json line 22 at r2 (raw file):

        "formidable": "^2.1.2",
        "fs-extra": "^11.1.0",
        "https": "^1.0.0",

https, and perhaps express-ws can be removed.

Just need http for this implementation

[szczz]

We should be removing the lock on a Storyline as soon as the user tries to load another product, even if it fails.

Currently if user 1 loads product "123", and user 2 loads product "456" they will each have a lock on their respective storyline. If User 1 then tries to load product "456", they'll be told its in use by another user and the meta data / version history of product "123" will no longer be displayed. If user 3 then tries to load "123", they'll be told its in use by another user even though user 1 would need to reload the product to do anything with it.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mohsin-r)

[RyanCoulsonCA]

A question:

Do we only reset the session timer when loading/saving a product? Assuming a 30 minute session timeout and 5 minute warning, I'm imagining a scenario where a user works on a product for 25 minutes without saving, tabs out for 6 minutes to search for an image, and then tabs back in to the product to find themselves kicked out due to inactivity and losing their work.

This doesn't need to be fixed right now and may be worth a discussion in a future meeting, but I wonder if it's worth resetting the timer any time an action is performed instead of just setting the timer on load/save, effectively acting as an inactivity timer.

I'm thinking that down the line we should also add a bit of text somewhere (maybe when loading the product, or in the help menu) that warns the user that their session will automatically end after X minutes, just in case someone who doesn't know about the timeout does a bunch of work on a product and then goes to lunch without saving.

---

While testing on the dev site, I found a couple of issues:

First one: this one probably isn't very important as it's a bit weird.

1. Enter a product UUID in tab 1 and click Load.
2. Once the product loads, change the app language by clicking 'Francais' at the top right.
3. The page switches languages and the product is now unloaded on that tab, but I can't load the product in tab 2 because it is still locked.
4. If I re-enter the same UUID in tab 1, I can still load that product.

---

Second one: it seems like I get kicked out before my session timer actually hits 0. I got lucky and recorded a gif of this one:

Reviewed 17 of 17 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mohsin-r)

---

server/index.js line 324 at r2 (raw file):

app.route(ROUTE_PREFIX + '/retrieve/:id/:lang').get(function (req, res) {
    // Before any operation can be performed with the storyline, we need to ensure that the requester is the one who holds the lock for this storyline.
    if (!lockedUuids[req.params.id]) {

Since we use this same code block for a bunch of different endpoints, I almost think it'd be useful to turn this into a helper function and we can just call it at the beginning of anything we want to protect with a lock. Feel free to ignore this if my logic doesn't work for whatever reason though.

[mohsin-r]

Thanks for all the detailed testing.

We should be removing the lock on a Storyline as soon as the user tries to load another product, even if it fails.

Currently if user 1 loads product "123", and user 2 loads product "456" they will each have a lock on their respective storyline. If User 1 then tries to load product "456", they'll be told its in use by another user and the meta data / version history of product "123" will no longer be displayed. If user 3 then tries to load "123", they'll be told its in use by another user even though user 1 would need to reload the product to do anything with it.

Good catch, donethanks.

I'm imagining a scenario where a user works on a product for 25 minutes without saving, tabs out for 6 minutes to search for an image, and then tabs back in to the product to find themselves kicked out due to inactivity and losing their work.

To rectify this point, we now have an inactivity timer instead of just a timer, so on moving the mouse or pressing a key it will reset.. This also applies if previewing the product by clicking the preview button (performing activity in the preview tab extends the session). However, after the warning popup shows up it no longer resets on activity (otherwise the popup would be somewhat redundant). Additionally, we have an autosave on session expire and a more clear banner is displayed on the home page informing the user that their work has been saved.

Some of the other idea like showing a message on load/in the help menu are great too, and perhaps these can be pushed to a later discussion.

Feel free to test this new behaviour in various scenarios.

First one: this one probably isn't very important as it's a bit weird.

1. Enter a product UUID in tab 1 and click Load.
2. Once the product loads, change the app language by clicking 'Francais' at the top right.
3. The page switches languages and the product is now unloaded on that tab, but I can't load the product in tab 2 because it is still locked.
4. If I re-enter the same UUID in tab 1, I can still load that product.

This should be fixed now, in particular going to 'Francais' will unlock the product for other users, and you'll have to load it again.
Thanks for the catch.

Second one: it seems like I get kicked out before my session timer actually hits 0. I got lucky and recorded a gif of this one:

This should also be fixed, donethanks.

Finally, based on slack feedback, previewing a product no longer requires locking it.

Reviewable status: 7 of 17 files reviewed, 5 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

---

server/index.js line 208 at r2 (raw file):

Previously, szczz (Matt Szczerba) wrote…

Can we reword this message? Something like

Aborted: The secret key is missing or does not correspond to this storyline's lock.

Donethanks. This text is now in the verifySecret() helper function below.

---

server/index.js line 324 at r2 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Since we use this same code block for a bunch of different endpoints, I almost think it'd be useful to turn this into a helper function and we can just call it at the beginning of anything we want to protect with a lock. Feel free to ignore this if my logic doesn't work for whatever reason though.

Nice suggestion, donethanks.

---

server/index.js line 560 at r2 (raw file):

Previously, szczz (Matt Szczerba) wrote…

Wouldn't hurt. Could help us debug locking issues in the future.

Donethanks.

---

server/package.json line 22 at r2 (raw file):

Previously, szczz (Matt Szczerba) wrote…

https, and perhaps express-ws can be removed.

Just need http for this implementation

Donethanks.

---

src/components/landing.vue line 46 at r2 (raw file):

Previously, szczz (Matt Szczerba) wrote…

This test code can be 💥

Donethanks.

[mohsin-r]

Reviewable status: 7 of 17 files reviewed, 6 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

---

src/stores/lockStore.ts line 91 at r3 (raw file):

            // Update the time remaining every second.
            this.timeInterval = setInterval(() => {
                console.log(this.timeRemaining);

Blocker.

The console continuously displays the number of seconds remaining in a session. Feel free to use this to gauge whether the correct action occurs at the correct time.

[szczz]

🏆

Reviewed 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)

---

src/components/metadata-editor.vue line 844 at r3 (raw file):
🚓 Crimes against spelling

inacitvity

---

src/components/metadata-editor.vue line 852 at r3 (raw file):

inacitvity

---

src/lang/lang.csv line 29 at r3 (raw file):

editor.lang.en,English,1,Anglais,1
editor.lang.fr,French,1,Français,1
editor.ok,Ok,1,[FR] Ok,0

Believe this is D'accord. I'm sure we have this translated somewhere.

[mohsin-r]

Reviewable status: 15 of 17 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA and @szczz)

---

src/components/metadata-editor.vue line 844 at r3 (raw file):

Previously, szczz (Matt Szczerba) wrote…

🚓 Crimes against spelling

inacitvity

Whoops, donethanks.

---

src/components/metadata-editor.vue line 852 at r3 (raw file):

Previously, szczz (Matt Szczerba) wrote…

inacitvity

Donethanks.

---

src/lang/lang.csv line 29 at r3 (raw file):

Previously, szczz (Matt Szczerba) wrote…

Believe this is D'accord. I'm sure we have this translated somewhere.

Donethanks.

[szczz]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)

[yileifeng]

1 issue found while testing on the dev site (on initial load):

1. Load product normally after selecting UUID and then back out from main editor page
2. Access the same product under the previous products section
3. From a different tab, load the same product to break concurrency

Also unsure if related to this PR but noticed that when the session times out and the product gets auto saved, it doesn't appear in the version history:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)

[mohsin-r]

Apologies for the late response, ran into a few issues with dev deployment.

1 issue found while testing on the dev site (on initial load):

Excellent catch, this should now be fixed. In particular, accessing the product under the "previous products" section will lock the storyline properly.

Also unsure if related to this PR but noticed that when the session times out and the product gets auto saved, it doesn't appear in the version history

After playing around, this one is a bit tricky. If you had any unsaved changes before the session expiring, the save occurs and shows up in the version history. However, if you did not have any unsaved changes, when you save, git sees that you have the exact same files and hence no commit is created since there are no changes so you can't see anything on the version history. Now ideally, we can trigger a save only if the unsavedChanges but lets say if you make a change and undo it, it would still trigger a save because of the indicator but git would still see the same files.

So because of this I am not sure what the best way forward is. Maybe we consider this as "correct" behaviour i.e. no changes = no new version? Bear in mind that even when you hit the save button with no difference in the zipped folder, you won't see a new version. Feel free to voice opinions below.

Reviewable status: 16 of 17 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA and @szczz)

[szczz]

After playing around, this one is a bit tricky. If you had any unsaved changes before the session expiring, the save occurs and shows up in the version history. However, if you did not have any unsaved changes, when you save, git sees that you have the exact same files and hence no commit is created since there are no changes so you can't see anything on the version history. Now ideally, we can trigger a save only if the unsavedChanges but lets say if you make a change and undo it, it would still trigger a save because of the indicator but git would still see the same files.

In this case we'll need to pass a flag from the Express server that prevents the Vue app from relaying the save to the .NET API. The VersionHistory table has a CommitHash column and thus if no commit is made then there will be no hash and API likely grouses back. ** Mohsin confirmed that the Express server will use the previous commit hash in the response. Likely disrespectful.

Is the save button enabled because of the possibility that the user made metadata changes? I would expect the button to be disabled if no new changes were made.

So because of this I am not sure what the best way forward is. Maybe we consider this as "correct" behaviour i.e. no changes = no new version? Bear in mind that even when you hit the save button with no difference in the zipped folder, you won't see a new version. Feel free to voice opinions below.

We can either just re-direct to the dashboard and not save if no changes have been made or we introduce the flag to ensure the DB is aligned with the local repo.

[mohsin-r]

Is the save button enabled because of the possibility that the user made metadata changes? I would expect the button to be disabled if no new changes were made.

So another issue we have is, theres no reliable way to detect "do we have unsaved changes rn?"
We have that indicator, but:

1. It doesnt fire on a few changes (e.g. editing a map config)
2. If you make a change and undo a change, it thinks you have unsaved changes

Thats why we have the always save enabled approach.
I like Matt's flag idea, we could use git trickery to detect if the commit was made and in addition to determining if we call the .NET API, we could also show the user a message along the lines of "You did not have any changes!"

Will hold off on implementing this so that people can get in their suggestions/grouses.

Reviewable status: 16 of 17 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA and @szczz)

[yileifeng]

Passing the flag from the express server sounds like a reasonable idea for the context of this PR.

Regarding the issue of reliably detecting unsaved changes, these issues have persisted since the creation of RESPECT with #139 #123. Definitely worth a discussion next meeting for implementation details and in agreement that we should improve upon the current simple behavior of the unsaved changes indicator. An initial idea is to store a separate copy of the last saved version of config slides and the metadata object inside a store upon loading in a product. Then, we could replace the current watcher behavior that updates the save status indicator with a deep object comparison method (e.g. Lodash).

Reviewable status: 16 of 17 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA and @szczz)

[RyanCoulsonCA]

After some more extensive testing, I've found an interesting issue:

1. Open the developer tools.
2. Load a product.
3. Switch tabs away from the product and keep an eye on the dev console.
4. Notice that for around 30 seconds (on Chrome, 60 seconds on Edge), the countdown is working as expected. After that, it begins to slow down drastically.

The top answer in this Stack Overflow thread explains that browsers tend to set inactive tabs to a lower priority which can cause issues with JavaScript timers.

I'm running into this issue on Chrome and Edge, not Firefox (just after posting this, the timer slowed down on Firefox with 13 seconds left on the clock). Browser settings may have an effect?

I won't block for this issue because I'm not sure what the best path forward is. We should keep it in mind though, because currently if I lock a product and tab out, the inactivity timer will take a very long time to expire.

Reviewed 8 of 10 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mohsin-r)

---

src/components/preview.vue line 136 at r5 (raw file):

inacitivity

Minor typo

[RyanCoulsonCA]

False alarm! Ignore my last comment about the timer slowing down. You still get kicked out after 10 minutes. It's just the console.logs in the dev console that slow down.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mohsin-r)

[mohsin-r]

Phew lol!

Update: added a boolean flag on the server to detect if git did a commit or not. If not, we display a "No unsaved changes detected" to the user and we also don't call the .NET API route that records a new version. If yes, we do the save actions as normal.

Reviewable status: 12 of 17 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

---

src/components/metadata-editor.vue line 1529 at r6 (raw file):

                .then((res: AxiosResponse) => {
                    const responseData = res.data;
                    console.log(responseData);

Blocker to be removed before merge. Allows people to see that the boolean flag spits out the correct value.

[mohsin-r]

Reviewable status: 11 of 17 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

---

src/components/metadata-editor.vue line 1529 at r6 (raw file):

Previously, mohsin-r (Mohsin Reza) wrote…

Blocker to be removed before merge. Allows people to see that the boolean flag spits out the correct value.

Actually I just removed it, since you can tell from the UI.

---

src/components/preview.vue line 136 at r5 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

inacitivity

Minor typo

Whoops sorry I missed this earlier, donethanks.

[mohsin-r]

Rebased. Since it was a nasty one, would appreciate a look to ensure I didnt erase anyone's work.

Reviewable status: 10 of 17 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

[RyanCoulsonCA]

Seems to be working well for me 👍

Reviewed 2 of 5 files at r6, 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @szczz)

[mohsin-r]

Rebased and remove console logs.

Reviewable status: 13 of 17 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA and @szczz)

[szczz]

Reviewed 3 of 5 files at r7, 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)