senior-knights / course-schedulizer

📝 Create semester schedules without stress
https://senior-knights.github.io/course-schedulizer/
MIT License
10 stars 4 forks source link

Reorder the columns of the CSV output #250

Closed rpruim closed 1 year ago

rpruim commented 1 year ago

The schedules will need to be hand entered into Workday. Workday has a long form for this, and it will greatly speed up the process for data entry if we can put our csv columns in an order that mirrors the data entry form. Here is what Melisa Hubka suggests:

Here’s the list of columns in the preferred order:

Section name: MATH 171 A Start/end dates Title Fac Load Section Capacity Sem Hours Notes to be shown to stu/faculty (if there are any)– this requires clicking on the section from Find Course Sections Delivery mode: in person, online, hybrid (rare) Instructor(s) Meeting Pattern – which is days then time

Instructional format defaults from the course (lecture, lab, etc) so we don’t need that information – or it could be to the far right.

rpruim commented 1 year ago

Raymond has requested another reordering:

Term
Section name: MATH 171 A
Start/end dates
Title
Fac Load
Sem Hours
Notes to be shown to stu/faculty (if there are any)– this requires clicking on the section from Find Course Sections
Delivery mode: in person, online (syn/async), hybrid (rare)
Instructor(s)
Meeting Pattern – which is days then time
Room
kvlinden commented 1 year ago

The "Teaching Loads" tab - "Other Duties" column get messed up in the production branch now and I suspect that this column-output selection issue is the change that messed things up. To reproduce the error:

  1. Read this 2023 CS schedule
  2. Go to the "Teaching Loads" tab and note the stuff in the "Other Duties" and "Other Hours" columns.
  3. Write it out and read it back in, and then revisit the other duties/hours.

The import code appears to be keying off of the "SectionName" column (in a strange manner). Adding that field to the CSV and setting it to "--" signals an other duties/hours entry, using the InstructionalMethod column as the other duty name. But note that when that column is added, all the section names for all the courses must be included, otherwise they all get moved to the other duties/hours column.

The workaround is painful: emport the CSV and then manually add the SectionName column with the appropriate values, i.e., either "--" or =CONCATENATE(C17,"-",D17,"-",E17)

rpruim commented 1 year ago

Ack.. I'm guessing there was no test to alert me to this and I didn't catch the non-teaching duties bit.

I'd like to avoid redundant information in the exported CSV since that makes it much more brittle/confusing to edit outside the tool, so I suggest we fix the import to handle other duties without the SectionName column. The question is how. Some possibilities:

  1. create an internal column doing what SectionName does, but don't export it.
    • looks like the concatenation will just become "--" -- that's probably how this arose in the first place
    • this isolates the fix to single location and should ensure all the downstream stuff works
    • this also means imports of currently broken exports should "just work"
  2. introduce a new column to indicate that something is non-teaching (basically a flag)
  3. put data into one of the existing columns (probably SubjectCode) to indicate non-teaching
  4. change in internal check for for non-teaching duties to look for empty SubjectCode, CourseNum, and SectionCode

There may be other fixes as well. Thoughts?

I might see how easy it would be to implement #1, since that would likely be our quickest fix.

rpruim commented 1 year ago

Starting dig into the code a bit. Just documenting the process a bit...

I'm seeing

     const sectionNameStr = `${course.prefixes.length ? course.prefixes[0] : ""}-${
        course.number
      }-${section.letter}`;

in scheduleToFullCSVString ().

This suggests (?) that SectionName is not used internally in very many places. At least it isn't the primary data for export. So perhaps the short-term fix is to figure out where it is used for other duties and get that code to rely on the primary fields.

rpruim commented 1 year ago

Now seeing this function:

export const getSectionName = (course: Course, section: Section) => {
  return `${course.prefixes[0]}-${course.number}-${section.letter}`;
};

Seems like (a) it should be updated to include the course.prefixes.length check and (b) it should replace any place that body of the function is duplicated elsewhere.

rpruim commented 1 year ago

This callback function is using the SectionName string to determine whether the item is a non-teaching item.

export const sectionCallback = (value: string, params: CaseCallbackParams) => {
  if (value === "--" || value.trim() === "") {
    params.section.isNonTeaching = true;
  } else {
    const sectionParts = value.split("-");
    prefixCallback(sectionParts[0], params);
    numberCallback(sectionParts[1], params);
    letterCallback(sectionParts[2], params);
  }
};

But I'm guessing it doesn't get called if the CSV doesn't have a SectionName field:

// Iterate through the fields of the CSV, and parse their values for this object
    // TODO: Create a sense of priority for MeetingDurationMinutes over MeetingTime and SemesterLength over SemesterEndDate - SemesterStartDate
    if (fields) {
      fields.forEach((field) => {
        const value = String(object[field]);
        field = field.replace(/\s/g, "").replace("(", "").replace(")", "").replace("-", "");
        if (field in callbacks) {
          callbacks[field as keyof ValidFields](value, { course, meetings, section });
        }
      });

      // Insert the Section to the Schedule, either as a new Course or to an existing Course
      section.meetings = meetings;
      insertSectionCourse(schedule, section, course);
    }
rpruim commented 1 year ago

@kvlinden, I think I have a reasonable fix for this. The core pieces are


export const getSectionName = (course: Course, section: Section) => {
  return `${course.prefixes.length ? course.prefixes[0] : ""}-${course.number}-${section.letter}`;
};

// if isNonTeaching hasn't already been set, infer it from the 
// section name (computed from prefix, course number, and section letter all being empty)
export const isNonTeaching = (course: Course, section: Section) => {
  return section.isNonTeaching || getSectionName(course, section) === "--";
};

and in instertSectionCourse():

    // Otherwise, add the new section to the existing course
    else {
      section.isNonTeaching = isNonTeaching(course, section);
      schedule.courses[existingCourseIndex].sections.push(section);
    }

When a new section is added (as when you are reading in the CSV), isNonTeaching() will calculate the value of section.isNonTeaching if it doesn't already exist by computing the SectionName string and comparing to "--".

I think I followed your reprex instructions above in my branch, and it appears to be working. Can you confirm it works as inteded?

branch = bugfix/section-name-for-other-duties

rpruim commented 1 year ago

If we confirm this is working, it would be good to get this into production ASAP since I think some people will be using this today and tomorrow. (I know of at least on chair who has told me such.)

rpruim commented 1 year ago

I'll create a PR in a moment.

jmw-75 commented 1 year ago

Based on the competition by professor Pruim and our testing, we believe this issue should be closed