Offer better protections against solution overwriting #372

yurug · 2020-12-22T17:00:55Z

Context

This PR introduces three mechanisms to avoid several synchronization issues when two clients are opened on an exercise solution.

Mechanism 1

We disable the automatic and implicit saving of the student answer when the browser tab is closed. Instead, we ask the user to confirm that she wants to leave the page.

Mechanism 2

When an answer has not been modified for 3 minutes, we check if a more recent solution exists on the server. In that case, we ask the user if she wants to download the most recent version.

Mechanism 3

To avoid to overload the server with many synchronization requests, we disable the synchronization button when the answer is synchronized, and reactive it only when a modification is made on the answer.

Fixes: #316

yurug · 2020-12-22T17:07:28Z

@erikmd Can you take a look at this MR?

erikmd · 2020-12-28T17:23:43Z

Hi @yurug, thanks for implementing this!

Can you take a look at this MR?

Sure! I'll have a look ASAP

src/app/learnocaml_common.mli

erikmd · 2021-01-18T01:20:05Z

Hi @yurug, sorry for the delay! but I should have more time this week, to be able to tackle this 🤞 :-)

yurug · 2021-02-17T08:18:02Z

@erikmd Any news related to this PR?

erikmd · 2021-02-18T03:50:22Z

Hi @yurug, not yet, sorry; I've been swamped by several exams… but it's still in my backlog!
BTW did you plan to do a release incorporating these features at a specific date?
would it still be OK if I test/review the PR by this WE?

yurug · 2021-06-12T18:51:02Z

@erikmd Will you have time to review this PR in a close future?

erikmd

Hi @yurug, belated thanks for this PR! (which addresses a very important usability issue)

I rebased the PR on latest master, eveything compiles cleanly with OCaml 4.12.

Then I tested the app on learn-ocaml's demo-repository with firefox-esr on Linux.

The three mechanisms you proposed nicely fit together, and I have one or two remarks/suggestions for each of them:

Mechanism 1

We disable the automatic and implicit saving of the student answer when the browser tab is closed. Instead, we ask the user to confirm that she wants to leave the page.

Minor issue: when the tab is already synchronized, I think the confirmation dialog This page is asking you to confirm that you want to leave - data you have entered may not be saved. should not appear, while it appears unconditionally(?)

Mechanism 2

When an answer has not been modified for 3 minutes, we check if a more recent solution exists on the server. In that case, we ask the user if she wants to download the most recent version.

Nice enhancement! I noticed that the dialog:

~~only shows up when the current tab is already synchronized (i.e., no unsaved code)~~ (ignore this, it was a false conjecture :)
only proposes two choices (OK / Cancel), basically "Keep editing" or Fetch and overwrite";
and the dialog is the same whatever is the status of the code editor (synchronized or with unsaved changes).

I believe we could ~~extend the logic and ask:~~

if the tab is already synchronized:
(Fetch from server | Ignore & keep editing | Do nothing ?)
→ using a dedicated dialog with 3 choices
if the tab contains unsaved code:
(Ignore & keep editing | Sync & keep editing | Fetch from server & overwrite | Do nothing ?)
→ using a dedicated dialog with 4 choices, and making sure that if the user clicks on anything but Do nothing, the dialog won't appear again at the next keystroke.

~~Would you agree with these two suggestions?~~

Edit: See also my later comment, #372 (comment):

We could then refactor these dialogs, relying e.g. on the snippet I suggested here:

if the tab is already synchronized:
(Fetch from server | Ignore & keep editing)
if the tab contains unsaved code:
(Ignore & keep editing | Fetch from server & overwrite)

Mechanism 3

To avoid to overload the server with many synchronization requests, we disable the synchronization button when the answer is synchronized, and reactive it only when a modification is made on the answer.

Good idea! this actually solves "at the root (frontend)" this issue I had raised some time ago.

But small nitpick: I believe the Sync button should be greyed at the beginning (when the exercise has just been opened), as there's no new changes to sync at that point… but this is not the case currently.

src/app/learnocaml_common.ml

src/ace-lib/ace.ml

Signed-off-by: Yann Regis-Gianas <yann@regis-gianas.org>

erikmd · 2022-01-14T15:25:40Z

Just FYI @yurug, I added some ~~strikethrough text~~ in my first review comment to be sure we don't implement my earlier suggestion with 4-button dialogs (needlessly complex).

yurug · 2022-03-04T10:29:38Z

