diff --git a/app/assets/javascripts/Components/__tests__/collect_submissions_modal.test.jsx b/app/assets/javascripts/Components/__tests__/collect_submissions_modal.test.jsx index 39019fb13b..01c63e975c 100644 --- a/app/assets/javascripts/Components/__tests__/collect_submissions_modal.test.jsx +++ b/app/assets/javascripts/Components/__tests__/collect_submissions_modal.test.jsx @@ -1,4 +1,3 @@ -import * as React from "react"; import {render, screen, fireEvent, waitFor} from "@testing-library/react"; import CollectSubmissionsModal from "../Modals/collect_submissions_modal"; import Modal from "react-modal"; diff --git a/app/assets/javascripts/Components/__tests__/repo_browser_manual_collection_form.test.jsx b/app/assets/javascripts/Components/__tests__/repo_browser_manual_collection_form.test.jsx index d7f1355b41..ce66eef1ad 100644 --- a/app/assets/javascripts/Components/__tests__/repo_browser_manual_collection_form.test.jsx +++ b/app/assets/javascripts/Components/__tests__/repo_browser_manual_collection_form.test.jsx @@ -1,7 +1,18 @@ -import * as React from "react"; -import {render, screen} from "@testing-library/react"; +import {render, screen, fireEvent} from "@testing-library/react"; import {ManualCollectionForm} from "../repo_browser"; +// workaround needed for using i18n in jest tests, see +// https://github.com/fnando/i18n/issues/26#issuecomment-1235751777 +jest.mock("i18n-js", () => { + return jest.requireActual("i18n-js/dist/require/index"); +}); + +jest.mock("@fortawesome/react-fontawesome", () => ({ + FontAwesomeIcon: () => { + return null; + }, +})); + describe("RepoBrowser's ManualCollectionForm", () => { let props, component; @@ -15,7 +26,6 @@ describe("RepoBrowser's ManualCollectionForm", () => { collected_revision_id: "test", }; - // Set the app element for React Modal component = render(); }); @@ -23,8 +33,8 @@ describe("RepoBrowser's ManualCollectionForm", () => { const lblRecollectExistingSubmissions = screen.getByTestId("lbl_retain_existing_grading"); const chkRecollectExistingSubmissions = screen.getByTestId("chk_retain_existing_grading"); - expect(lblRecollectExistingSubmissions).toBeInTheDocument(); - expect(chkRecollectExistingSubmissions).toBeInTheDocument(); + expect(lblRecollectExistingSubmissions).toBeVisible(); + expect(chkRecollectExistingSubmissions).toBeVisible(); }); it("does not show the option to retain existing grading when there is a not collected revision present", () => { @@ -34,7 +44,29 @@ describe("RepoBrowser's ManualCollectionForm", () => { const lblRecollectExistingSubmissions = screen.queryByTestId("lbl_retain_existing_grading"); const chkRecollectExistingSubmissions = screen.queryByTestId("chk_retain_existing_grading"); - expect(lblRecollectExistingSubmissions).not.toBeInTheDocument(); - expect(chkRecollectExistingSubmissions).not.toBeInTheDocument(); + expect(lblRecollectExistingSubmissions).not.toBeVisible(); + expect(chkRecollectExistingSubmissions).not.toBeVisible(); + }); + + it("should confirm with a full overwrite warning when retain existing grading option is checked", () => { + const confirmSpy = jest.spyOn(window, "confirm").mockImplementation(() => false); + const manualCollectionForm = component.getByTestId("form_manual_collection"); + + fireEvent.submit(manualCollectionForm); + + expect(confirmSpy).toHaveBeenCalledWith( + I18n.t("submissions.collect.partial_overwrite_warning") + ); + }); + + it("should confirm with a full overwrite warning when retain existing grading option is not checked", () => { + const confirmSpy = jest.spyOn(window, "confirm").mockImplementation(() => false); + const chkRecollectExistingSubmissions = screen.queryByTestId("chk_retain_existing_grading"); + const manualCollectionForm = component.getByTestId("form_manual_collection"); + + fireEvent.click(chkRecollectExistingSubmissions); + fireEvent.submit(manualCollectionForm); + + expect(confirmSpy).toHaveBeenCalledWith(I18n.t("submissions.collect.full_overwrite_warning")); }); }); diff --git a/app/assets/javascripts/Components/repo_browser.jsx b/app/assets/javascripts/Components/repo_browser.jsx index a2659f2f86..244c236c95 100644 --- a/app/assets/javascripts/Components/repo_browser.jsx +++ b/app/assets/javascripts/Components/repo_browser.jsx @@ -157,7 +157,22 @@ class ManualCollectionForm extends React.Component { {I18n.t("submissions.collect.manual_collection")} -
+ { + if ( + this.props.collected_revision_id && + ((!this.state.retainExistingGrading && + !confirm(I18n.t("submissions.collect.full_overwrite_warning"))) || + (this.state.retainExistingGrading && + !confirm(I18n.t("submissions.collect.partial_overwrite_warning")))) + ) { + event.preventDefault(); + } + }} + >

