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

Allow simultaneous creation of multiple extensions #1896

Merged
merged 10 commits into from
May 15, 2023
Merged

Conversation

damianhxy
Copy link
Member

@damianhxy damianhxy commented Apr 29, 2023

Description

  • Update autocomplete on manage extensions page to support multiple students at once
  • Update backend logic to support creating / updating multiple extensions at once

Motivation and Context

Currently, extensions can only be created or updated for one student at a time. This is tedious if multiple students should be given the same extension, as surfaced during 213 user testing.

Resolves #1854

How Has This Been Tested?

Creating extensions

e.g. give 1 day extensions to 3 users (user0, user1, user2)
note: already selected users are filtered from the dropdown, to prevent users from selecting them again
Screenshot 2023-05-14 at 16 11 01

Updating extensions

e.g. give infinite extensions to 3 users (user2, user3, user4) -> this updates user2's extension
Screenshot 2023-05-14 at 16 11 20

Autofill

  • Clicking on student email(s) automatically adds them to the list of selected students. A warning message is displayed if the student was already added (e.g. clicking or manual input)
  • If selected student(s) all have the same extension, the details will be autofilled. Otherwise, it displays default values

XSS

  • Create users with html tags in their names, ensure that the names are correctly escaped in the dropdown
    Screenshot 2023-05-07 at 23 51 16

Error conditions

  • Click create without any users selected -> "No users were specified!" error message
  • After selecting several students, modify the value of #course_user_data to include invalid cuds -> "No user with id xxx" was found for this course
  • Attempt to give extension of -1 days (after disabling min check) -> "Validation failed: Please enter (≥ 0) days of extension, or mark as infinite."

Ensure that when an error occurs, no changes are applied

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

@damianhxy damianhxy requested a review from lykimchee May 8, 2023 04:40
@damianhxy damianhxy marked this pull request as ready for review May 8, 2023 04:40
@damianhxy damianhxy changed the title Enable simultaneous creation of extensions for multiple students Allow multi-creation of extensions May 8, 2023
@damianhxy damianhxy changed the title Allow multi-creation of extensions Allow simultaneous creation of multiple extensions May 8, 2023
Copy link
Member

@lykimchee lykimchee left a comment

Choose a reason for hiding this comment

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

Testing:

  • Creating extension for multiple users
  • Autofill if all selected users have the same extension (from original task)
  • Autofill clears if all selected users have different extensions
  • Able to update extensions (for multiple users as well) (from original task)
  • Warning popup for already added users
  • No users specified error flash

The dropdown is super clean I love it!!! It looks like this task ended up being a lot more complicated and bigger than I initially thought, especially with you adding the security and input validation checks, tysm Damian!!

I had a couple questions that don't necessarily require changes, but:

  • I remember we had some discussion during one of the team meetings about a specific date format, are we updating the "Select a new due date" to be that format? (I don't remember what we ended up deciding for the finalized date format though, so let me know!)
image
  • Apologies for noticing just now and not when actually doing design overviews, but all three dates (including from the previous screenshot) on this page are a different format— do we want to do anything about this?
image image
  • Would you prefer having this merged to master or to feature/new-manage-submissions?

That's it for questions! This is beautiful I love the dropdown so much :'))

@damianhxy
Copy link
Member Author

damianhxy commented May 14, 2023

I remember we had some discussion during one of the team meetings about a specific date format, are we updating the "Select a new due date" to be that format? (I don't remember what we ended up deciding for the finalized date format though, so let me know!)
Apologies for noticing just now and not when actually doing design overviews, but all three dates (including from the previous screenshot) on this page are a different format— do we want to do anything about this?

In #1895, Michelle decided on a format, I believe in discussion with Victor. However, given that extensions are on a day-by-day basis, I feel that such precision is not required. I will standardize the formats to something like MMM D YYYY (e.g. Mar 11 2023). This is slightly different in that it uses a 4 digit year, but it feels more readable to me.

Would you prefer having this merged to master or to feature/new-manage-submissions?

I think this is a fairly standalone feature that can be merged to master, so that 213 can make use of it (unless new manage submissions will be merged to master soon?)

@damianhxy
Copy link
Member Author

Screenshot 2023-05-14 at 16 10 33

@lykimchee New standardized date format (PR description has also been updated). Let me know what you think of this, as well as merging into master!

@lykimchee
Copy link
Member

Merging into master sounds good to me!

I think the new date format looks good as well! I would prefer to keep the specific time for just the original due date though, in the format of Michelle's PR (except I like your year format better).

With this change, the MM-DD-YY part of the dates would still be consistent, even though the original due date would have an additional timestamp, so I don't think it would be confusing to look at— but let me know your thoughts though!

@damianhxy
Copy link
Member Author

Sounds good, I've made the change!

Screenshot 2023-05-15 at 14 27 02

@damianhxy damianhxy added this pull request to the merge queue May 15, 2023
Merged via the queue into master with commit c9c8a1b May 15, 2023
@damianhxy damianhxy deleted the multi-extensions branch May 15, 2023 14:29
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.

[Manage Submissions] Ability to enter multiple students in Manage Extensions
2 participants