Minor issue: when the tab is already synchronized, I think the confirmation dialog This page is asking you to confirm that you want to leave - data you have entered may not be saved. should not appear, while it appears unconditionally(?)

I turn this one in a follow-up issue (#467) : I cannot find a way to have firefox stop asking for confirmation.

Signed-off-by: Yann Regis-Gianas <yann@regis-gianas.org>

…POLA rule) POLA: https://en.wikipedia.org/wiki/Principle_of_least_astonishment

…e spec * Beforehand: assuming the user was continuously editing the exercise, it was checking the server version every 3 minutes. * Henceforth, as specified in the .ml comment: assuming the user has made no change to the answer since 3 minutes, it checks the server version only in that situation.

erikmd · 2022-03-04T17:15:14Z

Hi, thanks @yurug !

I tested the latest version and committed several minor enhancements.

Before squash-merging:

I'll try to have a look at Bug: Firefox asks for confirmation when leaving an exercise that is synchronized #467 (maybe some snippet from the learn-ocaml-editor branch could help)
And I'll retest the PR within another browser.

@leunam217

This hack is needed to address <#467> because [Dom.handler] <https://ocsigen.org/js_of_ocaml/latest/api/js_of_ocaml/Js_of_ocaml/Dom/index.html#val-handler> appears to be incompatible with "onbeforeunload" and Firefox. * Namely, the semantics of [Dom.handler] requires to "return true" to tell [Dom.handler] that we want to unload normally, * but with some browsers such as Firefox we should actually "return undefined" (better than "return null" BTW), * but then, "undefined" is seen as "false", implying [Dom.handler] will call "preventDefault", * end_of_the_story. Credits: the working code from this commit reuses a patch by @leunam217, pfitaxel@15780b5 See also: https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload#example

erikmd · 2022-03-04T20:59:22Z

The fix b9960f7 has been tested successfully with:

Mozilla Firefox 91.4.1esr
Chromium 98.0.4758.102 (on Debian GNU/Linux)
Safari (on macOS Big Sur)

erikmd · 2022-03-05T16:24:38Z

Good news: the latest version of the PR seems very fine with (Firefox, Chromium, Safari).
So I'll push a very last comment-cleanup commit and squash-merge.

…#372) This fixes "bugs 1, 2" of issue ocaml-sf#505.

…-sf#372 This button triggers a "Fetch_save" and makes a menu appear, allowing end users to reuse their latest graded code, their latest saved code, or the initial template code. This button only shows up when a non-static backend is detected. Update the French translation as well. This fixes "bug 3" of issue ocaml-sf#505.

…-sf#372 This button triggers a "Fetch_save" and makes a menu appear, allowing end users to reuse their latest graded code, their latest saved code, or the initial template code. This button only shows up when a non-static backend is detected. Update the French translation as well. This fixes "bug 3" of issue ocaml-sf#505. Close ocaml-sf#493 Close ocaml-sf#505

