diff --git a/_docs/assignments.md b/_docs/assignments.md new file mode 100644 index 00000000..23776012 --- /dev/null +++ b/_docs/assignments.md @@ -0,0 +1,14 @@ +# Autograder Assignments + +## LMS Syncing Mechanics + +When syncing assignment information from an LMS, +only fields not set in the assignment config will be brought over from the LMS. + +Matching autograder assignments with LMS assignments are first done via the LMS id set in the assignment's config. +If no LMS is is set, then an attempt is made to match assignments via their name (if the name is not empty). +A name match is made only if an autograder assignment matches one and only one LMS assignment. + +## Duplicate Assignments + +Assignments in the same course may not share the same ID, name, or LMS ID. diff --git a/api/lms/routes.go b/api/lms/routes.go index d7e6d343..0172aece 100644 --- a/api/lms/routes.go +++ b/api/lms/routes.go @@ -8,7 +8,7 @@ import ( var routes []*core.Route = []*core.Route{ core.NewAPIRoute(core.NewEndpoint(`lms/user/get`), HandleUserGet), - core.NewAPIRoute(core.NewEndpoint(`lms/sync/users`), HandleSyncUsers), + core.NewAPIRoute(core.NewEndpoint(`lms/sync`), HandleSync), core.NewAPIRoute(core.NewEndpoint(`lms/upload/scores`), HandleUploadScores), }; diff --git a/api/lms/sync_users.go b/api/lms/sync.go similarity index 53% rename from api/lms/sync_users.go rename to api/lms/sync.go index 90b96cda..4d95ae5e 100644 --- a/api/lms/sync_users.go +++ b/api/lms/sync.go @@ -2,10 +2,11 @@ package lms import ( "github.com/eriq-augustine/autograder/api/core" + "github.com/eriq-augustine/autograder/model" "github.com/eriq-augustine/autograder/procedures" ) -type SyncUsersRequest struct { +type SyncRequest struct { core.APIRequestCourseUserContext core.MinRoleAdmin @@ -13,24 +14,26 @@ type SyncUsersRequest struct { SkipEmails bool `json:"skip-emails"` } -type SyncUsersResponse struct { - core.SyncUsersInfo +type SyncResponse struct { + Users *core.SyncUsersInfo `json:"users"` + Assignments *model.AssignmentSyncResult `json:"assignments"` } -func HandleSyncUsers(request *SyncUsersRequest) (*SyncUsersResponse, *core.APIError) { +func HandleSync(request *SyncRequest) (*SyncResponse, *core.APIError) { if (request.Course.GetLMSAdapter() == nil) { return nil, core.NewBadRequestError("-403", &request.APIRequest, "Course is not linked to an LMS."). Add("course", request.Course.GetID()); } - result, err := procedures.SyncAllLMSUsers(request.Course, request.DryRun, !request.SkipEmails); + result, err := procedures.SyncLMS(request.Course, request.DryRun, !request.SkipEmails); if (err != nil) { return nil, core.NewInternalError("-404", &request.APIRequestCourseUserContext, - "Failed to sync LMS users.").Err(err); + "Failed to sync LMS information.").Err(err); } - response := SyncUsersResponse{ - SyncUsersInfo: *core.NewSyncUsersInfo(result), + response := SyncResponse{ + Users: core.NewSyncUsersInfo(result.UserSync), + Assignments: result.AssignmentSync, }; return &response, nil; diff --git a/api/lms/sync_users_test.go b/api/lms/sync_test.go similarity index 92% rename from api/lms/sync_users_test.go rename to api/lms/sync_test.go index a312cf61..34a2b0b6 100644 --- a/api/lms/sync_users_test.go +++ b/api/lms/sync_test.go @@ -8,7 +8,7 @@ import ( "github.com/eriq-augustine/autograder/util" ) -func TestAPISyncUsers(test *testing.T) { +func TestLMSSync(test *testing.T) { testCases := []struct{ role model.UserRole; dryRun bool; skipEmails bool; permError bool }{ {model.RoleOther, false, true, true}, {model.RoleStudent, false, true, true}, @@ -28,7 +28,7 @@ func TestAPISyncUsers(test *testing.T) { "skip-emails": testCase.skipEmails, }; - response := core.SendTestAPIRequestFull(test, core.NewEndpoint(`lms/sync/users`), fields, nil, testCase.role); + response := core.SendTestAPIRequestFull(test, core.NewEndpoint(`lms/sync`), fields, nil, testCase.role); if (!response.Success) { if (testCase.permError) { expectedLocator := "-020"; @@ -44,7 +44,7 @@ func TestAPISyncUsers(test *testing.T) { } // Ensure the response can unmarshal. - var responseContent SyncUsersResponse; + var responseContent SyncResponse; util.MustJSONFromString(util.MustToJSON(response.Content), &responseContent); } } diff --git a/cmd/lms-sync-users/main.go b/cmd/lms-sync/main.go similarity index 73% rename from cmd/lms-sync-users/main.go rename to cmd/lms-sync/main.go index 7fc20e8e..7527ef5f 100644 --- a/cmd/lms-sync-users/main.go +++ b/cmd/lms-sync/main.go @@ -1,12 +1,15 @@ package main import ( + "fmt" + "github.com/alecthomas/kong" "github.com/rs/zerolog/log" "github.com/eriq-augustine/autograder/config" "github.com/eriq-augustine/autograder/db" "github.com/eriq-augustine/autograder/procedures" + "github.com/eriq-augustine/autograder/util" ) var args struct { @@ -18,7 +21,7 @@ var args struct { func main() { kong.Parse(&args, - kong.Description("Sync local users with matching LMS users."), + kong.Description("Sync course information from a course's LMS."), ); err := config.HandleConfigArgs(args.ConfigArgs); @@ -31,10 +34,10 @@ func main() { course := db.MustGetCourse(args.Course); - result, err := procedures.SyncAllLMSUsers(course, args.DryRun, !args.SkipEmails); + result, err := procedures.SyncLMS(course, args.DryRun, !args.SkipEmails); if (err != nil) { - log.Fatal().Err(err).Msg("Failed to sync LMS users."); + log.Fatal().Err(err).Msg("Failed to sync LMS."); } - result.PrintReport(); + fmt.Println(util.MustToJSONIndent(result)); } diff --git a/cmd/users/main.go b/cmd/users/main.go index 551b9f5b..e44a2b28 100644 --- a/cmd/users/main.go +++ b/cmd/users/main.go @@ -52,7 +52,7 @@ func (this *AddUser) Run(course *model.Course) error { } fmt.Println("Add Report:"); - result.PrintReport(); + fmt.Println(util.MustToJSONIndent(result)); if (this.SyncLMS) { result, err = procedures.SyncLMSUserEmail(course, this.Email, this.DryRun, this.SendEmail); @@ -61,7 +61,7 @@ func (this *AddUser) Run(course *model.Course) error { } fmt.Println("\nLMS sync report:"); - result.PrintReport(); + fmt.Println(util.MustToJSONIndent(result)); } return nil; @@ -89,7 +89,7 @@ func (this *AddTSV) Run(course *model.Course) error { if (this.DryRun) { fmt.Println("Doing a dry run, users file will not be written to."); - result.PrintReport(); + fmt.Println(util.MustToJSONIndent(result)); } if (this.SyncLMS) { @@ -105,7 +105,7 @@ func (this *AddTSV) Run(course *model.Course) error { if (this.DryRun) { fmt.Println("LMS sync report:"); - result.PrintReport(); + fmt.Println(util.MustToJSONIndent(result)); } } diff --git a/common/timestamp.go b/common/timestamp.go index 6321c454..98ada177 100644 --- a/common/timestamp.go +++ b/common/timestamp.go @@ -45,6 +45,19 @@ func MustTimestampFromString(text string) Timestamp { return instance; } +func (this Timestamp) Validate() error { + if (this.IsZero()) { + return nil; + } + + _, err := this.Time(); + if (err != nil) { + return err; + } + + return nil; +} + func (this Timestamp) IsZero() bool { return (string(this) == ""); } diff --git a/db/database.go b/db/database.go index 0d32f9e9..2b9fdff8 100644 --- a/db/database.go +++ b/db/database.go @@ -44,14 +44,14 @@ type Backend interface { // Returns (nil, nil) if the course does not exist. GetCourse(courseID string) (*model.Course, error); - // Load a course into the databse and return the course. + // Load a course into the database and return the course. // This implies loading a course directory from a config and saving it in the db. // Will search for and load any assignments, users, and submissions // located in the same directory tree. // Override any existing settings for this course. LoadCourse(path string) (*model.Course, error); - // Explicitly save a course. + // Explicitly save a course (which includes all course assignments). SaveCourse(course *model.Course) error; // Dump a database course to the standard directory layout @@ -74,7 +74,7 @@ type Backend interface { // Remove a user. // Do nothing and return nil if the user does not exist. RemoveUser(course *model.Course, email string) error; - + // Remove a submission. // Return a bool indicating whether the submission exists or not and an error if there is one. RemoveSubmission(assignment *model.Assignment, email string, submissionID string) (bool, error); diff --git a/model/assignment.go b/model/assignment.go index 90d7809b..795493cc 100644 --- a/model/assignment.go +++ b/model/assignment.go @@ -21,6 +21,9 @@ type Assignment struct { Name string `json:"name"` SortID string `json:"sort-id,omitempty"` + DueDate common.Timestamp `json:"due-date,omitempty"` + MaxPoints float64 `json:"max-points,omitempty"` + LMSID string `json:"lms-id,omitempty"` LatePolicy *LateGradingPolicy `json:"late-policy,omitempty"` @@ -86,16 +89,21 @@ func (this *Assignment) Validate() error { return fmt.Errorf("No course found for assignment.") } - if (this.Name == "") { - this.Name = this.ID; - } - var err error; this.ID, err = common.ValidateID(this.ID); if (err != nil) { return err; } + err = this.DueDate.Validate(); + if (err != nil) { + return fmt.Errorf("Due date is not a valid timestamp: '%w'.", err); + } + + if (this.MaxPoints < 0.0) { + return fmt.Errorf("Max points cannot be negative: %f.", this.MaxPoints); + } + this.imageLock = &sync.Mutex{}; // Inherit submission limit from course or leave nil. diff --git a/model/assignment_load.go b/model/assignment_load.go index 6bb4c187..251d9c4f 100644 --- a/model/assignment_load.go +++ b/model/assignment_load.go @@ -36,13 +36,10 @@ func ReadAssignmentConfig(course *Course, path string) (*Assignment, error) { return nil, fmt.Errorf("Failed to validate assignment config (%s): '%w'.", path, err); } - otherAssignment := course.GetAssignment(assignment.GetID()); - if (otherAssignment != nil) { - return nil, fmt.Errorf( - "Found multiple assignments with the same ID ('%s'): ['%s', '%s'].", - assignment.GetID(), otherAssignment.GetSourceDir(), assignment.GetSourceDir()); + err = course.AddAssignment(&assignment); + if (err != nil) { + return nil, fmt.Errorf("Failed to add assignment to course (%s): '%w'.", path, err); } - course.Assignments[assignment.GetID()] = &assignment; return &assignment, nil; } diff --git a/model/course.go b/model/course.go index a40aeaf9..7120e29d 100644 --- a/model/course.go +++ b/model/course.go @@ -139,6 +139,31 @@ func (this *Course) Validate() error { return nil; } +func (this *Course) AddAssignment(assignment *Assignment) error { + for _, otherAssignment := range this.Assignments { + if (assignment.GetID() == otherAssignment.GetID()) { + return fmt.Errorf( + "Found multiple assignments with the same ID ('%s'): ['%s', '%s'].", + assignment.GetID(), otherAssignment.GetSourceDir(), assignment.GetSourceDir()); + } + + if ((assignment.GetName() != "") && (assignment.GetName() == otherAssignment.GetName())) { + return fmt.Errorf( + "Found multiple assignments with the same name ('%s'): ['%s', '%s'].", + assignment.GetName(), otherAssignment.GetID(), assignment.GetID()); + } + + if ((assignment.GetLMSID() != "") && (assignment.GetLMSID() == otherAssignment.GetLMSID())) { + return fmt.Errorf( + "Found multiple assignments with the same LMS ID ('%s'): ['%s', '%s'].", + assignment.GetLMSID(), otherAssignment.GetID(), assignment.GetID()); + } + } + + this.Assignments[assignment.GetID()] = assignment; + return nil; +} + // Returns: (successful image names, map[imagename]error). func (this *Course) BuildAssignmentImages(force bool, quick bool, options *docker.BuildOptions) ([]string, map[string]error) { goodImageNames := make([]string, 0, len(this.Assignments)); diff --git a/model/sync.go b/model/sync.go index f06d10e4..2927a39e 100644 --- a/model/sync.go +++ b/model/sync.go @@ -1,11 +1,29 @@ package model -import ( - "fmt" -) - type LMSSyncResult struct { UserSync *UserSyncResult `json:"user-sync"` + AssignmentSync *AssignmentSyncResult `json:"assignment-sync"` +} + +type AssignmentSyncResult struct { + SyncedAssignments []AssignmentInfo `json:"synced-assignments"` + AmbiguousMatches []AssignmentInfo `json:"ambiguous-matches"` + NonMatchedAssignments []AssignmentInfo `json:"non-matched-assignments"` + UnchangedAssignments []AssignmentInfo `json:"unchanged-assignments"` +} + +type AssignmentInfo struct { + ID string `json:"id"` + Name string `json:"name"` +} + +func NewAssignmentSyncResult() *AssignmentSyncResult { + return &AssignmentSyncResult{ + SyncedAssignments: make([]AssignmentInfo, 0), + AmbiguousMatches: make([]AssignmentInfo, 0), + NonMatchedAssignments: make([]AssignmentInfo, 0), + UnchangedAssignments: make([]AssignmentInfo, 0), + }; } type UserSyncResult struct { @@ -47,33 +65,6 @@ func (this *UserSyncResult) Count() int { return len(this.Add) + len(this.Mod) + len(this.Del); } -func (this *UserSyncResult) PrintReport() { - groups := []struct{operation string; users []*User}{ - {"Added", this.Add}, - {"Modified", this.Mod}, - {"Deleted", this.Del}, - {"Skipped", this.Skip}, - {"Unchanged", this.Unchanged}, - }; - - for i, group := range groups { - if (i != 0) { - fmt.Println(); - } - - fmt.Printf("%s %d users.\n", group.operation, len(group.users)); - for _, user := range group.users { - fmt.Println(" " + user.ToRow(", ")); - } - } - - fmt.Println(); - fmt.Printf("Generated %d passwords.\n", len(this.ClearTextPasswords)); - for email, pass := range this.ClearTextPasswords { - fmt.Printf(" %s, %s\n", email, pass); - } -} - func (this *UserSyncResult) AddResolveResult(resolveResult *UserResolveResult) { if (resolveResult == nil) { return; diff --git a/procedures/lms_sync.go b/procedures/lms_sync.go index ea4dc64e..7a7dd5f7 100644 --- a/procedures/lms_sync.go +++ b/procedures/lms_sync.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" + "github.com/eriq-augustine/autograder/common" "github.com/eriq-augustine/autograder/db" "github.com/eriq-augustine/autograder/lms" "github.com/eriq-augustine/autograder/lms/lmstypes" @@ -11,6 +12,25 @@ import ( "github.com/eriq-augustine/autograder/util" ) +func SyncLMS(course *model.Course, dryRun bool, sendEmails bool) (*model.LMSSyncResult, error) { + userSync, err := SyncAllLMSUsers(course, dryRun, sendEmails); + if (err != nil) { + return nil, err; + } + + assignmentSync, err := syncAssignments(course, dryRun); + if (err != nil) { + return nil, err; + } + + result := &model.LMSSyncResult{ + UserSync: userSync, + AssignmentSync: assignmentSync, + }; + + return result, nil; +} + // Sync users with the provided LMS. func SyncAllLMSUsers(course *model.Course, dryRun bool, sendEmails bool) (*model.UserSyncResult, error) { lmsUsersSlice, err := lms.FetchUsers(course); @@ -203,3 +223,104 @@ func getAllEmails(localUsers map[string]*model.User, lmsUsers map[string]*lmstyp return emails; } + +func syncAssignments(course *model.Course, dryRun bool) (*model.AssignmentSyncResult, error) { + result := model.NewAssignmentSyncResult(); + + adapter := course.GetLMSAdapter(); + if (!adapter.SyncAssignments) { + return result, nil; + } + + lmsAssignments, err := lms.FetchAssignments(course); + if (err != nil) { + return nil, fmt.Errorf("Failed to get assignments: '%w'.", err); + } + + localAssignments := course.GetAssignments(); + + // Match local assignments to LMS assignments. + matches := make(map[string]int); + for _, localAssignment := range localAssignments { + localID := localAssignment.GetID(); + localName := localAssignment.GetName(); + lmsID := localAssignment.GetLMSID(); + + for i, lmsAssignment := range lmsAssignments { + matchIndex := -1; + + if (lmsID != "") { + // Exact ID match. + if (lmsID == lmsAssignment.ID) { + matchIndex = i; + } + } else { + // Name match. + if ((localName != "") && (localName == lmsAssignment.Name)) { + matchIndex = i; + } + } + + if (matchIndex != -1) { + _, exists := matches[localID]; + if (exists) { + delete(matches, localID); + result.AmbiguousMatches = append(result.AmbiguousMatches, model.AssignmentInfo{localID, localName}); + break; + } + + matches[localID] = matchIndex; + } + } + + _, exists := matches[localID]; + if (!exists) { + result.NonMatchedAssignments = append(result.NonMatchedAssignments, model.AssignmentInfo{localID, localName}); + } + } + + for localID, lmsIndex := range matches { + localName := localAssignments[localID].GetName(); + changed := mergeAssignment(localAssignments[localID], lmsAssignments[lmsIndex]); + if (changed) { + result.SyncedAssignments = append(result.SyncedAssignments, model.AssignmentInfo{localID, localName}); + } else { + result.UnchangedAssignments = append(result.UnchangedAssignments, model.AssignmentInfo{localID, localName}); + } + } + + if (!dryRun) { + err = db.SaveCourse(course); + if (err != nil) { + return nil, fmt.Errorf("Failed to save course: '%w'.", err); + } + } + + return result, nil; +} + +func mergeAssignment(localAssignment *model.Assignment, lmsAssignment *lmstypes.Assignment) bool { + changed := false; + + if (localAssignment.LMSID == "") { + localAssignment.LMSID = lmsAssignment.ID; + changed = true; + } + + if ((localAssignment.Name == "") && (lmsAssignment.Name != "")) { + localAssignment.Name = lmsAssignment.Name; + changed = true; + } + + if (localAssignment.DueDate.IsZero() && (lmsAssignment.DueDate != nil) && !lmsAssignment.DueDate.IsZero()) { + localAssignment.DueDate = common.TimestampFromTime(*lmsAssignment.DueDate); + changed = true; + } + + if (util.IsZero(localAssignment.MaxPoints) && !util.IsZero(lmsAssignment.MaxPoints)) { + localAssignment.MaxPoints = lmsAssignment.MaxPoints; + changed = true; + } + + return changed; +} diff --git a/util/dirent.go b/util/dirent.go index c10c1a67..025f1edb 100644 --- a/util/dirent.go +++ b/util/dirent.go @@ -19,6 +19,10 @@ func CopyDirent(source string, dest string, onlyContents bool) error { return fmt.Errorf("Source dirent for copy does not exist: '%s'", source); } + if (IsSymLink(source)) { + return CopyLink(source, dest); + } + if (IsDir(source)) { return CopyDir(source, dest, onlyContents); } @@ -26,9 +30,33 @@ func CopyDirent(source string, dest string, onlyContents bool) error { return CopyFile(source, dest); } +func CopyLink(source string, dest string) error { + if (!PathExists(source)) { + return fmt.Errorf("Source link for copy does not exist: '%s'", source); + } + + if (IsDir(dest)) { + dest = filepath.Join(dest, filepath.Base(source)); + } + + MkDir(filepath.Dir(dest)); + + target, err := os.Readlink(source); + if (err != nil) { + return fmt.Errorf("Failed to read link being copied (%s): %w.", source, err); + } + + err = os.Symlink(target, dest); + if (err != nil) { + return fmt.Errorf("Failed to write link (target: '%s', source: '%s', dest: '%s'): %w.", target, source, dest, err); + } + + return nil; +} + // Copy a file from source to dest creating any necessary parents along the way.. // If dest is a file, it will be truncated. -// If fest is a dir, then the file will be created inside dest. +// If dest is a dir, then the file will be created inside dest. func CopyFile(source string, dest string) error { if (!PathExists(source)) { return fmt.Errorf("Source file for copy does not exist: '%s'", source); @@ -53,7 +81,11 @@ func CopyFile(source string, dest string) error { defer destIO.Close() _, err = io.Copy(destIO, sourceIO); - return err; + if (err != nil) { + return fmt.Errorf("Failed to copy file contents from '%s' to '%s': %w.", source, dest, err); + } + + return nil; } // Copy a directory (or just it's contents) into dest. @@ -118,16 +150,9 @@ func CopyDirContents(source string, dest string) error { sourcePath := filepath.Join(source, dirent.Name()); destPath := filepath.Join(dest, dirent.Name()); - if (dirent.IsDir()) { - err = CopyDirWhole(sourcePath, destPath); - if (err != nil) { - return err; - } - } else { - err = CopyFile(sourcePath, destPath); - if (err != nil) { - return err; - } + err = CopyDirent(sourcePath, destPath, false); + if (err != nil) { + return err; } } diff --git a/util/path.go b/util/path.go index 4af6f1ca..2e69ffe3 100644 --- a/util/path.go +++ b/util/path.go @@ -90,6 +90,24 @@ func IsEmptyDir(path string) bool { return false; } +func IsSymLink(path string) bool { + if (!PathExists(path)) { + return false; + } + + stat, err := os.Stat(path); + if (err != nil) { + if os.IsNotExist(err) { + return false; + } + + log.Warn().Err(err).Str("path", path).Msg("Unexpected error when checking if a path is a symbolic link."); + return false; + } + + return (stat.Mode() & fs.ModeSymlink) != 0; +} + func FindFiles(filename string, dir string) ([]string, error) { return FindDirents(filename, dir, true, false, true); }