Skip to content

[Feature:Submission] Rework gradeable access log#5623

Merged
bmcutler merged 21 commits into
masterfrom
access
Sep 17, 2020
Merged

[Feature:Submission] Rework gradeable access log#5623
bmcutler merged 21 commits into
masterfrom
access

Conversation

@MasterOdin

@MasterOdin MasterOdin commented Jul 4, 2020

Copy link
Copy Markdown
Member

Please check if the PR fulfills these requirements:

  • The PR title and message follows our guidelines
  • Tests for the changes have been added/updated (if possible)
  • Documentation has been updated/added if relevant

What is the current behavior?

Closes #5282

The original implementation to record access by a user to a gradeable page was done using a log file that was read completely into memory on each page load and then written back out with a new entry. The file was then copied around during the autograding process, and potentially used. The format of the timestamp in the logfile was not in the right format, using MM-DD-YYYY instead of YYYY-MM-DD. Additionally, during a period of high usage, reading and writing the file was causing an unfortunate system bottleneck. This file was saved one level above the submission version folder, and so required being copied in during the grading process.

Old format:

{
    "page_load_history": [
        {
            "time": "07-04-2020 14:30:33-0400",
            "who": "instructor"
        },
        {
            "time": "07-04-2020 14:34:33-0400",
            "who": "instructor"
        }
    ]
}

What is the new behavior?

We now write to the DB each access, and this is then written out to a file on submission. The timestamp of each entry is also now in the proper format. This file is saved now within the submission folder as a dotfile (.user_assignment_access.json), which removes the need for copying it in during the grading process.

The new format:

[
    {
        "id": 4,
        "g_id": "open_homework",
        "user_id": "instructor",
        "team_id": null,
        "accessor_id": "instructor",
        "timestamp": "2020-07-04 12:35:10-0400"
    },
    {
        "id": 5,
        "g_id": "open_homework",
        "user_id": "instructor",
        "team_id": null,
        "accessor_id": "instructor",
        "timestamp": "2020-07-04 12:35:28-0400"
    }
]

Other information?

Currently I am writing out the entire gradeable_access table row to the file. I expect to get feedback on trimming it down, as well as naming of fields within the review, which will help revise things.

To Do:

  • Decide if to include or not backwards compatibility with old format
  • Commit DB changes to course_schema.sql
  • Update submission tests for checking new generated file

@MasterOdin
MasterOdin requested review from bmcutler, emaicus and hdrodz July 4, 2020 18:36
@MasterOdin

MasterOdin commented Jul 4, 2020

Copy link
Copy Markdown
Member Author

As somewhat hinted at in the OP, this patch as initially committed completely breaks backward compatibility, as it's not clear how much we want to retain it from the linked issue. It would be good to get feedback on how much we might want that, as well as if it would be better to hold off on this PR until semesters change or some other preferred option. There's also a discussion on how much backwards compatibility we want in the file structure itself as well.

Thoughts @bmcutler, @emaicus?

@codecov

codecov Bot commented Jul 4, 2020

Copy link
Copy Markdown

Codecov Report

Merging #5623 into master will increase coverage by 0.00%.
The diff coverage is 37.50%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5623   +/-   ##
=========================================
  Coverage     22.26%   22.27%           
- Complexity     6269     6274    +5     
=========================================
  Files           157      157           
  Lines         20532    20452   -80     
=========================================
- Hits           4572     4555   -17     
+ Misses        15960    15897   -63     
Flag Coverage Δ Complexity Δ
#autograder 9.75% <0.00%> (+0.02%) 0.00 <0.00> (ø)
#migrator 99.10% <ø> (ø) 0.00 <ø> (ø)
#php 21.28% <41.66%> (-0.01%) 6274.00 <6.00> (+5.00) ⬇️
#python_submitty_utils 84.61% <ø> (+2.28%) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@bmcutler
bmcutler marked this pull request as ready for review August 31, 2020 07:01
@bmcutler

Copy link
Copy Markdown
Member

@MasterOdin
I've looked over the code and overall approve it.
I've tested this, and debugged the autograding for time limit notebooks.
So I'm ok to merge, but we are failing travis tests.

Can you please debug travis? Thanks

@emaicus

emaicus commented Sep 16, 2020

Copy link
Copy Markdown
Contributor

Someone should review the migration for this and run the e2e tests locally.

@bmcutler
bmcutler merged commit 7c7a54f into master Sep 17, 2020
@bmcutler
bmcutler deleted the access branch September 17, 2020 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Revise gradeable page load/access log. (2 notes)

4 participants