Remove Mechanism-2 (#372), Add a 3-fold on-demand Reload button, Fix extra minor bugs

yurug force-pushed the fix-synchronization-issues branch from 380596f to f5c4abd Compare December 22, 2020 17:06

yurug changed the title ~~Offer better protections again solution overwriting~~ Offer better protections against solution overwriting Dec 30, 2020

yurug commented Dec 30, 2020

View reviewed changes

src/app/learnocaml_common.mli Outdated Show resolved Hide resolved

erikmd mentioned this pull request Jan 6, 2021

Integrate learn-ocaml-editor #295

Draft

10 tasks

erikmd added the kind: feature New user-facing feature. label Aug 25, 2021

erikmd mentioned this pull request Sep 15, 2021

meta-issue: learn-ocaml 0.13.0 release plan and milestones suggestions for upcoming releases #418

Closed

15 tasks

erikmd added this to the learn-ocaml 0.13 milestone Sep 15, 2021

erikmd self-assigned this Sep 16, 2021

erikmd force-pushed the fix-synchronization-issues branch 2 times, most recently from 4bd1544 to 4ddafce Compare September 16, 2021 15:52

erikmd reviewed Sep 16, 2021

View reviewed changes

src/app/learnocaml_common.ml Outdated Show resolved Hide resolved

erikmd reviewed Sep 16, 2021

View reviewed changes

src/ace-lib/ace.ml Show resolved Hide resolved

erikmd modified the milestones: learn-ocaml 0.13.0, learn-ocaml 0.14.0 Sep 29, 2021

erikmd mentioned this pull request Oct 5, 2021

meta-issue: learn-ocaml 0.13.1+ release plan and milestones suggestions for upcoming releases #437

Closed

15 tasks

yurug force-pushed the fix-synchronization-issues branch from 4ddafce to f8bb160 Compare January 14, 2022 10:16

Offer better protections against solution overwriting

0d25f20

Signed-off-by: Yann Regis-Gianas <yann@regis-gianas.org>

yurug force-pushed the fix-synchronization-issues branch from f8bb160 to 0d25f20 Compare January 14, 2022 10:29

yurug mentioned this pull request Mar 4, 2022

Bug: Firefox asks for confirmation when leaving an exercise that is synchronized #467

Closed

yurug added 2 commits March 4, 2022 11:56

fixup! Offer better protections against solution overwriting

d90944f

Signed-off-by: Yann Regis-Gianas <yann@regis-gianas.org>

fixup! fixup! Offer better protections against solution overwriting

6b63b7d

Signed-off-by: Yann Regis-Gianas <yann@regis-gianas.org>

yurug requested a review from erikmd March 4, 2022 13:23

erikmd added 5 commits March 4, 2022 17:01

fix(learnocaml_common.ml): Get the *last* version *after* the click (…

66f90b8

…POLA rule) POLA: https://en.wikipedia.org/wiki/Principle_of_least_astonishment

fix: on "Fetch from server & overwrite", set_synchronized := true

d560c17

fix(learnocaml_exercise_main.ml): Refocus after "Sync"

46e43d8

fix(fr.po): Shorten some translations & Run make update-fr-translation

4f37bb1

style: Add/remove comments as needed

c05f8f2

erikmd merged commit 5c12539 into master Mar 5, 2022

erikmd deleted the fix-synchronization-issues branch March 5, 2022 17:44

erikmd mentioned this pull request Dec 13, 2022

Bug: Mechanism-2 of PR #372 does not work #505

Closed

erikmd added a commit to pfitaxel/learn-ocaml that referenced this pull request Dec 16, 2022

fix(web-app): Remove automatic dialog (cf. Mechanism-2 of PR ocaml-sf…

7ea03f1

…#372) This fixes "bugs 1, 2" of issue ocaml-sf#505.

yurug added a commit that referenced this pull request Jan 6, 2023

Merge pull request #506 from pfitaxel/fix-reload

b72c6d3

Remove Mechanism-2 (#372), Add a 3-fold on-demand Reload button, Fix extra minor bugs

github-actions bot mentioned this pull request Jan 6, 2023

chore(master): release 0.14.1 #486

Merged

proofbot mentioned this pull request Jan 7, 2023

2 packages from ocaml-sf/learn-ocaml at 0.14.1 ocaml/opam-repository#22877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offer better protections against solution overwriting #372

Offer better protections against solution overwriting #372

yurug commented Dec 22, 2020

yurug commented Dec 22, 2020

erikmd commented Dec 28, 2020

erikmd commented Jan 18, 2021

yurug commented Feb 17, 2021

erikmd commented Feb 18, 2021

yurug commented Jun 12, 2021

erikmd left a comment •

edited

Loading

erikmd commented Jan 14, 2022

yurug commented Mar 4, 2022

erikmd commented Mar 4, 2022 •

edited

Loading

erikmd commented Mar 4, 2022 •

edited

Loading

erikmd commented Mar 5, 2022

Offer better protections against solution overwriting #372

Offer better protections against solution overwriting #372

Conversation

yurug commented Dec 22, 2020

Context

Mechanism 1

Mechanism 2

Mechanism 3

yurug commented Dec 22, 2020

erikmd commented Dec 28, 2020

erikmd commented Jan 18, 2021

yurug commented Feb 17, 2021

erikmd commented Feb 18, 2021

yurug commented Jun 12, 2021

erikmd left a comment • edited Loading

Choose a reason for hiding this comment

Mechanism 1

Mechanism 2

Mechanism 3

erikmd commented Jan 14, 2022

yurug commented Mar 4, 2022

erikmd commented Mar 4, 2022 • edited Loading

erikmd commented Mar 4, 2022 • edited Loading

erikmd commented Mar 5, 2022

erikmd left a comment •

edited

Loading

erikmd commented Mar 4, 2022 •

edited

Loading

erikmd commented Mar 4, 2022 •

edited

Loading