Skip to content

feat(ui): teacher tab: Display last synced student's draft (≠ graded code) #548

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 30 commits into from
Jul 7, 2023
Merged

feat(ui): teacher tab: Display last synced student's draft (≠ graded code) #548

merged 30 commits into from
Jul 7, 2023

Conversation

Plictox
Copy link
Collaborator

@Plictox Plictox commented May 2, 2023 •
edited by erikmd
Loading

  • Kind: bugfix / enhancement / feature

Close #527

Description

Checklist

Note to maintainers

  • Read this wiki page.
  • Make sure the PR has a milestone.
  • Assign yourself before merging.
  • Either do a regular merge:
    • for PRs containing several commits following conventional-commits,
    • or for PRs containing 1 commit shared with a later PR (to preserve the SHA1)
  • Or do a squash-merge:
    • for PRs containing only 1 commit (not shared with a later PR),
    • or for PRs containing several commits that need not be kept in the history;
    • → Update the commit message header with a conventional-commit type,
    • → Add a footer Close #… if a related issue exists.

@Plictox Plictox self-assigned this May 2, 2023
@Plictox Plictox requested a review from erikmd May 2, 2023 14:21
@erikmd erikmd mentioned this pull request May 3, 2023
Open
@erikmd erikmd added the kind: feature New user-facing feature. label May 3, 2023
@erikmd erikmd assigned Plictox and unassigned Plictox May 3, 2023
@erikmd erikmd assigned erikmd and unassigned Plictox May 3, 2023
@erikmd
refactor: move draft tab
f770483 
Apply a tab order by decreasing importance:
  list (exos) → stats → report →
  editor (graded code) → draft (saved code) →
  text (subject)

Note:
  It would suffice to reorder the buttons in student-view.html,
  but for consistency the other "sequences" are also reordered.
erikmd
erikmd requested changes May 15, 2023
View reviewed changes
Copy link
Member

@erikmd erikmd left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Plictox I just reviewed your PR.
To sum up, the code looks OK at first sight and the "Draft retrieval" feature works.

However, I found the following bug during acceptance testing:

  • If there are ≥2 open exercises,
  • If the student graded only the 1st exercise or saved one Draft for the 1st exercise, not for the 2nd one,
  • If the teacher first clicks on the 1st exercise for this student, then the Draft code appears,
  • But if the teacher then clicks on the 2nd exercise, then the Draft code of the 1st exercise unexpectedly remains visible.

Can you investigate and fix this?

@erikmd
Copy link
Member

erikmd commented May 15, 2023 •
edited
Loading

Also @Plictox, ideally the feature offered by this PR #548 should be complemented with another commit adding this:

  • As a teacher seeing the <div id="learnocaml-exo-tab-draft"></div> tab,
  • I would like that there is a one-line text, just above the ace code block,
  • And this one-liner would show [%i"Ungraded draft, synced on "] ^ "M/D/YYYY, HH:MM:SS AM" (with the same date formatting convention as that of the learnocaml-exo-tab-list), e.g.:
    Brouillon non noté, sauvé le M/D/YYYY, HH:MM:SS AM
  • Normally, the timestamp is available (as a float) in the sync.json data:

    learn-ocaml/src/state/learnocaml_data.ml

    Line 74 in 1584a08

    all_exercise_editors : (float * string) SMap.t ;

@erikmd erikmd added this to the learn-ocaml 1.0.0 milestone May 15, 2023
@Plictox
Copy link
Collaborator Author

Plictox commented May 16, 2023

@Plictox I just reviewed your PR. To sum up, the code looks OK at first sight and the "Draft retrieval" feature works.

However, I found the following bug during acceptance testing:

* If there are ≥2 open exercises,

* If the student graded only the 1st exercise or saved one Draft for the 1st exercise, _not for the 2nd one_,

* If the teacher first clicks on the 1st exercise for this student, then the Draft code appears,

* But if the teacher then clicks on the 2nd exercise, then **the Draft code of the 1st exercise unexpectedly remains visible**.

Can you investigate and fix this?

I found the problem and fixed it.
However I discovered another bug : for a given exercise, if a student has never written code that has compiled the synchronised code will not appear in the draft tab. The code must have compiled at least once.
I will investigate this as well.

@erikmd
Copy link
Member

erikmd commented May 16, 2023

@Plictox OK !

regarding the issue you raised

if a student has never written code that has compiled the synchronised code will not appear in the draft tab

just to be sure (that the draft code is indeed available on the backend side, and not in the teacher dashboard) :

  • check that the draft code appears in ./sync/???/???/???/???/save.json / exercises-editors
  • check that the bug is reproducible even if you refresh the window of your browser

