Attachment categories #1983
Attachment categories #1983
Conversation
reviewpad
bot
added
large
There was a problem hiding this comment.
Code overall LGTM!
One question I have is: will the category_name only be used for released/unreleased? Or are there other use cases I should be aware about?
If category_name is only used for released/unreleased, then maybe another name for the field would be more intuitive (e.g. released_status, tag, etc.)
Testing
- Only see course attachments on course landing page
- Only see assessment attachments inside each assessment
- Category (unreleased) shows up if attachment not released
- Creating attachment in both course + assessment
- Deleting attachment from edit attachment page
- Attachment groups
- Attachments sorted by release date
Frontend Nitpicks
For some reason, my attachment names always have a bullet point on them?
Also, if the name is too long then it'll shift the edit button down in this weird format
In the case that the label overflows to the next line, I think it would be neater to have the button left-aligned with the download button instead of what's shown in the screenshot
Inside the edit page the save changes and delete buttons have different stylings
Similarly, upload and save have different styles too.
UX Considerations
Unused Category
In creating an attachment within an assessment, we now have to put in a category, but the category won't show up anywhere:
Can we hide the category field when creating an attachment within an assessment?
Delete Button
The location of the delete inside the edit attachment page feels like a cancel button (which we should also add, if possible?). Also, the coloring of the delete button makes it seem like a primary action within the page, but the primary action of the edit page should only be saving the changes. I would recommend something like the following:
In general, though, I think having delete be part of the edit functionality isn't very great (here's a stack exchange post, LOL). Which sort of ties in to the next point:
Clicking on Attachment
Since the new attachment groups use the same format as the assessment categories, it was weird that I couldn't click into each attachment from the groups (I'm assuming this wasn't a bug?).
In the regular assessment attachments, clicking on the attachment name automatically downloads the file. Could we do the same for the new course attachments, remove the download button -> clicking attachment will automatically download?
Then, you can align the edit/delete buttons to the right in course attachments so it's consistent with assessment attachments, which look like the following:
Thanks for reading through this long comment, and let me know if you have questions!! Amazing PR as per always Damian :>>
category_name is used for the category name displayed for each list, e.g. "General" or "Writing". released/unreleased is determined from
Fixed (removed bullet point, ensure that the buttons stick together) |
Fixed by giving Also added the same styling for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Done (it will automatically default to "General") |
Removed delete button, added a "cancel" button
This was on purpose, due to the difficulty of having nested links. However, I've updated the logic so that this is how it works for students
For instructors, I've made the name clickable, removed the download button, aligned the edit/delete buttons to the right
|
|
I know there were previous discussions about this above, but what are your thoughts about keeping the course card clickable to download but still including the download icon to make it clear that clicking it will result in a download?
|
|
Also currently for instructors when looking at all attachments via
|
|
nit: seems like text and the icons aren't vertically aligned completely |
|
Nit: can you add more vertical space between the create attachment button and the text about the release date? |
|
Functionally LGTM, nice test cases! Some visual things that seem a bit off, but nothing big. |
WalkthroughWalkthroughThe overall change involves a significant update to the attachment handling system in a Ruby on Rails application. The updates include refactoring controller logic, adding new database columns for attachment categorization and release timing, removing obsolete columns, and updating views and tests to align with these changes. The focus is on improving the management of attachments by introducing more granular control over their categorization and visibility based on release dates and user roles. Changes
TipsChat with CodeRabbit Bot (
|
For students, added download icon:
Updated so that only course attachments get displayed
Aligned now, but for simplicity I moved the icons to the left (
Added margin of 20px |
|
Changes LGTM |
* Add category_name field and update course attachment UI * Improve styling of list items * Remove anchor link for unreleased badge, simplify delete button logic * Hide assessment attachments from course landing page * Add release_at field, remove released field * Fix tests * Add fixtures * Simplify variable names * Remove bullet points * Group buttons together * Make font-family consistent * Hide category for assessment attachments * Add cancel button, remove delete button, improve styling * Improve migration to be backwards compatible / reversible * Use update instead of update_attribute * Display when attachment will be released * Update tests * Simplify code * Use Time instead of DateTime * Add download icon for students * Vertically align icons * Hide assessment attachments from course attachment index * Add vertical space above release date (cherry picked from commit 70a8986)
* Lint views/submissions (#1969) * Begin linting views/submissions * finish linting views/submissions * address issues in code_viewer * Prevent spoofing the author of an annotation (#1985) * Prevent spoofing the author of an annotation (cherry picked from commit d2ab510) * Remove submitted_by from createAnnotation * Update link to docs in PR template (#1991) Fix link to docs * Hide irrelevant cud fields for students (#1988) * Show edit CUD button for students * Hide irrelevant CUD fields from students * Lint views/autograders, fix help-block gap from input (#1963) * lint views/autograders, fix help block gap from input * update form path * Lint views/announcements and Touch-up UI (#1957) * lint views/announcements and touch-up UI * address nits * Add erblint to overcommit and github actions (#1994) * update erb-lint config, overcommit config to enable erb-lint as a pre-commit hook, run erblint --lint-all * update github actions to run erb-lint during linting phase * update pull request template to include erblint check * Display Grace Day usage on submission history table, improve management of assessment penalty settings (#1990) * Display of grace days used * Fix calculation of effective_late_penalty and effective_version_penalty * Show course default values when applicable * Show warning messages when late submissions allowed but config does not make sense * Fix tests * Update wording * Improve formatting * Revert changes to effective penalties * Simplify check * Add toggles * Update wording on courseFields * Fix version threshold logic * Correctly set version threshold to blank when using course default * Clear / default values when checkbox clicked * Remove bottom padding * Improve UI when checkboxes selected * Address AI nits * Handle malformed scoreboard results from autograder, fix error handling for scoreboards (#1982) * begin fixing broken redirects * add code to check that entries are arrays, return flash error if not valid entry * fix spacing * address nit * Add logging * Click into submissions from gradebook score (#1998) * Clickable gradebook scores * Only scores have links --------- Co-authored-by: kestertan <kestert@andrew.cmu.edu> * Switch mossnet clean to use rails root instead of tilde expansion (#1997) use Rails root join function instead of ~/ to make sure moss clean script works across systems * Merge pull request from GHSA-h8wq-ghfq-5hfx * fixes * Add validation for handout, writeup, and handin_directory * Avoid use of and * Check that handout/writeup exists before checking path (#2001) Move present? check to front * Adds warning when assessment.rb file upload isn't a .rb file (#1999) * preliminary working version * only validates .rb files --------- Co-authored-by: Damian Ho <damian_ho_xu_yang@yahoo.com> * Refactor Assessment name rules, remove config file requirement (#1987) * begin refactoring naming rules for assessments * continue working on file acceptance * add testing * fix autograde * work on backwards compatibility / revertibility * keep working on implementing revertability * Fix some code creating assessmentConfigFile before assessment id created * Add documentation to naming rules * add line about assessment name uniqueness * update error messages * fix tests * add error handling code to redirect user in case assessment config file can't be loaded, run robocop * address AI code review * remove redundant flash * Fix text * Fix reload assessment config button text * Add more error handling, revamp regex string to better reflect valid ruby module names, add better sanitization for display name -> name conversion, fix docs to reflect actually valid assessment names. * fix test * Address nits * Fix issue where assessment could affect another assessment's config file if they both had names that mapped to pre-PR config file name * Delete config/oauth_config.yml * Delete diff.patch * Delete assessment.patch * remove unnecessary files * more removals * Suppress confirmation dialog on edit assessment page when no changes made (#2004) * Extract logic, call functions directly * Remove extraneous space * Remove another extraneous space * Display submission version in gradebook (#2005) * First Commit: version info is on gradebook as new columns * Second commit: only add a ver column after each assignment * Delete database.docker.yml * Delete schema.rb * Deleted debug code * change gitignore to original version * Address nits * Fix tooltips * Simplify version logic * Stop overwriting headerCssClass * Fix tooltip for not_yet_submitted * Handle nil aud * Add version to gradebook CSV export * Render tooltips onMouseEnter too * Simplify version header * Simplify logic * Increase gradebook width --------- Co-authored-by: SimonMen65 <jm7828@nyu.edu> Co-authored-by: Simon Men <60764463+SimonMen65@users.noreply.github.com> * Don't clear assessment penalty fields on initial load (#2006) * Don't clear on initial load * Remove extraneous spaces * No line breaks when generating base64 strings (#2008) * fix bug for long strings * Update base64.js using new TextEncoder * Show all courses for MOSS (#2015) * Show all courses, restore filter * Address AI nits * Fix use of autocomplete attribute * Add newline * Simplify toggleOptions implementation * Fix style of isArchive checkboxes * Correct use of javascript_include_tag * Fix failing test * Update styling of warning * Extract dropdown logic, use OR for filtering * Add newline * Add spacing between dropdowns * Use find instead of children, check for selector existence * Removed name from assessment yml (#1993) * Removed name from assessment yml * Modified test after removing name from assessment yml * Removed unnecessary test for wrong assessment name * Removed yml name check in assessments_controller --------- Co-authored-by: Nicholas Clark <nicholas@califoriaclarks.com> * Account for hooks in viewFeedback instead of feedback output (#2003) * preliminary working version * resolve merge conflicts * use submission.scores instead of feedback array * don't show non autograded scores in autograded scores tab * rabbit ai suggestions * more rabbit.ai nits * make finishedAutograding not an instance variable * Remove element overlapping scrollbar hitbox (#2009) * Remove element overlapping scrollbar hitbox * Move style to annotation.scss --------- Co-authored-by: Damian Ho <damian_ho_xu_yang@yahoo.com> * Attachment categories (#1983) * Add category_name field and update course attachment UI * Improve styling of list items * Remove anchor link for unreleased badge, simplify delete button logic * Hide assessment attachments from course landing page * Add release_at field, remove released field * Fix tests * Add fixtures * Simplify variable names * Remove bullet points * Group buttons together * Make font-family consistent * Hide category for assessment attachments * Add cancel button, remove delete button, improve styling * Improve migration to be backwards compatible / reversible * Use update instead of update_attribute * Display when attachment will be released * Update tests * Simplify code * Use Time instead of DateTime * Add download icon for students * Vertically align icons * Hide assessment attachments from course attachment index * Add vertical space above release date * Passwordless temporary login (#1984) * Passwordless temporary login created * Login using devise * User is not signed in before changing password * Removing unneeded files * Removing changes to user.rb * Removing unneeded files * Resetting password does not log you out * Added mailer * Added/removed newlines * Changed naming * Added checks for nil user or params * Error handling for passwords * Removed email after password reset * Added documentation * Updated documentation * Moved documentation to features * Renamed to admin-features * Added link in mkdocs.yml * Visual cue for assessments (#2016) * Add dates to assessment card * Add CSS formatting for date * Fix margin and card sizes to be more pretty * Show all students on gradesheet (#2019) * add course members with blank info if no submissions found * add email for no submission users * update bg color * Move submission version logic to be handled by AUD (#2024) * Move submission version logic to be handled by AUD * update migration variable naming * fix unit tests, version number for new auds * fix coderabbit issues * add version number to schema * change schema timestamp * Use ActiveStorage for attachments, add attachment size limit (#2023) * 1810 Use ActiveStorage for attachments * 1864 Add backwards compatibility to ActiveStorage Attachments * 1872 Add size limit to attachments * Set mime_type * Remove require * redirect to index on error * Rails 6.1.7.6 Migration (#2037) * Initial update to 6.1.7.6 * Lock fomantic-ui-sass to 2.8.8.1 * Update schema.rb * Avoid locking setup-ruby version * Include net-http in Gemfile to avoid errors * Run rubocop * Lock uri to 0.10.0 * Fix lint issue * Fix course_number values for roster export * Use flash for drop warning * Properly display submission errors * Only show invalid assessment warning to instructors * Ensure gradebook search bar renders correctly for CAs * Filter by lecture too when CA views section * Only show missing submissions from section if CA filters by section * Update tests * Better handling for submission errors * More specific error handling for save_entries * Better error display for statistics page * Return 404 for popover on non-existent submission * load Archive in files that use its methods * Update Ruby to 3.2.2, Misc fixes (#2040) * Update Ruby to 3.2.2 - update Capybara config so that it works with new ruby version and so that js can be enabled again on selenium test - update releaseSectionGrades redirect to go to viewGradesheet for CA's section - add some more status text / more informative flash when instructor drops student - redact tango key in getjob * Address nits, update bundler / github integration * add arm64-darwin-23 to platforms * fix users nit * Bump uri from 0.10.0 to 0.10.3 (#2039) Bumps [uri](https://github.com/ruby/uri) from 0.10.0 to 0.10.3. - [Release notes](https://github.com/ruby/uri/releases) - [Commits](ruby/uri@v0.10.0...v0.10.3) --- updated-dependencies: - dependency-name: uri dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update old migrations for Ruby 3 (#2044) Use splat on old migrations * Remove grading deadline (#2014) * Initial removal of grading_deadline * Add brackets around arguments to grading_complete? * Consistency fixes * Migration to remove grading deadline * Add guards to migration * Rename grading_complete? to grades_released? * Address issues from v2.8.0 testing, misc fixes/changes (#2038) * Fix command for promoting a user to admin * Extract aria/collapsible code, switch to path helpers * Automatically open first accordion * Fix docs for Tango info endpoint * Create user directory on autograde_done * Coalesce accordions, simplify js, remove admin options * Update doorkeeper translations * Remove extraneous quotes * Remove redundant / useless assessment nil check * Fix error display when calling downloadAll on invalid assessment * Simplify failure redirect logic for downloadAll * Deduplicate logic for autograde feedback path and handin file path * Remove unused ass_dir variable * Remove redundant gitignores * Uncoalesce accordions * Fix redirects for invalid assessment Previously, calling downloadAll with an invalid assessment led to infinite redirects * Update the API to allow retrieving group members (#1956) * Add a param to the index groups api to retriee group members * Add api show endpoint for groups * Update docs * Update api docs for groups#show * Compact group members api response * Move fetching group json to a private method * Remove empty line --------- Co-authored-by: Damian Ho <damian_ho_xu_yang@yahoo.com> * Update index and show docs for Groups API (#2045) Update index and show docs * Main Table UI Changes (#1886) * Start Manage Submissions * Center checkbox in manage submission table (#1868) fix checkbox issue * Main Table UI * Updates with selecting students and buttons * Add Score Popup Icon * Icon spacing and codebase style --------- Co-authored-by: Michelle Liu <mliuu3259@gmail.com> * Add sorting icons to new manage submissions (#1890) * change icon to swap_vert, hide for file and actions headers * change icon on diff sort * Adds Score Details (#1893) * Adds score details without styling * address general styling for score details * refactor code * address pr issues * bring back css * bring back div * add back class names * add back icons * addressed nits * address nits --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Joey Wildman <josephwildman88@gmail.com> Co-authored-by: Nicholas Myers <32116122+NicholasMy@users.noreply.github.com> Co-authored-by: Damian Ho <damian_ho_xu_yang@yahoo.com> Co-authored-by: Kester <kestertan040@gmail.com> Co-authored-by: kestertan <kestert@andrew.cmu.edu> Co-authored-by: SimonMen65 <jm7828@nyu.edu> Co-authored-by: Simon Men <60764463+SimonMen65@users.noreply.github.com> Co-authored-by: Ugo <7947217+ugogon@users.noreply.github.com> Co-authored-by: Nicholas AJ Clark <nicholas@californiaclarks.com> Co-authored-by: Nicholas Clark <nicholas@califoriaclarks.com> Co-authored-by: lykimchee <lchaeryn5863@gmail.com> Co-authored-by: Joanna Ge <45646252+jlge@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Umar Alkafaween <30433769+umar221b@users.noreply.github.com> Co-authored-by: Victor Huang <victorhuangwq@gmail.com>
Summary
Summary generated by Reviewpad on 10 Nov 23 20:36 UTC
This pull request includes the following changes:
The addition of a new file called "attachment.txt" in the "spec/fixtures/attachments" directory. The content of the file is "Default attachment" and there is no newline at the end of the file.
The addition of a new file called "assessment.txt" in the "spec/fixtures/attachments" directory. The content of the file is "Assessment attachment file" and there is no newline at the end of the file.
Changes in the
AttachmentsControllerrelated to theindexaction, theset_attachmentmethod, and theattachment_paramsmethod.Changes in the
style.css.scssfile related to thefont-familystyle properties.The addition of a new file called
attachments.rbin thespec/factoriesdirectory, which defines a factory for theAttachmentmodel with various attributes.The addition of a new file called "course.txt" in the "spec/fixtures/attachments" directory. The content of the file is "Course attachment file" and there is no newline at the end of the file.
Changes in the
show.html.erbfile related to the display of attachments.Changes in the
assessments_controller.rbfile related to the handling of attachments.A new migration file called "remove_released_from_attachments.rb" that removes the
releasedcolumn from theattachmentstable.Changes in the
controller_macros.rbfile related to the creation of attachments usingFactoryBot.Changes in the
_attachment.html.erbtemplate file related to the display of attachments.A new migration file called "add_category_name_to_attachments.rb" that adds a
category_namecolumn to theattachmentstable.Changes in a file related to an attachment form, including JavaScript code and HTML modifications.
Changes in the
app/views/assessments/index.html.erbfile related to the display of assessments and attachments.Changes in the
db/schema.rbfile, including an update of the schema version and the addition of columns to theattachmentstable.Changes in the
Attachmentmodel, including validations, scopes, and methods related to category name, release date, assessments, and file uploads.A migration file named "20230924161219_add_release_at_to_attachments.rb" that adds a
release_atcolumn to theattachmentstable.A new file called
_course_attachment.html.erbthat contains HTML code for displaying attachments on a course homepage.Please review these changes and provide your feedback.
Description
category_nameandrelease_atfields, removereleasedfielddevelopMotivation and Context
Improves attachments feature by adding categories (to group related attachments), and release date (to automatically release attachments).
This facilitates the distribution of course material. 122 previously used assessment writeups for this purpose, but this is clunky since it requires multiple clicks to download the files, whereas attachments provide a one-click download functionality.
Note: unreleased attachments will be set to be released one year in the future
How Has This Been Tested?
Note: mostly covered by tests
Create two attachments under different categories, one unreleased and one released:
Student view
Clicking anywhere in the box downloads the attachment
Instructor view
Clicking on the name downloads the attachment
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting