diff --git a/Changelog.md b/Changelog.md index 35e84309ad..a121d6e2d9 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,19 @@ # Changelog +## [v2.6.1] + +### ✨ New features and improvements + +- Give instructors the ability to delete a TA from the Users Graders Tab (#7304) +- Added zoom and rotate functionality to PDF viewer (#7306) + +### 🐛 Bug fixes + +- Ensure we handle JSON parsing exceptions when converting Jupyter Notebooks (#7308) +- Fixed bug in grading context menu for editing/deleting annotations (#7314) +- Fixed bug in grading annotations table when deleting annotations (#7314) +- Ensure correct LTI version of lti_user_id is used on launch (#7335) + ## [v2.6.0] ### ✨ New features and improvements diff --git a/Gemfile.lock b/Gemfile.lock index 6ad23284f6..240be988cb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -254,7 +254,7 @@ GEM marcel (1.0.4) matrix (0.4.2) mini_mime (1.1.5) - mini_portile2 (2.8.7) + mini_portile2 (2.8.8) minitest (5.25.1) mono_logger (1.1.2) msgpack (1.7.2) @@ -271,7 +271,7 @@ GEM net-smtp (0.5.0) net-protocol nio4r (2.7.4) - nokogiri (1.16.7) + nokogiri (1.16.8) mini_portile2 (~> 2.8.2) racc (~> 1.4) observer (0.1.2) @@ -333,9 +333,9 @@ GEM activesupport (>= 5.0.0) minitest nokogiri (>= 1.6) - rails-html-sanitizer (1.6.0) + rails-html-sanitizer (1.6.1) loofah (~> 2.21) - nokogiri (~> 1.14) + nokogiri (>= 1.15.7, != 1.16.7, != 1.16.6, != 1.16.5, != 1.16.4, != 1.16.3, != 1.16.2, != 1.16.1, != 1.16.0.rc1, != 1.16.0) rails-i18n (7.0.3) i18n (>= 0.7, < 2) railties (>= 6.0.0, < 8) diff --git a/app/MARKUS_VERSION b/app/MARKUS_VERSION index f8c2d0c7ec..7990992d10 100644 --- a/app/MARKUS_VERSION +++ b/app/MARKUS_VERSION @@ -1 +1 @@ -VERSION=v2.6.0,PATCH_LEVEL=DEV +VERSION=v2.6.1,PATCH_LEVEL=DEV diff --git a/app/assets/javascripts/Components/Result/pdf_viewer.jsx b/app/assets/javascripts/Components/Result/pdf_viewer.jsx index b3ab471bd1..ec71a80bfa 100644 --- a/app/assets/javascripts/Components/Result/pdf_viewer.jsx +++ b/app/assets/javascripts/Components/Result/pdf_viewer.jsx @@ -1,9 +1,14 @@ import React from "react"; +import {SingleSelectDropDown} from "../../DropDownMenu/SingleSelectDropDown"; export class PDFViewer extends React.PureComponent { constructor(props) { super(props); this.pdfContainer = React.createRef(); + this.state = { + zoom: "page-width", + rotation: 0, // NOTE: this is in degrees + }; } componentDidMount() { @@ -18,6 +23,8 @@ export class PDFViewer extends React.PureComponent { if (this.props.resultView) { this.eventBus.on("pagesinit", this.ready_annotations); this.eventBus.on("pagesloaded", this.refresh_annotations); + } else { + this.eventBus.on("pagesloaded", this.update_pdf_view); } if (this.props.url) { @@ -31,6 +38,8 @@ export class PDFViewer extends React.PureComponent { } else { if (this.props.resultView) { this.refresh_annotations(); + } else { + this.update_pdf_view(); } } } @@ -44,7 +53,6 @@ export class PDFViewer extends React.PureComponent { ready_annotations = () => { annotation_type = ANNOTATION_TYPES.PDF; - this.pdfViewer.currentScaleValue = "page-width"; window.annotation_manager = new PdfAnnotationManager(!this.props.released_to_students); window.annotation_manager.resetAngle(); this.annotation_manager = window.annotation_manager; @@ -61,15 +69,35 @@ export class PDFViewer extends React.PureComponent { window.pdfViewer = undefined; } + update_pdf_view = () => { + if ( + !!document.getElementById("pdfContainer") && + !!document.getElementById("pdfContainer").offsetParent + ) { + this.pdfViewer.currentScaleValue = this.state.zoom; + this.pdfViewer.pagesRotation = this.state.rotation; + } + }; + refresh_annotations = () => { $(".annotation_holder").remove(); - this.pdfViewer.currentScaleValue = "page-width"; + this.update_pdf_view(); this.props.annotations.forEach(this.display_annotation); if (!!this.props.annotationFocus) { document.getElementById("annotation_holder_" + this.props.annotationFocus).scrollIntoView(); } }; + rotate = () => { + if (this.props.resultView) { + annotation_manager.rotateClockwise90(); + } + + this.setState(({rotation}) => ({ + rotation: (rotation + 90) % 360, + })); + }; + display_annotation = annotation => { if (annotation.x_range === undefined || annotation.y_range === undefined) { return; @@ -101,31 +129,72 @@ export class PDFViewer extends React.PureComponent { ); }; - rotate = () => { - annotation_manager.rotateClockwise90(); - this.pdfViewer.rotatePages(90); + getZoomValuesToDisplayName = () => { + // 25-200 in increments of 25 + const zoomLevels = Array.from({length: (200 - 25) / 25 + 1}, (_, i) => + ((i * 25 + 25) / 100).toFixed(2) + ); + + const valueToDisplayName = zoomLevels.reduce( + (acc, value) => { + acc[value] = `${(value * 100).toFixed(0)} %`; + return acc; + }, + {"page-width": I18n.t("results.fit_to_page_width")} + ); + + return valueToDisplayName; }; render() { const cursor = this.props.released_to_students ? "default" : "crosshair"; const userSelect = this.props.released_to_students ? "default" : "none"; + const zoomValuesToDisplayName = this.getZoomValuesToDisplayName(); + return ( -
-
-
+ +
+
+ {I18n.t("results.current_rotation", {rotation: this.state.rotation})} + + {I18n.t("results.zoom")} + { + this.setState({zoom: selection}); + }} + /> +
+
+
+ id="pdfContainer" + className="pdfContainer" + style={{cursor, userSelect}} + ref={this.pdfContainer} + > +
+
+
-
+ ); } } diff --git a/app/assets/javascripts/Components/__tests__/pdf_viewer.test.jsx b/app/assets/javascripts/Components/__tests__/pdf_viewer.test.jsx new file mode 100644 index 0000000000..9a9b727927 --- /dev/null +++ b/app/assets/javascripts/Components/__tests__/pdf_viewer.test.jsx @@ -0,0 +1,116 @@ +import React from "react"; +import {render, screen, fireEvent} from "@testing-library/react"; +import {PDFViewer} from "../Result/pdf_viewer"; + +describe("PDFViewer", () => { + let mockPdfViewer; + let mockAnnotationManager; + + beforeEach(() => { + mockPdfViewer = { + setDocument: jest.fn(), + pagesRotation: 0, + currentScaleValue: "page-width", + }; + + mockAnnotationManager = { + rotateClockwise90: jest.fn(), + }; + + global.pdfjsViewer = { + EventBus: class { + on = jest.fn(); + }, + PDFViewer: jest.fn(() => mockPdfViewer), + }; + + global.annotation_manager = mockAnnotationManager; + + render(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + delete global.pdfjsViewer; + delete global.annotation_manager; + }); + + describe("rotation", () => { + let rotateButton; + + beforeEach(() => { + rotateButton = screen.getByText(I18n.t("results.rotate_image")); + }); + + it("initially has a rotation of 0", async () => { + expect(mockPdfViewer.pagesRotation).toBe(0); + }); + + it("rotates to 90 degrees when rotate button is clicked once", () => { + fireEvent.click(rotateButton); + + expect(mockAnnotationManager.rotateClockwise90).toHaveBeenCalledTimes(1); + expect(mockPdfViewer.pagesRotation).toBe(90); + }); + + it("rotates back to 0 degrees when rotate button is clicked four times", () => { + for (let i = 0; i < 4; i++) { + fireEvent.click(rotateButton); + } + + expect(mockAnnotationManager.rotateClockwise90).toHaveBeenCalledTimes(4); + expect(mockPdfViewer.pagesRotation).toBe(0); + }); + }); + + describe("zoom", () => { + it("has default zoom 'page-width' on initial render", () => { + expect(mockPdfViewer.currentScaleValue).toBe("page-width"); + }); + + it("updates zoom to 100% (1.0) when the option is selected from dropdown", () => { + const dropdown = screen.getByTestId("dropdown"); + fireEvent.click(dropdown); + + const option100 = screen.getByText("100 %"); + fireEvent.click(option100); + + expect(mockPdfViewer.currentScaleValue).toBe("1.00"); + }); + + it("updates zoom to 75% (0.75) when the option is selected from dropdown", () => { + const dropdown = screen.getByTestId("dropdown"); + fireEvent.click(dropdown); + + const option110 = screen.getByText("75 %"); + fireEvent.click(option110); + + expect(mockPdfViewer.currentScaleValue).toBe("0.75"); + }); + + it("updates zoom to 125% (1.25) when the option is selected from dropdown", () => { + const dropdown = screen.getByTestId("dropdown"); + fireEvent.click(dropdown); + + const option120 = screen.getByText("125 %"); + fireEvent.click(option120); + + expect(mockPdfViewer.currentScaleValue).toBe("1.25"); + }); + + it("resets zoom to 'page-width' when the option is selected after selecting another zoom", () => { + // set some arbitrary zoom first + const dropdown = screen.getByTestId("dropdown"); + fireEvent.click(dropdown); + const option120 = screen.getByText("125 %"); + fireEvent.click(option120); + + // now put it back to page width + fireEvent.click(dropdown); + const fitToPageWidthOption = screen.getByText(I18n.t("results.fit_to_page_width")); + fireEvent.click(fitToPageWidthOption); + + expect(mockPdfViewer.currentScaleValue).toBe("page-width"); + }); + }); +}); diff --git a/app/assets/javascripts/Components/__tests__/ta_table.test.jsx b/app/assets/javascripts/Components/__tests__/ta_table.test.jsx index 2eeb0ac25d..abdbf5bbf3 100644 --- a/app/assets/javascripts/Components/__tests__/ta_table.test.jsx +++ b/app/assets/javascripts/Components/__tests__/ta_table.test.jsx @@ -1,6 +1,5 @@ -import {mount} from "enzyme"; - import {TATable} from "../ta_table"; +import {render, screen, fireEvent, waitFor, within} from "@testing-library/react"; global.fetch = jest.fn(() => Promise.resolve({ @@ -14,17 +13,19 @@ global.fetch = jest.fn(() => ); describe("For the TATable's display of TAs", () => { - let wrapper, tas_sample; + let tas_sample; describe("when some TAs are fetched", () => { - const tas_in_one_row = (wrapper, ta) => { - // Find the row - const row = wrapper.find({children: ta.user_name}).parent(); - // Expect the row to contain these information - expect(row.children({children: ta.first_name})).toBeTruthy(); - expect(row.children({children: ta.last_name})).toBeTruthy(); + const tas_in_one_row = ta => { + const row = screen.getByText(ta.user_name).closest("div[role='row']"); + expect(row).toBeInTheDocument(); + + expect(within(row).queryByText(ta.first_name)).toBeInTheDocument(); + + expect(within(row).queryByText(ta.last_name)).toBeInTheDocument(); + if (ta.email) { - expect(row.children({children: ta.email})).toBeTruthy(); + expect(within(row).queryByText(ta.email)).toBeInTheDocument(); } }; @@ -72,11 +73,15 @@ describe("For the TATable's display of TAs", () => { counts: {all: 4, active: 3, inactive: 1}, }), }); - wrapper = mount(); + + render(); }); - it("each TA is displayed as a row of the table", () => { - tas_sample.forEach(ta => tas_in_one_row(wrapper, ta)); + it("each TA is displayed as a row of the table", async () => { + await screen.findByRole("grid"); + console.log(screen.debug()); + + tas_sample.forEach(ta => tas_in_one_row(ta)); }); }); @@ -92,11 +97,61 @@ describe("For the TATable's display of TAs", () => { counts: {all: 0, active: 0, inactive: 0}, }), }); - wrapper = mount(); + render(); }); it("No rows found is shown", () => { - expect(wrapper.find({children: "No rows found"})).toBeTruthy(); + expect(screen.getByText("No rows found")).toBeInTheDocument(); + }); + }); + + describe("When the delete button is pressed", () => { + let mock_course_id = 1; + let mock_ta_id = 42; + + beforeEach(() => { + jest.clearAllMocks(); + jest.spyOn(global, "fetch").mockResolvedValue({ + ok: true, + json: jest.fn().mockResolvedValue({ + data: [ + { + id: mock_ta_id, + user_name: "testtest", + first_name: "Test", + last_name: "Test", + email: "test@test.com", + hidden: false, + }, + ], + counts: {all: 1, active: 1, inactive: 0}, + }), + }); + + document.querySelector = jest.fn().mockReturnValue({ + content: "mocked-csrf-token", + }); + }); + + it("calls the correct endpoint when removeTA is triggered", async () => { + render(); + + await screen.findByText("testtest"); + + fireEvent.click(screen.getByText(I18n.t("delete"))); + + await waitFor(() => { + expect(fetch).toHaveBeenCalledWith( + Routes.course_ta_path(mock_course_id, mock_ta_id), + expect.objectContaining({ + method: "DELETE", + headers: expect.objectContaining({ + "Content-Type": "application/json", + "X-CSRF-Token": expect.any(String), + }), + }) + ); + }); }); }); }); diff --git a/app/assets/javascripts/Components/ta_table.jsx b/app/assets/javascripts/Components/ta_table.jsx index 2939ce20a5..bd3e1e8d62 100644 --- a/app/assets/javascripts/Components/ta_table.jsx +++ b/app/assets/javascripts/Components/ta_table.jsx @@ -36,6 +36,24 @@ class TATable extends React.Component { }); } + removeTA = ta_id => { + fetch(Routes.course_ta_path(this.props.course_id, ta_id), { + method: "DELETE", + headers: { + "Content-Type": "application/json", + "X-CSRF-Token": document.querySelector('[name="csrf-token"]').content, + }, + }) + .then(response => { + if (response.ok) { + this.fetchData(); + } + }) + .catch(error => { + console.error("Error deleting TA:", error); + }); + }; + render() { const {data} = this.state; return ( @@ -89,12 +107,19 @@ class TATable extends React.Component { Header: I18n.t("actions"), accessor: "id", Cell: data => ( - - - {I18n.t("edit")} - -   - + <> + + + {I18n.t("edit")} + + +  |  + + this.removeTA(data.value)}> + {I18n.t("delete")} + + + ), filterable: false, sortable: false, diff --git a/app/assets/javascripts/DropDownMenu/SingleSelectDropDown.jsx b/app/assets/javascripts/DropDownMenu/SingleSelectDropDown.jsx index 71e5112126..5af54a0bad 100644 --- a/app/assets/javascripts/DropDownMenu/SingleSelectDropDown.jsx +++ b/app/assets/javascripts/DropDownMenu/SingleSelectDropDown.jsx @@ -72,28 +72,30 @@ export class SingleSelectDropDown extends React.Component { return (
this.setState({expanded: !this.state.expanded})} onBlur={() => this.setState({expanded: false})} tabIndex={-1} data-testid={"dropdown"} > - + {this.props.valueToDisplayName != null ? this.props.valueToDisplayName[this.props.selected] : this.props.selected} {this.renderArrow()} -
{ - e.preventDefault(); - this.onSelect(e, this.props.defaultValue); - }} - data-testid={"reset-dropdown-selection"} - > - -
- + {!this.props.hideXMark && ( +
{ + e.preventDefault(); + this.onSelect(e, this.props.defaultValue); + }} + data-testid={"reset-dropdown-selection"} + > + +
+ )} {expanded && this.renderDropdown(options, selected, expanded, disabled)}
); diff --git a/app/assets/javascripts/Results/context_menu.js b/app/assets/javascripts/Results/context_menu.js index 48da0d1525..17cb05be1f 100644 --- a/app/assets/javascripts/Results/context_menu.js +++ b/app/assets/javascripts/Results/context_menu.js @@ -106,7 +106,7 @@ var annotation_context_menu = { } return ""; } else { - return clicked_element.attr("id").replace("annotation_holder_", ""); + return clicked_element.id.replace("annotation_holder_", ""); } } diff --git a/app/assets/stylesheets/common/codeviewer.scss b/app/assets/stylesheets/common/codeviewer.scss index d7e4ada00e..59924c6216 100644 --- a/app/assets/stylesheets/common/codeviewer.scss +++ b/app/assets/stylesheets/common/codeviewer.scss @@ -11,7 +11,7 @@ .toolbar-actions { background-color: $background-main; font: 0.825em $fonts; - padding: 5px 0; + padding: 5px; text-align: right; a { diff --git a/app/assets/stylesheets/common/pdfjs_custom.scss b/app/assets/stylesheets/common/pdfjs_custom.scss index 16d0f85172..40b4a39c89 100644 --- a/app/assets/stylesheets/common/pdfjs_custom.scss +++ b/app/assets/stylesheets/common/pdfjs_custom.scss @@ -1,5 +1,7 @@ .pdfContainerParent { position: relative; + overflow: auto; + height: 100%; } .pdfContainer { diff --git a/app/controllers/lti_deployments_controller.rb b/app/controllers/lti_deployments_controller.rb index 6ce0d96b71..22dfe1fea2 100644 --- a/app/controllers/lti_deployments_controller.rb +++ b/app/controllers/lti_deployments_controller.rb @@ -102,7 +102,7 @@ def redirect_login lti_data[:line_items] = grades_endpoints['lineitems'] end end - lti_data[:lti_user_id] = lti_params[LtiDeployment::LTI_CLAIMS[:user_launch_data]]['user_id'] + lti_data[:lti_user_id] = lti_params[LtiDeployment::LTI_CLAIMS[:user_id]] unless logged_in? lti_data[:lti_redirect] = request.url cookies.encrypted.permanent[:lti_data] = diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 67f4646dc2..12281c582c 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -971,7 +971,11 @@ def notebook_to_html(file_contents, unique_path, type) '--template', 'markus-html-template' ] end - file_contents = JSON.parse(file_contents) + begin + file_contents = JSON.parse(file_contents) + rescue JSON::ParserError => e + return "#{I18n.t('submissions.invalid_jupyter_notebook_content')}: #{e}" + end if file_contents['metadata'].key?('widgets') file_contents['metadata'].delete('widgets') end diff --git a/app/controllers/tas_controller.rb b/app/controllers/tas_controller.rb index dfd4211375..d20a4130ce 100644 --- a/app/controllers/tas_controller.rb +++ b/app/controllers/tas_controller.rb @@ -43,6 +43,22 @@ def update respond_with @role, location: course_tas_path(current_course) end + def destroy + @role = record + begin + @role.destroy! + rescue ActiveRecord::DeleteRestrictionError => e + flash_message(:error, I18n.t('flash.tas.destroy.restricted', user_name: @role.user_name, message: e.message)) + head :conflict + rescue ActiveRecord::RecordNotDestroyed => e + flash_message(:error, I18n.t('flash.tas.destroy.error', user_name: @role.user_name, message: e.message)) + head :bad_request + else + flash_now(:success, I18n.t('flash.tas.destroy.success', user_name: @role.user_name)) + head :ok + end + end + def download keys = [:user_name, :last_name, :first_name, :email] tas = current_course.tas.joins(:user).pluck_to_hash(*keys) diff --git a/app/models/annotation.rb b/app/models/annotation.rb index 6ddf95cc2f..22670ee400 100644 --- a/app/models/annotation.rb +++ b/app/models/annotation.rb @@ -50,9 +50,8 @@ def get_data(include_creator: false) criterion_name: annotation_text.annotation_category&.flexible_criterion&.name, criterion_id: annotation_text.annotation_category&.flexible_criterion&.id } - if include_creator - data[:creator] = creator.display_name + data[:creator] = creator.present? ? creator.display_name : I18n.t('deleted_placeholder') end data diff --git a/app/models/lti_deployment.rb b/app/models/lti_deployment.rb index aa427fa470..5b91304a0e 100644 --- a/app/models/lti_deployment.rb +++ b/app/models/lti_deployment.rb @@ -15,7 +15,7 @@ class LtiDeployment < ApplicationRecord names_role: 'https://purl.imsglobal.org/spec/lti-nrps/claim/namesroleservice', ags_lineitem: 'https://purl.imsglobal.org/spec/lti-ags/claim/endpoint', deployment_id: 'https://purl.imsglobal.org/spec/lti/claim/deployment_id', - user_launch_data: 'https://purl.imsglobal.org/spec/lti/claim/lti1p1' }.freeze + user_id: 'sub' }.freeze LTI_ROLES = { learner: 'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner', test_user: 'http://purl.imsglobal.org/vocab/lti/system/person#TestUser', ta: 'http://purl.imsglobal.org/vocab/lis/v2/membership/Instructor#TeachingAssistant', diff --git a/app/models/ta.rb b/app/models/ta.rb index bf62698781..7c28c5254d 100644 --- a/app/models/ta.rb +++ b/app/models/ta.rb @@ -5,11 +5,17 @@ class Ta < Role validates :grader_permission, presence: { unless: -> { self.new_record? } } validate :associated_user_is_an_end_user accepts_nested_attributes_for :grader_permission - has_many :criterion_ta_associations, dependent: :delete_all + + has_many :annotation_texts, dependent: :nullify, inverse_of: :creator, foreign_key: :creator_id + has_many :annotations, dependent: :nullify, inverse_of: :creator, foreign_key: :creator_id + + has_many :criterion_ta_associations, dependent: :delete_all, inverse_of: :ta has_many :criteria, through: :criterion_ta_associations - has_many :grade_entry_student_tas, dependent: :delete_all - has_many :grade_entry_students, through: :grade_entry_student_tas, dependent: :delete_all + has_many :grade_entry_student_tas, dependent: :delete_all, inverse_of: :ta + has_many :grade_entry_students, through: :grade_entry_student_tas + + has_many :notes, dependent: :restrict_with_exception, inverse_of: :role, foreign_key: :creator_id BLANK_MARK = ''.freeze diff --git a/app/policies/ta_policy.rb b/app/policies/ta_policy.rb index 132954af82..8f71e9ae12 100644 --- a/app/policies/ta_policy.rb +++ b/app/policies/ta_policy.rb @@ -23,6 +23,10 @@ def download? role.instructor? end + def destroy? + role.instructor? + end + def upload? role.instructor? end diff --git a/config/locales/common/en.yml b/config/locales/common/en.yml index 8bb3c0e0dd..afe2d1f284 100644 --- a/config/locales/common/en.yml +++ b/config/locales/common/en.yml @@ -11,6 +11,7 @@ en: continue: Continue copy: Copy delete: Delete + deleted_placeholder: "(deleted)" edit: Edit encoding: Encoding errors: diff --git a/config/locales/defaults/responders/en.yml b/config/locales/defaults/responders/en.yml index 90c043416a..c706e3c34a 100644 --- a/config/locales/defaults/responders/en.yml +++ b/config/locales/defaults/responders/en.yml @@ -39,5 +39,9 @@ en: tas: create: error: "%{resource_name} could not be created due to the following error(s): %{errors}." + destroy: + error: 'Error deleting TA (%{user_name}): %{message}' + restricted: 'Cannot delete TA (%{user_name}): %{message}' + success: Succesfully deleted TA (%{user_name}) from course. update: error: "%{resource_name} could not be updated due to the following error(s): %{errors}." diff --git a/config/locales/views/results/en.yml b/config/locales/views/results/en.yml index 7ac5624dc5..bbb482f027 100644 --- a/config/locales/views/results/en.yml +++ b/config/locales/views/results/en.yml @@ -39,6 +39,7 @@ en: ascending: Ascending descending: Descending text_box_placeholder: Search text + fit_to_page_width: Fit to page width fullscreen_enter: Fullscreen fullscreen_exit: Leave fullscreen keybinding: @@ -91,5 +92,6 @@ en: view_group_repo: View group repository view_token_submit: Please enter the unique token provided by your instructor to view the results for this assignment. your_mark: Your Mark + zoom: 'Zoom:' zoom_in_image: Zoom in zoom_out_image: Zoom out diff --git a/config/locales/views/submissions/en.yml b/config/locales/views/submissions/en.yml index 09c952f8dc..2ff91017f4 100644 --- a/config/locales/views/submissions/en.yml +++ b/config/locales/views/submissions/en.yml @@ -55,6 +55,7 @@ en: tas: Click on assignments to grade them. Rows in green have been collected and are ready to begin marking. If your assigned submissions haven't been collected, you can click on the group name to collect them. how_many_marked: "%{num_marked}/%{num_assigned} completed" how_many_marked_in_collected: "%{num_marked}/%{num_collected} completed" + invalid_jupyter_notebook_content: Invalid Jupyter Notebook content invalid_url: "%{item} is not a valid HTTP/HTTPS URL" marking_incomplete_warning: |- Note: This will only download the submissions that have been collected. diff --git a/config/routes.rb b/config/routes.rb index 1a0d7a79ba..3f2451d599 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -471,7 +471,7 @@ end end - resources :tas, only: [:create, :new, :index, :edit, :update] do + resources :tas, only: [:create, :new, :index, :edit, :update, :destroy] do collection do get 'download' post 'upload' diff --git a/jest_after_env_setup.js b/jest_after_env_setup.js index 2fdb3bda17..31844c031e 100644 --- a/jest_after_env_setup.js +++ b/jest_after_env_setup.js @@ -21,3 +21,26 @@ configure({adapter: new Adapter()}); // Jest fetch mock require("jest-fetch-mock").enableMocks(); + +// Define HTMLElement.prototype.offsetParent +// Code from https://github.com/jsdom/jsdom/issues/1261#issuecomment-1765404346 +Object.defineProperty(HTMLElement.prototype, "offsetParent", { + get() { + // eslint-disable-next-line @typescript-eslint/no-this-alias + for (let element = this; element; element = element.parentNode) { + if (element.style?.display?.toLowerCase() === "none") { + return null; + } + } + + if (this.stye?.position?.toLowerCase() === "fixed") { + return null; + } + + if (this.tagName.toLowerCase() in ["html", "body"]) { + return null; + } + + return this.parentNode; + }, +}); diff --git a/spec/controllers/submissions_controller_spec.rb b/spec/controllers/submissions_controller_spec.rb index 64016ff41f..50d5b57d01 100644 --- a/spec/controllers/submissions_controller_spec.rb +++ b/spec/controllers/submissions_controller_spec.rb @@ -1786,6 +1786,21 @@ expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path')) end end + + context 'where there is an invalid Jupyter notebook content' do + render_views + let(:filename) { 'pdf_with_ipynb_extension.ipynb' } + + it 'should display an invalid Jupyter notebook content error message' do + get_as instructor, :notebook_content, params: { course_id: course.id, + assignment_id: assignment.id, + file_name: filename, + preview: true, + grouping_id: grouping.id, + revision_identifier: submission.revision_identifier } + expect(response.body).to include(I18n.t('submissions.invalid_jupyter_notebook_content')) + end + end end describe '#get_file' do diff --git a/spec/controllers/tas_controller_spec.rb b/spec/controllers/tas_controller_spec.rb index 5b97287daa..aa7f6d5447 100644 --- a/spec/controllers/tas_controller_spec.rb +++ b/spec/controllers/tas_controller_spec.rb @@ -270,4 +270,98 @@ end end end + + describe '#destroy' do + context 'when not an instructor' do + let(:instructor) { create(:instructor) } + let(:ta) { create(:ta, course: course) } + let(:student_role) { create(:student) } + + before do + delete_as student_role, :destroy, params: { course_id: course.id, id: ta.id } + end + + it 'does not delete TA and gets 403 response' do + expect(Ta.count).to eq(1) + expect(flash.now[:success]).to be_nil + expect(response).to have_http_status(:forbidden) + end + end + + context 'when current_role is an instructor but destroy fails' do + let(:instructor) { create(:instructor) } + let(:ta) { create(:ta, course: course) } + + before do + allow_any_instance_of(Role).to receive(:destroy).and_return(false) + delete_as instructor, :destroy, params: { course_id: course.id, id: ta.id } + end + + it 'does not delete the TA, shows an error message, and gets a bad request response' do + expect(Ta.count).to eq(1) + expect(flash.now[:success]).to be_nil + expect(flash[:error].first).to include(I18n.t('flash.tas.destroy.error', user_name: ta.user_name, message: '')) + expect(response).to have_http_status(:bad_request) + end + end + + context 'when TA has a note' do + let(:instructor) { create(:instructor) } + let(:ta) { create(:ta, course: course) } + + before do + create(:note, role: ta) + delete_as instructor, :destroy, params: { course_id: course.id, id: ta.id } + end + + it 'does not delete the TA, shows an error message and gets a conflict response' do + expect(Ta.count).to eq(1) + expect(flash.now[:success]).to be_nil + expect(flash[:error].first).to include(I18n.t('flash.tas.destroy.restricted', user_name: ta.user_name, + message: '')) + expect(response).to have_http_status(:conflict) + end + end + + context 'succeeds for TA deletion' do + let!(:ta) { create(:ta, course: course) } + let(:instructor) { create(:instructor) } + + let!(:annotation) { create(:text_annotation, creator: ta) } + + let(:student) { create(:student) } + + before do + create(:criterion_ta_association, ta: ta) + create(:grade_entry_form) + create(:grade_entry_student_ta, ta: ta, grade_entry_student: student.grade_entry_students.first) + + delete_as instructor, :destroy, params: { course_id: course.id, id: ta.id } + end + + it 'deletes TA, flashes success, and gets an ok response' do + expect(Ta.exists?).to be(false) + expect(flash.now[:success].first).to include(I18n.t('flash.tas.destroy.success', user_name: ta.user_name)) + expect(response).to have_http_status(:ok) + end + + it 'deletes associated grader permisison' do + expect(GraderPermission.exists?(role_id: ta.id)).to be(false) + end + + it 'nullifies creator id in associated annotation' do + annotation.reload + expect(annotation.creator_id).to be_nil + expect(Annotation.exists?(annotation.id)).to be(true) + end + + it 'deletes associated criterion ta association' do + expect(CriterionTaAssociation.exists?(ta_id: ta.id)).to be(false) + end + + it 'deletes associated grade entry student ta' do + expect(GradeEntryStudentTa.exists?(ta_id: ta.id)).to be(false) + end + end + end end diff --git a/spec/fixtures/files/pdf_with_ipynb_extension.ipynb b/spec/fixtures/files/pdf_with_ipynb_extension.ipynb new file mode 100644 index 0000000000..e8ef024e77 Binary files /dev/null and b/spec/fixtures/files/pdf_with_ipynb_extension.ipynb differ diff --git a/spec/support/annotation_shared_examples.rb b/spec/support/annotation_shared_examples.rb index c26c338cc6..daf4519356 100644 --- a/spec/support/annotation_shared_examples.rb +++ b/spec/support/annotation_shared_examples.rb @@ -12,8 +12,19 @@ end context 'when include_creator is true' do - it 'gets all data including creator' do - expect(Set.new(annotation.get_data(include_creator: true).keys)).to eq keys + [:creator] + context 'and creator is present' do + it 'gets all data including creator' do + expect(Set.new(annotation.get_data(include_creator: true).keys)).to eq keys + [:creator] + end + end + + context 'but creator is not present' do + before { allow(annotation).to receive(:creator).and_return(nil) } + + it 'sets creator as delete placeholder' do + data = annotation.get_data(include_creator: true) + expect(data[:creator]).to eq(I18n.t('deleted_placeholder')) + end end end end diff --git a/spec/support/lti_controller_examples.rb b/spec/support/lti_controller_examples.rb index 6c3e7f3671..f98a709d1d 100644 --- a/spec/support/lti_controller_examples.rb +++ b/spec/support/lti_controller_examples.rb @@ -142,9 +142,7 @@ course_id: 1, user_id: 1 }, - LtiDeployment::LTI_CLAIMS[:user_launch_data] => { - user_id: 'lti_user_id' - } } + LtiDeployment::LTI_CLAIMS[:user_id] => 'some_user_id' } end let(:pub_jwk) { JWT::JWK.new(OpenSSL::PKey::RSA.new(1024)) } let(:lti_jwt) { JWT.encode(payload, pub_jwk.keypair, 'RS256', { kid: pub_jwk.kid }) }