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

More detailed roster error messages & sort by account status #1402

Merged
merged 33 commits into from
Nov 20, 2021

Conversation

michellexliu
Copy link
Contributor

@michellexliu michellexliu commented Oct 20, 2021

Description

Updated the error messages to include more information about roster uploads and CSV parsing; added a "sort by account status" toggle that displays newly-added and newly-dropped accounts at the top of the confirmation screen.

Motivation and Context

This updated roster upload aims to address two main issues:

  1. Lack of detailed error messages - Multiple professors have had issues uploading their rosters due to a lack of detailed error messaging when a roster doesn’t satisfy Autolab’s parsing requirements.
  2. New & dropped students are difficult to locate - In large roster uploads, the user would need to scroll throughout the entire roster to locate which students have newly dropped or added the course (indicated by green or red text).

How Has This Been Tested?

(incomplete)

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:

  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

Other issues / help required

2 remaining rubocop errors:

app/controllers/courses_controller.rb:851:5: W: Lint/SuppressedException: Do not suppress exceptions.
    rescue StandardError
    ^^^^^^^^^^^^^^^^^^^^
app/controllers/courses_controller.rb:864:5: W: Lint/SuppressedException: Do not suppress exceptions.
    rescue StandardError

Not sure if removing the rescue StandardError from here will break anything, so I'd like to have someone else look at this as well.

@victorhuangwq
Copy link
Contributor

Just a quick first comment, but why are there such major changes in the CSS files?

@@ -85,3 +44,18 @@ assignments or download new assignments</font>
<% end %>

<% end %>

<script src="//cdnjs.cloudflare.com/ajax/libs/jquery-scrollTo/2.1.2/jquery.scrollTo.min.js"></script>
<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this is because this is still WIP, but it is preferable that scripts be moved to a separate corresponding Javascript file (with similar filename), instead of being included in the html file

@michellexliu
Copy link
Contributor Author

Just a quick first comment, but why are there such major changes in the CSS files?

looks like my vscode extension autoformatted the file when I saved it

@michellexliu michellexliu marked this pull request as ready for review November 13, 2021 18:56
Copy link
Contributor

@victorhuangwq victorhuangwq left a comment

Choose a reason for hiding this comment

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

LGTM! Tested using the test document, and with comma only lines, or lines without enough commas.

There's two other minor issues (that existed prior to this feature) discovered from testing @xinyis991105 :

  • If student have already been dropped before, it will still appear as red
  • If a student is dropped because it no longer exists on the roster. But then in a new roster it is added back again, the student is not "undropped" <- not sure if that's a good behavior
    We should add them to the backlog

@michellexliu michellexliu merged commit 5bf9ae7 into master Nov 20, 2021
@michellexliu michellexliu deleted the roster-fix branch November 20, 2021 18:15
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.

3 participants