Integrate learn-ocaml-editor

[erikmd]

Kind: feature Depends: #302

Dear all,

This PR adds the "learn-ocaml-editor" feature that has been developed in https://github.com/pfitaxel/learn-ocaml-editor by the PFITAXEL team, recently migrated by @leunam217 to the latest version of learn-ocaml (18f0ddcbc37d7505aaa9219cd1a8014c01031375) then simplified − removing one view (high-level version of "Test") and renaming "Test.ml" to "Test" − to anticipate an upcoming evolution to be implemented within @leunam217's internship co-supervised by @yurug and me.

Brief overview of the design choices

From the beginning of the development of this feature, the idea is to provide a local web environment (fully client-side & statically served) for the teachers to create learn-ocaml exercises with:

• a facility to edit all the components of an exercise in the "exodir format" (viz. solution.ml, template.ml, prepare.ml, prelude.ml, test.ml, metadata etc.) with a faster development cycle for teachers.
• in particular:
  • some "Check" buttons are available in all the corresponding tabs ~(albeit since the migration an issue has to be fixed with the "Test" tab)~ [now fixed]
  • a "Generate" feature allows one to create monomorphic test cases automatically from the solution, as well as a "Quality Code" section that we had devised for our teaching unit "PFITA"
  • a "Grade" feature allows one to grade the solution w.r.t. the test suite
  • and an "Experiment" feature (which has been temporarily disabled due to migration concerns) that will allow teachers to immediately visualize the devised exercise from a student point of view (in particular, learn-ocaml's original features "Check" and "Grade" will be available in this view)

~There is currently a JSON import/export feature for exercises, but we plan to work after this PR on enhancing these import/export facilities (e.g. targetting .zip files in the exodir format and/or fetch from external Git exercises repos)~

There is now a .zip import/export feature for exercises in the exodir format. (but we could implement later on another import facility, e.g. to fetch from external Git exercises repos)

Summary of the changes brought by this PR

• Add the button Editor in the main view of the web app (Note: this button is only shown when the token is a teacher one, but the Editor URL itself is not protected (this seems unnecessary) because by design the editor is local-only and doesn't allow inspecting the compiled exercices: it is intended to create new exercices and possibly later fetch from external Git repositories some exercises to be edited)
• Change Learnocaml_data to add data-types used by the editor
• Add the folder editor containing the corresponding files (Note: this can be refactored further, especially editor.ml which is strongly inspired by learnocaml_exercise_main.ml)
• Do a few changes in toplevel (including adding an optional arg to Learnocaml_toplevel.check) to implement the "Check" and "Generate" features for the "Test" tab, while reusing learn-ocaml's codebase.
• Extend the local storage code to save the teacher's exercises that are edited locally.

Overview of the interface

Below is a screenshot [updated] to give a sketch of the corresponding interface:

FYI a statically deployed version (using Github pages) of the previous version of learn-ocaml-editor (0.4.0) is available here: https://pfitaxel.github.io/pfitaxel-demo/

(we'll be able to deploy the latest version of "learn-ocaml+editor" − corresponding to this PR − statically as well if #288 is addressed)

Things to be done before merging this PR

• [x] Finish the integration of the "Experiment" feature
• [x] Investigate the remaining issue with the "Check" feature of the "Test" tab
• [x] Add a feature to warn (and propose to save the exercise) in case of browser tab close or refresh
• [x] Implement some .zip import/export feature in the exodir format
• [x] Refactor the Test/Generate feature to regroup questions about the same function (@erikmd)
• [x] Resolve some remaining little bugs (@leunam217)
• [x] Update the "Generate" feature when #302 is merged (@erikmd)
• [ ] (2022-01-07) 1. fix the blocking bug I had isolated in this commit: https://github.com/ocaml-sf/learn-ocaml/pull/295/commits/a06f37ef6ebdcecef0bd7d7b4f196f7802eb48be
  (I can say I'm up for any help on this issue anytime soon 😅 Cc @yurug @AltGr)
• [ ] (2022-01-07) 2. finish the refactoring/polishing of (ocaml) code I undertook, fixing along the way a few other minor bugs I have spotted
• [ ] (2022-01-07) 3. squash the commits in a better release-please compliant way (but we may probably want to keep the commits in an archived branch, for historical purpose)

[erikmd]

I've just marked this PR as ready for review since all the blocking changes we wanted to address for the first version of "learn-ocaml + editor" are completed; @leunam217 also implemented a .zip import/export feature within this PR :)

@yurug if you want to checkout/test this PR branch, you might want to use this git alias:

git config --global alias.pr '!f() { if [ $# -lt 1 ]; then echo "Usage: git pr <id> [<remote>]  # assuming <remote>[=origin] is on GitHub"; else git checkout -q "$(git rev-parse --verify HEAD)" && git fetch -fv "${2:-origin}" pull/"$1"/head:pr/"$1" && git checkout pr/"$1"; fi; }; f'
git pr 295 origin
make
make opaminstall
_opam/bin/learn-ocaml --repo=demo-repository

Erik

[erikmd]

I've just rebased a few commits to avoid a spurious merge

[erikmd]

FYI I'll be able to finalize the "Generate" feature involved in this PR when #302 is merged.

[yurug]

#302 is merged!

[erikmd]

Hi @yurug, I've just seen that you updated this PR #295; would you have some plan to suggest?

As far as I am concerned, here is what I had in mind before merging this:

• In the test.ml Generate feature, take advantage of the [%funty: _ -> _] new construct (cf. my comment https://github.com/ocaml-sf/learn-ocaml/pull/295#issuecomment-515412333) − this should be easy;
• Polish the PR a bit more;
• (and maybe address https://github.com/ocaml-sf/learn-ocaml/issues/288 …)

[erikmd]

Then, regarding the second step for integrating @leunam217's work: I'll open a new PR based on branch pfitaxel:generate1… (which features a customizable list of templates that can be used by teachers for inserting snippets from Test_lib or so)

[yurug]

Hi Erik, how far are we from the completion of this PR?

[erikmd]

Hi @yurug, technically we are very close! (and moreover we're going to really need this feature on next week) so let's say I finalize this PR and solve the conflict ASAP, for Monday 2020-01-13.

BTW, note that the architecture brought by learn-ocaml-editor / #295 doesn't need a native-ocaml server as a backend, but AFAIK learn-ocaml can't be currently deployed statically while it used to… so if #288 was addressed as well when #295 is ready, that would really be very cool! :+1: (Actually I don't know if addressing #288 is difficult or not, I didn't have a look yet)

[erikmd]

@yurug sorry for the delay; I've resolved the conflict and updated the required bits, but I'll need to do a very last pass on the learn-ocaml-editor branch on this afternoon before you consider it for merging. Stay tuned :)

[yurug]

Hi @erikmd, what is the status of this PR?

[erikmd]

Hi @yurug, thanks a lot for the follow-up! I've been very busy lately but I need to finalize this PR indeed, I bump this in my todo list and I'll try my best to address it before the end of the week.

Then, the plan would be to address #288 as well, so that we could deploy a static learn-ocaml-editor, similarly to what was possible in the earlier versions of learn-ocaml: https://pfitaxel.github.io/pfitaxel-demo/

Do you think #288 is easy to tackle?

[erikmd]

@yurug FYI I've resolved the conflict so the learn-ocaml-editor contrib works again, and I'm precisely going to redeploy it statically.

So to sum up, this PR could be merged soon after:

• merging PR #368 first (which is ready BTW)
• and doing some further polishing (that I could hopefully perform before end-of-december!)

(so I'm temporarily marking this PR as a draft PR)

[erikmd]

(but this PR is not yet ready for review !-)

Actually FYI @yurug, I've just made some progress in the (parallel) branch https://github.com/pfitaxel/learn-ocaml/tree/learn-ocaml-editor-docker that is continuously deployed to https://hub.docker.com/r/pfitaxel/staging-learn-ocaml-editor thanks to that commit; this allowed me to fix all conflicts with master and learn-ocaml-editor, following the integration of the new static deployment support…

So the good news is that now, https://pfitaxel.github.io/pfitaxel-demo (static GitHub Pages) is working again: it thus provides an online "official" version of the learn-ocaml-editor, with learn-ocaml 0.12 compatible test_lib.

What remains to do? polishing the learn-ocaml-editor code and fixing several things that are tracked in the trello… And finally, I'll remove that commit and update this PR's branch itself.

But in the upcoming days, I'll first work on learn-ocaml.el (as well as reviewing #372 :)

[yurug]

I guess we are close to a mergeable PR, right @erikmd ?

[yurug]

Hello @erikmd : what's the status of this PR? Should we close it so that you can integrate its feature in a more incremental way?

[erikmd]

Hi @yurug, thanks anew, so the feature of this branch is almost ready but there are three things that remain to do:

• 1. fix the blocking bug I had isolated in this commit: https://github.com/ocaml-sf/learn-ocaml/pull/295/commits/a06f37ef6ebdcecef0bd7d7b4f196f7802eb48be
     (I can say I'm up for any help on this issue anytime soon 😅 Cc @yurug @AltGr)
• 2. finish the refactoring/polishing of (ocaml) code I undertook, fixing along the way a few other minor bugs I have spotted
• 3. squash the commits in a better release-please compliant way (but we may probably want to keep the commits in an archived branch, for historical purpose)

but no need to close the PR I guess, I'll be able to force-push once the branch has been archived.

Edit: I added these three steps at the end of the description of the issue

[erikmd]

To be a bit more specific of the blocking issue partly documented in commit https://github.com/ocaml-sf/learn-ocaml/pull/295/commits/a06f37ef6ebdcecef0bd7d7b4f196f7802eb48be → the fact is that in (Learn-OCaml)Editor, tested with Firefox, when we click on Experiment to switch from teacher view to student view (and load the exercise data from the Editor tabs), if the descr.md text is not empty but has e.g. ≥14000 chars, then we get a JS Stack_overflow :-/

Furthermore, this issue was not "present" at all in the initial version of (Learn-OCaml)Editor… (I had done a bisection and noticed the issue was introduced when the src/repo/learnocaml_exercise.ml datatypes had been extended in learn-ocaml master… but I don't remember the exact PR number I had found, sorry for that)