From 01e0d6377ea8abae522084e775ac8beec13c156b Mon Sep 17 00:00:00 2001
From: Pranav Rao <56097527+pranavrao145@users.noreply.github.com>
Date: Thu, 17 Oct 2024 23:26:40 -0400
Subject: [PATCH] test(retain-old-grading-data-option): further test coverage

---
 .../collect_submissions_modal.test.jsx        |  1 -
 ...po_browser_manual_collection_form.test.jsx | 46 +++++++++++--
 .../javascripts/Components/repo_browser.jsx   | 67 ++++++++++---------
 spec/jobs/submissions_job_spec.rb             | 50 ++++++++------
 4 files changed, 104 insertions(+), 60 deletions(-)

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(<ManualCollectionForm {...props} />);
   });
 
@@ -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 {
         <legend>
           <span>{I18n.t("submissions.collect.manual_collection")}</span>
         </legend>
-        <form method="POST" action={action}>
+        <form
+          method="POST"
+          action={action}
+          data-testid="form_manual_collection"
+          onSubmit={event => {
+            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();
+            }
+          }}
+        >
           <input
             type="hidden"
             name="current_revision_identifier"
@@ -176,36 +191,26 @@ class ManualCollectionForm extends React.Component {
               {I18n.t("submissions.collect.apply_late_penalty")}
             </label>
           </p>
-          {this.props.collected_revision_id && (
-            <div className="inline-labels" style={{marginBottom: "1em"}}>
-              <input
-                type="checkbox"
-                name="retain_existing_grading"
-                data-testid="chk_retain_existing_grading"
-                checked={this.state.retainExistingGrading}
-                onChange={e => {
-                  this.setState({retainExistingGrading: e.target.checked});
-                }}
-              />
-              <label data-testid="lbl_retain_existing_grading">
-                {I18n.t("submissions.collect.retain_existing_grading")}
-              </label>
-            </div>
-          )}
-          <button
-            type="submit"
-            name="commit"
-            onClick={event => {
-              if (
-                (!this.state.retainExistingGrading &&
-                  !confirm(I18n.t("submissions.collect.full_overwrite_warning"))) ||
-                (this.state.retainExistingGrading &&
-                  !confirm(I18n.t("submissions.collect.partial_overwrite_warning")))
-              ) {
-                event.preventDefault();
-              }
-            }}
-          >
+          <p className="inline-labels">
+            <input
+              type="checkbox"
+              name="retain_existing_grading"
+              data-testid="chk_retain_existing_grading"
+              checked={this.state.retainExistingGrading}
+              hidden={!this.props.collected_revision_id}
+              disabled={!this.props.collected_revision_id} // prevent from sending info on submit
+              onChange={e => {
+                this.setState({retainExistingGrading: e.target.checked});
+              }}
+            />
+            <label
+              data-testid="lbl_retain_existing_grading"
+              hidden={!this.props.collected_revision_id}
+            >
+              {I18n.t("submissions.collect.retain_existing_grading")}
+            </label>
+          </p>
+          <button type="submit" name="commit">
             <FontAwesomeIcon icon="fa-solid fa-file-import" />
             {I18n.t("submissions.collect.this_revision")}
           </button>
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