[Feature:InstructorUI] Add numeric CSV upload support#12995
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12995 +/- ##
============================================
- Coverage 21.32% 21.29% -0.03%
- Complexity 10055 10071 +16
============================================
Files 278 278
Lines 37646 37697 +51
Branches 516 516
============================================
Hits 8028 8028
- Misses 29115 29166 +51
Partials 503 503
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I did a code review and a functionality review, and just noticed a couple things.
- Are you supposed to be able to edit data that is already set by a previous CSV upload? In this case I uploaded the CSV with the numeric component as 1, and then tried to upload again with the component as 2 instead. The component did not update with the new value.
- What happens if you try to upload a new student? The modal says information about entering a new student, but when I tried to enter a new student nothing happened.
Edit, here is what I am requesting to add:
- The warning message that displays in the screenshot should provide a row AND a column of where the error is. this should be the same for all other relevant warning messages.
- Update the modal to include instructions that the Total column must be filled.
- Update the modal to remove any info about adding new students, however, you can keep those headers and just say they are optional.
- Can we just remove the link / href from the download CSV button for now? Right now if you click it it redirects you to the homepage, it would be better if literally nothing happened when you click it.
A more significant refactor that may / may not be necessary is to update this upload feature to actually parse the headers of the CSV. The advantage of this would be that users could only upload the columns with headers that they want to edit. See the Manage Students page and PR #12601 for how this could be implemented. In my opinion, this would be a great QOL improvement.
Lastly, once we have a final implementation ready to go for this, let's do a functionality review / demo with Barb to verify that the original functionality is preserved.
github-project-automation
Bot
moved this from Seeking Reviewer
to Work in Progress
in Submitty Development
Thank you for the feedback, updated all of the changes requested. Should work properly now! |
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
roye2
left a comment
roye2
left a comment
There was a problem hiding this comment.
This seems to be working really well now, I was able to upload a CSV with only the user ID and the questions. Furthermore, since the column headers are being parsed, you can include any number of them and in any order which I think is great. I just have a couple small things in the code comments that I think could be addressed.
| <li>"Registration Subsection": Optional; included for readability and ignored by upload.</li> | ||
| </ol> | ||
| <p> | ||
| Columns must match the order of the numeric/text items shown in the grading table, and the "Total" column must be filled in for each row. |
There was a problem hiding this comment.
You actually don't need to fill in the Total column, and, now that the headers are parsed, you don't need to fill out the columns in order. So we can probably just remove that message. Additionally, I wonder if there is a better way to display what the actual content / grading columns should be formatted as. Maybe something like: "List each numeric / text column with the matching header from the grading table" could replace the messsage.
| }, | ||
| () => { | ||
| closePopup('numeric-csv-upload-form'); | ||
| window.location.reload(); |
There was a problem hiding this comment.
I think that once the columns upload, we should reload the page for the user so that they can see the changes right away. As it is right now, the new information doesn't display on the page until you manually press reload. It looks like this is maybe intended to happen but isn't working properly? I am not sure. One way that you could do this, is to parse the Json response from the PHP code. Then, you can display whatever that message is and reload on a success. Or, maybe there is some small modification to the existing code that you could do to achieve the same thing.
There was a problem hiding this comment.
Okay I changed the modal, along with adding a refresh. However, since this code uses JS I could not think of a way for the display message to appear after the refresh so I added an extra letting a potential user know what was updated and then stating that the page will be refreshing.
github-project-automation
Bot
moved this from In Review
to Work in Progress
in Submitty Development
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
roye2
left a comment
roye2
left a comment
There was a problem hiding this comment.
I tested the new message + reload and it works well, I like that it states which sections of the table are being edited. One last thing, let's add a space here :)
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
roye2
left a comment
roye2
left a comment
There was a problem hiding this comment.
Okay, with the most recent changes I think everything looks good, approving.
github-project-automation
Bot
moved this from In Review
to Awaiting Maintainer Review
in Submitty Development
|
@dandrecollins07-ctrl |
make the popup text for Upload CSV button clearer
Can be confusing because table is not visually updated until reload, which takes place a few seconds after this message.
|
Please see my PR #13007, which includes a fix to the broken cypress that's failing here, as well as my recommendations for wording changes and CSV header titles. |
…ggestions' into numeric-gradeable-csv-upload
Done! |
automateprojectmangement
Bot
moved this from Awaiting Maintainer Review
to In Review
in Submitty Development
There was a problem hiding this comment.
I have done a code and functionality review and I think it all looks good. One final comment that I have, do you think that it is worth mentioning to the instructor that the Total column (which will appear in the download), is not parsed by the upload feature and is instead calculated? Maybe we can change this message: "List each numeric or text column you want to update, with a header matching the column name shown in the grading table. Only the columns you include will be updated — any others will be left unchanged."
to something like "List each numeric or text column you want to update, with a header matching the column name shown in the grading table. Only the columns you want to include will be updated. The 'Total' column can be included for readability but will not be read and is instead calculated."
Or, we could put it in with the other parts of the modal about different optional columns. I guess that would be okay, but we don't list any other of the actual content columns in that section so it may be confusing.
|
I'm going to merge now |
### Why is this Change Important & Necessary? Currently, the old UI for numeric CSV upload was outdated and should match recent similar pages such as Managing Students. Fixes Submitty#12973 ### What is the New Behavior? - Adds an Upload CSV modal for numeric gradeables. - Validates required CSV headers (User ID, Given Name, Family Name). - Validates numeric grade values and totals. - Displays descriptive error messages for malformed CSVs. - Updates grades from the uploaded CSV. - Closes the upload modal after a successful upload and displays a success message. Images: <img width="821" height="408" alt="image" src="https://github.com/user-attachments/assets/a87f96a9-eaad-477e-be13-2d61e7b103fd" /> <img width="337" height="231" alt="image" src="https://github.com/user-attachments/assets/9253efff-bd2c-40ad-9093-131f505d0021" /> <img width="304" height="29" alt="image" src="https://github.com/user-attachments/assets/bdb7a2e1-028c-4fd3-b803-dc22ceede477" /> ### Automated Testing & Documentation 1. Run submitty_install_site. 2. Log in as an instructor. 3. Open or if needed create: a numeric/text gradeable. 4. Click "Upload CSV". 5. Verify: - Valid CSV with all columns uploads successfully, and the success message lists all updated columns. - CSV with only a subset of columns (e.g. User ID + one question) uploads successfully, updates only those columns, and leaves all other components unchanged. - CSV with columns in a different order than the grading table still uploads successfully. - Missing "User ID" header is rejected with a clear error. - A row with a blank User ID is rejected with the row number. - Invalid (non-numeric) values are rejected with the specific row and column identified. - Values exceeding a component's max value are rejected with the specific row and column identified. - Incorrect totals (when a "Total" column is included) are rejected with the row and column identified. - CSV with no "Total" column uploads successfully without a total-mismatch error. - Unrecognized/misspelled column headers are silently ignored (not applied), and do not appear in the "Updated: ..." success message. - A User ID not found in the course triggers a "User does not exist" alert without blocking the rest of the upload. - Grades and computed totals update correctly on screen immediately after upload, without requiring a page refresh. ### Other information Note: The Download CSV button currently does not do anything but it will be used for a future PR. Does this PR include migrations to update existing installations? N/A --------- Co-authored-by: Emma Roy <[email protected]> Co-authored-by: NicholasCiuica <[email protected]>
4 participants
Why is this Change Important & Necessary?
Currently, the old UI for numeric CSV upload was outdated and should match recent similar pages such as Managing Students.
Fixes #12973
What is the New Behavior?
Images:
Automated Testing & Documentation
Other information
Note: The Download CSV button currently does not do anything but it will be used for a future PR.
Does this PR include migrations to update existing installations?
N/A