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

Add button to bulk import assessments from file system #1931

Merged
merged 8 commits into from
Jul 17, 2023

Conversation

damianhxy
Copy link
Member

@damianhxy damianhxy commented Jun 24, 2023

Summary

Summary generated by Reviewpad on 16 Jul 23 19:01 UTC

This pull request includes several patches that make various changes to the codebase. Here is a summary of each patch:

  1. [PATCH 1/7] Update old-style path to use path helper:

    • Makes a modification in the assessments_controller.rb file to update the old-style path to use the path helper. This change updates the redirect_to method call.
  2. [PATCH 2/7] Add install all button:

    • Makes changes in the install_assessment.html.erb file to add an "Install all" button. The button triggers a script that sends a POST request for each import link and reloads the page after the import is done.
  3. [PATCH 3/7] Rename install -> import:

    • Renames the "install" class to "import" in the install_assessment.html.erb file.
  4. [PATCH 4/7] Improve logic:

    • Adds logic to display an alert message when the "Import all" button is clicked. The message indicates whether all assessments have been imported successfully, or if there were errors during the import process.
  5. [PATCH 5/7] Add alert:

    • Adds an alert message to inform the user about the completion of the import process and any errors encountered during the import of assessments.
  6. [PATCH 6/7] Rephrase message:

    • Modifies the alert message to rephrase the text when all assessments have been imported, but there are remaining assessments that encountered errors during the import process.
  7. [PATCH 7/7] Rename btn variable:

    • Renames the 'btn' variable to '$btn' for clarity in the install_assessment.html.erb file.

Please review these patches and provide any necessary feedback or comments.

Description

  • Update an old-style path to use a path helper
  • Improve UI of uninstalled assessments list
  • Add button to bulk import assessments

Motivation and Context

Currently, assessments can only be individually imported from the file system. This PR provides an option to import all of them at once.

Closes #1928

How Has This Been Tested?

Create a course and a few assessments. Delete all of the assessments.

  • Ensure that importing from file system still works
  • Ensure that import all button works
Screenshot 2023-06-25 at 02 58 16

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

@reviewpad reviewpad bot added the small Pull request is small label Jun 24, 2023
@damianhxy damianhxy marked this pull request as ready for review June 24, 2023 19:00
@reviewpad reviewpad bot requested a review from evanyeyeye June 24, 2023 19:00
Copy link
Member

@evanyeyeye evanyeyeye left a comment

Choose a reason for hiding this comment

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

Functionally everything works. However, on the frontend side the Import All button seems to remove only one assessment from the list per click, when it should remove them all.

To reproduce this:

  • Add and remove 2 assessments.
  • Click Import All.
  • Observe that only one assessment is removed from the list, but that both are added to the course. If you refresh the page or click the button again, the list will be empty.

@damianhxy
Copy link
Member Author

I've improved the javascript code so that the page only reloads after all ajax requests are complete

Copy link
Contributor

@20wildmanj 20wildmanj left a comment

Choose a reason for hiding this comment

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

LGTM besides comment on post-bulk-install notification

@damianhxy damianhxy added this pull request to the merge queue Jul 17, 2023
Merged via the queue into master with commit e885c67 Jul 17, 2023
6 checks passed
@damianhxy damianhxy deleted the bulk-import-asmt branch July 17, 2023 13:15
NicholasMy pushed a commit to UB-CSE-IT/Autolab that referenced this pull request Jan 4, 2024
* Update old-style path to use path helper

* Add install all button

* Rename install -> import

* Improve logic

* Add alert

* Rephrase message

* Rename btn variable

(cherry picked from commit e885c67)
@damianhxy damianhxy mentioned this pull request Mar 23, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small Pull request is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk import assessments from file system
3 participants