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

Lint views/admins, views/application, views/assessment_user_data, views/attachments #1967

Merged
merged 2 commits into from
Sep 17, 2023

Conversation

20wildmanj
Copy link
Contributor

@20wildmanj 20wildmanj commented Aug 21, 2023

Summary

Summary generated by Reviewpad on 17 Sep 23 15:43 UTC

This pull request includes the following changes:

  • In the maintenance.html.erb file, the indentation of the code has been fixed.
  • In the edit.html.erb file, the form has been refactored to use hash syntax instead of hash rocket syntax for options.
  • In the _attachment.html.erb file, the link_to paths have been refactored to use the named route helper methods.
  • In the _form.html.erb file, the indentation and spacing of the code has been adjusted for better readability.
  • In the index.html.erb file, the link_to paths have been refactored to use the named route helper methods.
  • In the new.html.erb file, the form has been refactored to use hash syntax instead of hash rocket syntax for options.

Please review these changes and provide any necessary feedback.

Description

  • Lint views/admins, views/application, views/assessment_user_data, views/attachments using bundle exec erblint app/views/*(folder)/*(file).html.erb -a
  • Replace bracket links with function (_path()) links
  • Remove script in app/views/attachments/_form.html.erb seemed to previously update the file name next to the upload file button, which is now redundant with the new file_field button

Motivation and Context

  • As part of Summer linting

How Has This Been Tested?

  • Ensure links for attachments (both course and assessment) work, i.e. editing, deleting, downloading
  • Ensure that link to course/assessment from attachment overview works (go to <course>/attachment or <course>/<assessment>attachment and click top link)
  • Ensure that "Bulk Email Instructors" looks correct
  • Ensure that assessment_user_data page renders (Go to gradesheet and click on link as shown below)
Screenshot 2023-08-20 at 8 26 05 PM

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting

@reviewpad reviewpad bot requested a review from lykimchee August 21, 2023 00:33
@reviewpad reviewpad bot added medium Pull request is medium waiting-for-review labels Aug 21, 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.

  • Creating, editing, deleting, downloading coursewide and assessment attachments work
    NOTE @20wildmanj: Can we add a confirmation step for users before as they delete an attachment? I still approved the PR so it's okay if we don't add it, but would be nice!

  • Able to get from attachment overview (both coursewide and assessment) to the course or assessment

  • Bulk email instructors looks good

Screenshot 2023-09-17 at 11 30 19 AM
  • Can see assessment_user_data from gradebook
Screenshot 2023-09-17 at 11 31 21 AM

@20wildmanj 20wildmanj added this pull request to the merge queue Sep 17, 2023
Merged via the queue into master with commit 8a34dac Sep 17, 2023
6 checks passed
@20wildmanj 20wildmanj deleted the joeywildman-lint-views-admins-application branch September 17, 2023 15:48
NicholasMy pushed a commit to UB-CSE-IT/Autolab that referenced this pull request Jan 5, 2024
…ws/attachments (autolab#1967)

* Lint views/admins, views/application, views/assessment_user_data, views/attachments

* add deletion confirmation

(cherry picked from commit 8a34dac)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium Pull request is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants