Skip to content
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.

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

Fix roster format #1557

Merged
merged 3 commits into from
Jul 13, 2022
Merged

Fix roster format #1557

merged 3 commits into from
Jul 13, 2022

Conversation

damianhxy
Copy link
Member

@damianhxy damianhxy commented Jul 11, 2022

Description

  • When exporting roster, use @course.name for the missing courseNumber column
  • Make ROSTER_COLUMNS_F16 and ROSTER_COLUMNS_F20 use Lecture (rather than Course) for courseLecture field.
  • Added entire ROSTER_COLUMNS_F16 and ROSTER_COLUMNS_F20 formats in comments
  • Map map[0] (Semester) to -1 since it is not used
  • Fix mapping for grading_policy in ROSTER_COLUMNS_F16 format

Motivation and Context

The "General Autolab Format" is described by Semester,email,last_name,first_name,school,major,year,grading_policy,courseNumber,courseLecture,section.

Inside parse_roster_csv, the corresponding fields are extracted (except for Semester and courseNumber, since the data is stored in @course). However, when exporting a roster, the courseNumber column is not outputted, which causes the columns to shift if we reimport the data (courseLecture will now take on the value of section, and section will be blank). This PR fixes this by using @course.name for the courseNumber column.

Furthermore, when processing Roster formats ROSTER_COLUMNS_F16 and ROSTER_COLUMNS_F20, the Course field, instead of the Lecture field, is mapped to courseLecture, meaning that we are recording the Course name (e.g. 15122) as the Lecture number (which should be 1, 2, etc.). In older format ROSTER_COLUMNS_S15, the courseLecture was correctly mapped to the Lecture field.

Fixes #997.

How Has This Been Tested?

Test 1: Ensure General Autolab Format still works correctly

Roster

M22,xho@foo.bar,Ho,Damian,SCS,CS,2025,L,15122,1,A

Screen Shot 2022-07-12 at 12 42 06

Test 2: Check that Export + Import works correctly

Roster (Note the 4 undropped students)

Screen Shot 2022-07-11 at 14 30 45

Before PR: Export + Import

Observe that the section letter shifted into the lecture column.

Screen Shot 2022-07-11 at 14 31 42

After PR: Export + Import

Screen Shot 2022-07-11 at 14 32 03

Test 3: Check that CMU Roster Format works correctly

Roster

Semester,Course,Section,Lecture,Mini,Last Name,Preferred/First Name,MI,Andrew ID,Email,College,Department,Major,Class,Graduation Semester,Units,Grade Option,QPA Scale,Mid-Semester Grade,Primary Advisor,Final Grade,Default Grade,Time Zone Code,Time Zone Description,Added By,Added On,Confirmed,Waitlist Position,Units Carried/Max Units,Waitlisted By,Waitlisted On,Dropped By,Dropped On,Roster As Of Date
M22,15122,A,1,N,Ho,Damian,,xho,xho@andrew.cmu.edu,SCS,CS,CS,3,F25,0,L,4+,,Test Advisor,,,EST,Eastern Standard Time (GMT-5:00),fake_andrewid,11 Jul 2022,Y,,,,,,,11 Jul 2022 12:00 AM

Before PR

Observe that Course number appears in the lecture field.

Screen Shot 2022-07-11 at 14 39 39

After PR

Screen Shot 2022-07-11 at 14 39 14

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

Copy link
Contributor

@fanpu fanpu left a comment

Choose a reason for hiding this comment

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

Great work catching this and the changes looks good! Could you also update the comments to include the subsequent columns that were elided so people will be able to know the entire roster format in the future?

- Avoid mapping semester (since it's unused anyway)
- For ROSTER_COLUMNS_F16 format, fixed mapping for grade option
# Section(10), ...
# Semester(0 - skip), Email(1), Last Name(2), First Name(3), School(4),
# Major(5), Year(6), Grade Policy(7), Course(8 - skip), Lecture(9),
# Section(10)
return parsedRoster
Copy link
Member Author

Choose a reason for hiding this comment

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

To be fully consistent with what we're doing with SIO rosters (i.e. dropping the Semester and Course columns here, even though they will be ignored later on anyway), we should technically use a "map" here [-1, 1, 2, 3, 4, 5, 6, 7, -1, 9, 10]. However, this doesn't make a difference unless for some reason we make use of those columns in the future.

@fanpu
Copy link
Contributor

fanpu commented Jul 13, 2022

Updated changes looks good, don't worry about the stuff from S15 too, no one should be using those now

@damianhxy damianhxy merged commit 818a736 into master Jul 13, 2022
@damianhxy damianhxy deleted the fix-roster-format branch July 13, 2022 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Roster export format differs from import format
2 participants