@Plictox
Copy link
Collaborator Author

Plictox commented May 16, 2023

just to be sure (that the draft code is indeed available on the backend side, and not in the teacher dashboard) :

  • for the .json file, the synchronized code appears at the right place
  • i tried with safari and firefox after restarting them, and with different students' accounts ; the bug appeared each time

erikmd
erikmd reviewed May 17, 2023
View reviewed changes
Copy link
Member

@erikmd erikmd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small remark

@erikmd
Copy link
Member

erikmd commented Jun 18, 2023

Dear all,

Good news: this PR that implements a simple but (AFAICT) very useful feature for teachers in the learn-ocaml dashboard, is now ready!

Below are two screenshots when:

the student managed to Grade one's code, and Synced a different code afterwards

2023-06-18_20-03-06_Screenshot_student-view+draft

the student did not try or manage to Grade one's code, and Synced once meanwhile

2023-06-18_20-05-18_Screenshot_student-view+draft

@erikmd
Copy link
Member

erikmd commented Jun 18, 2023 •
edited
Loading

@yurug @AltGr I believe the PR is ready and tidy, so I propose to merge it in 4 days: on Thursday evening, 2023-06-22; unless someone objects (or you approve the PR earlier!)

Note: I tested the PR with Firefox-ESR and Chromium. (edited)

@erikmd
Copy link
Member

erikmd commented Jun 18, 2023

Finally, note that this PR should only be squashed-merged (because there are many fixup commits, including a merge from master, but which does not cause any harm given the future squash-merge).

@erikmd erikmd changed the title feat: Add a draft tab to review synced (not graded) code feat(ui): teacher tab: Display last synced student's draft (≠ graded code) Jun 18, 2023
yurug
yurug reviewed Jun 20, 2023
View reviewed changes
yurug
yurug reviewed Jun 20, 2023
View reviewed changes
yurug
yurug reviewed Jun 20, 2023
View reviewed changes
yurug
yurug reviewed Jun 20, 2023
View reviewed changes
yurug
yurug reviewed Jun 20, 2023
View reviewed changes
yurug
yurug reviewed Jun 20, 2023
View reviewed changes
@yurug
Copy link
Collaborator

yurug commented Jun 20, 2023

I have made a pass on the code and I will try to test ASAP.

@erikmd
Copy link
Member

erikmd commented Jun 20, 2023

@yurug

I have made a pass on the code

Thanks; I addressed your remarks/questions

and I will try to test ASAP.

👍

AltGr
AltGr approved these changes Jun 23, 2023
View reviewed changes
Copy link
Collaborator

@AltGr AltGr left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty nice, thanks!

(NOTE: I reviewed the code but haven't tested)

@erikmd @AltGr
Update src/app/learnocaml_student_view.ml
26fc628 
Apply code review suggestion

Co-authored-by: Louis Gesbert <[email protected]>
erikmd
erikmd approved these changes Jun 24, 2023
View reviewed changes
Copy link
Member

@erikmd erikmd left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just approving, formally (as I'm very happy with this PR which wasn't opened by me)

@erikmd
Copy link
Member

erikmd commented Jun 25, 2023

FYI @yurug @AltGr while we are on it and the PR is not yet merged, I pushed a commit to revert all seemingly-unwanted newlines that were in fr.po.

Still, it appears some of these unwanted newlines were already committed in master before this PR.
(Maybe it is due to gettext, maybe it is due to emacs that has a default auto-fill mode for paragraphs.)

Anyway, now it should be more "standard".

@yurug
Copy link
Collaborator

yurug commented Jul 7, 2023

I have tested it on my machine. Nothing to report. LGTM.

@yurug yurug merged commit 48583ba into ocaml-sf:master Jul 7, 2023
@github-actions github-actions bot mentioned this pull request Jul 7, 2023
Merged
@erikmd erikmd deleted the draft_tab branch July 9, 2023 09:12
@erikmd
Copy link
Member

erikmd commented Jul 9, 2023

@yurug thanks for the squash-merge 👍

@erikmd erikmd mentioned this pull request Jul 9, 2023
Closed
This was referenced Aug 23, 2023
Merged
Merged
@erikmd erikmd mentioned this pull request Nov 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurug yurug yurug left review comments

@AltGr AltGr AltGr approved these changes

@erikmd erikmd erikmd approved these changes

Assignees

@erikmd erikmd

Labels
kind: feature New user-facing feature.
Projects
None yet
Milestone
learn-ocaml 0.15.0
Development

Successfully merging this pull request may close these issues.

Feature: View synced (not graded) version of students' code
4 participants
@Plictox @erikmd @yurug @AltGr