- {this.props.collected_revision_id && ( -
- { - this.setState({retainExistingGrading: e.target.checked}); - }} - /> - -
- )} - diff --git a/spec/jobs/submissions_job_spec.rb b/spec/jobs/submissions_job_spec.rb index b613127131..50dd4bbeb2 100644 --- a/spec/jobs/submissions_job_spec.rb +++ b/spec/jobs/submissions_job_spec.rb @@ -191,25 +191,25 @@ before do # explicit storing and freezing here or it may implicitly reload this and # we can't compare to the old one - @original_submissions = assignment.reload.current_submissions_used.to_a - @original_results = assignment.current_results.to_a + @original_submissions = assignment.reload.current_submissions_used.order('submissions.grouping_id').to_a + @original_results = assignment.current_results.order('submissions.grouping_id').to_a SubmissionsJob.perform_now(assignment.groupings, retain_existing_grading: true) - @new_submissions = assignment.reload.current_submissions_used - @new_results = assignment.reload.current_results + @new_submissions = assignment.reload.current_submissions_used.order('submissions.grouping_id') + @new_results = assignment.reload.current_results.order('submissions.grouping_id') end it 'creates the correct number of new submissions' do # still the same number of groupings => the same number of submissions expect(@new_submissions.size).to eq(@original_submissions.size) - expect(@new_submissions.ids).not_to eq(@original_submissions.map(&:id)) + expect(@new_submissions.ids.sort).not_to eq(@original_submissions.map(&:id).sort) end it 'creates the correct number of new results' do # still the same number of submissions => the same number of results expect(@new_results.size).to eq(@original_results.size) - expect(@new_results.ids).not_to eq(@original_results.map(&:id)) + expect(@new_results.ids.sort).not_to eq(@original_results.map(&:id).sort) end context 'for feedback files on each new submission' do @@ -218,13 +218,16 @@ it 'creates the correct number of new feedback files on each submission' do @new_submissions.zip(@original_submissions).each do |new_submission, old_submission| expect(new_submission.feedback_files.size).to eq(old_submission.feedback_files.size) - expect(new_submission.feedback_files.ids).not_to eq(old_submission.feedback_files.ids) + expect(new_submission.feedback_files.ids.sort).not_to eq(old_submission.feedback_files.ids.sort) end end it 'retains the file name on each new feedback file' do @new_submissions.zip(@original_submissions).each do |new_submission, old_submission| - expect(new_submission.feedback_files.map(&:filename)).to eq(old_submission.feedback_files.map(&:filename)) + # cannot compare by order here because there is no common field between the two, so + # instead we check the set of filenames is equal + expect(new_submission.feedback_files.map(&:filename).sort).to eq(old_submission + .feedback_files.map(&:filename).sort) end end end @@ -235,7 +238,7 @@ it 'creates the correct number of new test runs' do @new_submissions.zip(@original_submissions).each do |new_submission, old_submission| expect(new_submission.test_runs.size).to eq(old_submission.test_runs.size) - expect(new_submission.test_runs.ids).not_to eq(old_submission.test_runs.ids) + expect(new_submission.test_runs.ids.sort).not_to eq(old_submission.test_runs.ids.sort) end end @@ -244,7 +247,7 @@ @new_submissions.zip(@original_submissions).each do |new_submission, old_submission| new_submission.test_runs.zip(old_submission.test_runs).each do |old_test_run, new_test_run| expect(new_test_run.test_group_results.size).to eq(old_test_run.test_group_results.size) - expect(new_test_run.test_group_results.ids).not_to eq(old_test_run.test_group_results.ids) + expect(new_test_run.test_group_results.ids.sort).not_to eq(old_test_run.test_group_results.ids.sort) end end end @@ -255,7 +258,7 @@ new_submission.test_runs.zip(old_submission.test_runs).each do |old_test_run, new_test_run| new_test_run.test_group_results.zip(old_test_run.test_group_results).each do |old_tgr, new_tgr| expect(new_tgr.test_results.size).to eq(old_tgr.test_results.size) - expect(new_tgr.test_results.ids).not_to eq(old_tgr.test_results.ids) + expect(new_tgr.test_results.ids.sort).not_to eq(old_tgr.test_results.ids.sort) end end end @@ -266,7 +269,7 @@ new_submission.test_runs.zip(old_submission.test_runs).each do |old_test_run, new_test_run| new_test_run.test_group_results.zip(old_test_run.test_group_results).each do |old_tgr, new_tgr| expect(new_tgr.feedback_files.size).to eq(old_tgr.feedback_files.size) - expect(new_tgr.feedback_files.ids).not_to eq(old_tgr.feedback_files.ids) + expect(new_tgr.feedback_files.ids.sort).not_to eq(old_tgr.feedback_files.ids.sort) end end end @@ -286,8 +289,8 @@ it 'retains the marks from each original submission\'s original result' do @new_submissions.zip(@original_submissions).each do |new_submission, old_submission| # the last result in each submission is the original one in the old submission - expect(new_submission.current_result.marks.map(&:mark)).to eq(old_submission - .get_original_result.marks.map(&:mark)) + expect(new_submission.current_result.marks.order(:criterion_id).map(&:mark)).to eq(old_submission + .get_original_result.marks.order(:criterion_id).map(&:mark)) end end end @@ -296,13 +299,14 @@ it 'creates the correct number of new marks for each result' do @new_results.zip(@original_results).each do |new_result, old_result| expect(new_result.marks.size).to eq(old_result.marks.size) - expect(new_result.marks.ids).not_to eq(old_result.marks.ids) + expect(new_result.marks.ids.sort).not_to eq(old_result.marks.ids.sort) end end it 'retains the correct mark values for each result' do @new_results.zip(@original_results).each do |new_result, old_result| - expect(new_result.marks.map(&:mark)).to eq(old_result.marks.map(&:mark)) + expect(new_result.marks.order(:criterion_id).map(&:mark)).to eq(old_result + .marks.order(:criterion_id).map(&:mark)) end end end @@ -313,19 +317,22 @@ it 'creates the correct number of new annotations for each result' do @new_results.reload.zip(@original_results).each do |new_result, old_result| expect(new_result.annotations.size).to eq(old_result.annotations.size) - expect(new_result.annotations.ids).not_to eq(old_result.annotations.ids) + expect(new_result.annotations.ids.sort).not_to eq(old_result.annotations.ids.sort) end end it 'retains the mark deductions from deductive annotations' do @new_results.zip(@original_results).each do |new_result, old_result| - expect(new_result.marks.map(&:calculate_deduction)).to eq(old_result.marks.map(&:calculate_deduction)) + expect(new_result.marks.order(:criterion_id).map(&:calculate_deduction)).to eq(old_result + .marks.order(:criterion_id).map(&:calculate_deduction)) end end it 'retains the text from each annotation' do @new_results.zip(@original_results).each do |new_result, old_result| - expect(new_result.annotations.map { |a| a.annotation_text.content }).to eq(old_result.annotations.map do |a| + expect(new_result.annotations.order(:annotation_text_id).map do |a| + a.annotation_text.content + end).to eq(old_result.annotations.order(:annotation_text_id).map do |a| a.annotation_text.content end) end @@ -348,13 +355,14 @@ it 'creates the correct number of new extra marks for each result' do @new_results.zip(@original_results).each do |new_result, old_result| expect(new_result.extra_marks.size).to eq(old_result.extra_marks.size) - expect(new_result.extra_marks.ids).not_to eq(old_result.extra_marks.ids) + expect(new_result.extra_marks.ids.sort).not_to eq(old_result.extra_marks.ids.sort) end end it 'retains the correct mark values for each result' do @new_results.zip(@original_results).each do |new_result, old_result| - expect(new_result.extra_marks.map(&:extra_mark)).to eq(old_result.extra_marks.map(&:extra_mark)) + # no common field so compare by full array + expect(new_result.extra_marks.map(&:extra_mark).sort).to eq(old_result.extra_marks.map(&:extra_mark).sort) end end end