From 22ad13bbb25330af3c35cf3fdbf1d3e8842042dd Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Sun, 24 Dec 2023 20:44:50 +0200 Subject: [PATCH 01/21] Add #destroy action for Assignment api controller Although routes.rb already generated the endpoint for destroying an assignment, there was no #destroy action in controllers/api/assignments_controller.rb. An authorized instructor should be able to delete an assignment if and only if the assignment does not have any groups. I implemented this #destroy action and wrote corresponding rspec tests in spec/controller/api/assignments_controller_spec.rb. --- app/controllers/api/assignments_controller.rb | 15 ++++++ .../api/assignments_controller_spec.rb | 48 +++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/app/controllers/api/assignments_controller.rb b/app/controllers/api/assignments_controller.rb index e7ed183ce5..569c11c8a3 100644 --- a/app/controllers/api/assignments_controller.rb +++ b/app/controllers/api/assignments_controller.rb @@ -299,6 +299,21 @@ def submit_file upload_file(grouping, only_required_files: assignment.only_required_files) end + def destroy + assignment = Assignment.find_by(id: params[:id]) + if assignment.nil? + render 'shared/http_status', locals: { code: '404', message: 'assignment not found' }, status: :not_found + # render 'shared/http_status', locals: { code: '404', message: I18n.t('tags.not_found') }, status: :not_found + elsif assignment.groups.length != 0 + render 'shared/http_status', + locals: { code: :conflict, message: 'Assignment still has groupings!' }, status: :conflict + else + assignment.destroy + render 'shared/http_status', + locals: { code: '200', message: HttpStatusHelper::ERROR_CODE['message']['200'] }, status: :ok + end + end + protected def implicit_authorization_target diff --git a/spec/controllers/api/assignments_controller_spec.rb b/spec/controllers/api/assignments_controller_spec.rb index 73a8992055..1c9d6275f3 100644 --- a/spec/controllers/api/assignments_controller_spec.rb +++ b/spec/controllers/api/assignments_controller_spec.rb @@ -188,6 +188,11 @@ put :update, params: { id: assignment.id, course_id: course.id } expect(response).to have_http_status(403) end + + it 'should fail to authenticate a DELETE destroy request' do + delete :destroy, params: { id: assignment.id, course_id: course.id } + expect(response).to have_http_status(403) + end end context 'An authenticated instructor request requesting' do @@ -502,6 +507,35 @@ expect(response).to have_http_status(403) end end + context 'DELETE assignment' do + it 'successfully deletes assignment because the assignment has no groups' do + assignment + expect(Assignment.all.length == 1) + delete :destroy, params: { id: assignment.id, course_id: course.id } + expect(response).to have_http_status(200) + expect(Assignment.all.length == 0) + end + + it 'fails to delete assignment because assignment has groups' do + assignment # since lazy let is used for creating an assignment, I invoke it here to trigger its execution + # creates a grouping (and thus a group) for the assignment + create :grouping, assignment: assignment, start_time: nil + expect(assignment.groups).to_not be_empty + delete :destroy, params: { id: assignment.id, course_id: course.id } + expect(response).to have_http_status(409) + expect(Assignment.all.length == 1) + end + + it 'fails to delete assignment because of invalid id' do + assignment # since lazy let is used for creating an assignment, I invoke it here to trigger its execution + # creates a grouping (and thus a group) for the assignment + create :grouping, assignment: assignment, start_time: nil + expect(assignment.groups).to_not be_empty + delete :destroy, params: { id: assignment.id + 1, course_id: course.id } + expect(response).to have_http_status(404) + expect(Assignment.all.length == 1) + end + end end context 'An authenticated student request' do @@ -670,6 +704,13 @@ end end end + + context 'DELETE destroy' do + it 'should fail to authenticate a DELETE destroy request' do + delete :destroy, params: { id: assignment.id, course_id: course.id } + expect(response).to have_http_status(403) + end + end end context 'An authenticated ta request' do @@ -704,5 +745,12 @@ end end end + + context 'DELETE destroy' do + it 'should fail to authenticate a DELETE destroy request' do + delete :destroy, params: { id: assignment.id, course_id: course.id } + expect(response).to have_http_status(403) + end + end end end From e6fdc5ec65daffa2803ea521605b5802a158fdd0 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Sun, 24 Dec 2023 21:49:25 +0200 Subject: [PATCH 02/21] Refining #destroy action and related rspec tests Added informative en.yml messages for the different possible scenarios that can arise when attempting to delete an assignment (using those in the api controller instead of the former hardcoded messages) and improved the style of the expectations in the corresponding rspec tests. --- app/controllers/api/assignments_controller.rb | 14 ++++++++------ config/locales/views/assignments/en.yml | 3 +++ .../api/assignments_controller_spec.rb | 18 +++++++++--------- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/app/controllers/api/assignments_controller.rb b/app/controllers/api/assignments_controller.rb index 569c11c8a3..704d647cfc 100644 --- a/app/controllers/api/assignments_controller.rb +++ b/app/controllers/api/assignments_controller.rb @@ -300,17 +300,19 @@ def submit_file end def destroy - assignment = Assignment.find_by(id: params[:id]) + assignment_id = params[:id] + assignment = Assignment.find_by(id: assignment_id) if assignment.nil? - render 'shared/http_status', locals: { code: '404', message: 'assignment not found' }, status: :not_found - # render 'shared/http_status', locals: { code: '404', message: I18n.t('tags.not_found') }, status: :not_found - elsif assignment.groups.length != 0 + render 'shared/http_status', locals: { code: '404', message: + I18n.t('assignments.assignment_not_found', + invalid_id: assignment_id) }, status: :not_found + elsif !assignment.groups.empty? render 'shared/http_status', - locals: { code: :conflict, message: 'Assignment still has groupings!' }, status: :conflict + locals: { code: :conflict, message: I18n.t('assignments.assignment_has_groupings') }, status: :conflict else assignment.destroy render 'shared/http_status', - locals: { code: '200', message: HttpStatusHelper::ERROR_CODE['message']['200'] }, status: :ok + locals: { code: '200', message: I18n.t('assignments.successful_deletion') }, status: :ok end end diff --git a/config/locales/views/assignments/en.yml b/config/locales/views/assignments/en.yml index e815eef4ad..cf712740a5 100644 --- a/config/locales/views/assignments/en.yml +++ b/config/locales/views/assignments/en.yml @@ -85,3 +85,6 @@ en: upload_config_help_html: "

This zip file will configure and create this assignment.

This does not configure student or grader settings.
For more information please visit this page.

" upload_file_requirement: Filename %{file_name} is not allowed for this assignment. upload_file_requirement_in_folder: Filename %{file_name} in %{file_path} is not allowed for this assignment. + assignment_not_found: No assignment with id '%{invalid_id}'. + assignment_has_groupings: Assignment has groupings. + successful_deletion: Assignment with id '%{invalid_id}' has been successfully deleted. diff --git a/spec/controllers/api/assignments_controller_spec.rb b/spec/controllers/api/assignments_controller_spec.rb index 1c9d6275f3..89afb9b649 100644 --- a/spec/controllers/api/assignments_controller_spec.rb +++ b/spec/controllers/api/assignments_controller_spec.rb @@ -508,12 +508,12 @@ end end context 'DELETE assignment' do - it 'successfully deletes assignment because the assignment has no groups' do - assignment - expect(Assignment.all.length == 1) + it 'should successfully delete assignment because the assignment has no groups' do + assignment # since lazy let is used for creating an assignment, I invoke it here to trigger its execution + expect(assignment.groups).to be_empty delete :destroy, params: { id: assignment.id, course_id: course.id } expect(response).to have_http_status(200) - expect(Assignment.all.length == 0) + expect(Assignment.all).to eq([]) end it 'fails to delete assignment because assignment has groups' do @@ -521,19 +521,19 @@ # creates a grouping (and thus a group) for the assignment create :grouping, assignment: assignment, start_time: nil expect(assignment.groups).to_not be_empty + original_size = Assignment.all.length delete :destroy, params: { id: assignment.id, course_id: course.id } expect(response).to have_http_status(409) - expect(Assignment.all.length == 1) + expect(Assignment.all.length).to eq(original_size) end it 'fails to delete assignment because of invalid id' do assignment # since lazy let is used for creating an assignment, I invoke it here to trigger its execution - # creates a grouping (and thus a group) for the assignment - create :grouping, assignment: assignment, start_time: nil - expect(assignment.groups).to_not be_empty + original_size = Assignment.all.length + # Since we only have one assignment, it is guaranteed that assignment.id + 1 is an invalid id delete :destroy, params: { id: assignment.id + 1, course_id: course.id } expect(response).to have_http_status(404) - expect(Assignment.all.length == 1) + expect(Assignment.all.length).to eq(original_size) end end end From 5fd31f2873b64a90146daa36a677839b879e44ee Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Sun, 24 Dec 2023 22:20:57 +0200 Subject: [PATCH 03/21] finishing-details Corrected a I18 style problem and updated the Changelog (though this will change again to get the PR number). --- Changelog.md | 1 + config/locales/views/assignments/en.yml | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Changelog.md b/Changelog.md index 733af52fc2..fbf392949b 100644 --- a/Changelog.md +++ b/Changelog.md @@ -5,6 +5,7 @@ - Enable plotly rendering for jupyter notebooks (#6871) - Bug Fix: Prevent assigning inactive graders to a group (#6801) - Added new API routes for the `create` and `destroy` actions of SectionsController (#6873) +- Added `destroy` action to `controllers/api/assignments_controller` () ## [v2.4.1] - Internal changes only diff --git a/config/locales/views/assignments/en.yml b/config/locales/views/assignments/en.yml index cf712740a5..4e59b6f675 100644 --- a/config/locales/views/assignments/en.yml +++ b/config/locales/views/assignments/en.yml @@ -1,6 +1,8 @@ --- en: assignments: + assignment_has_groupings: Assignment has groupings. + assignment_not_found: No assignment with id '%{invalid_id}'. average_annotations: "%{average_annotations} annotations per marked submission" configuration_zip_file: Configuration Zip File deadline_with_extension: You have an extension until %{extension_deadline}. @@ -63,6 +65,7 @@ en: title: Starter Files upload_confirmation: The starter files for this assignment have changed since you last downloaded them. Are you sure you want to continue? use_original_filename: Use original filename + successful_deletion: Assignment with id '%{invalid_id}' has been successfully deleted. timed: before_start_instructions: Once you start this timed assessment, you will have %{duration_string} to complete it without penalty. before_start_instructions_extension: Once you start this timed assessment, you will have %{duration_string} to complete it without penalty (this includes a duration extension of %{extension_string}). @@ -85,6 +88,3 @@ en: upload_config_help_html: "

This zip file will configure and create this assignment.

This does not configure student or grader settings.
For more information please visit this page.

" upload_file_requirement: Filename %{file_name} is not allowed for this assignment. upload_file_requirement_in_folder: Filename %{file_name} in %{file_path} is not allowed for this assignment. - assignment_not_found: No assignment with id '%{invalid_id}'. - assignment_has_groupings: Assignment has groupings. - successful_deletion: Assignment with id '%{invalid_id}' has been successfully deleted. From fe7dfd69c23a85baf8491d7c0c43b1b8b054651a Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Sun, 24 Dec 2023 22:57:50 +0200 Subject: [PATCH 04/21] update-changelog --- Changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index fbf392949b..bee4a4a694 100644 --- a/Changelog.md +++ b/Changelog.md @@ -5,7 +5,7 @@ - Enable plotly rendering for jupyter notebooks (#6871) - Bug Fix: Prevent assigning inactive graders to a group (#6801) - Added new API routes for the `create` and `destroy` actions of SectionsController (#6873) -- Added `destroy` action to `controllers/api/assignments_controller` () +- Added `destroy` action to `controllers/api/assignments_controller` (#6880) ## [v2.4.1] - Internal changes only From fdef002e4879beee845d3ebe4cd803e84ca23881 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Sat, 6 Jan 2024 19:09:16 +0200 Subject: [PATCH 05/21] assignment-destroy-with-associations Instead of having the logic that checks if the assignment has any groups inside the api-assignments-controller, this is now taking place in the model itself, simply by including 'dependent: restrict_with_exception' in the association definition. Now, the api-assignments controller has a begin-rescue block to catch this exception. --- app/controllers/api/assignments_controller.rb | 14 ++++++++------ app/models/assignment.rb | 2 +- .../controllers/api/assignments_controller_spec.rb | 5 +++-- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/assignments_controller.rb b/app/controllers/api/assignments_controller.rb index 704d647cfc..8d29e5aeb1 100644 --- a/app/controllers/api/assignments_controller.rb +++ b/app/controllers/api/assignments_controller.rb @@ -306,13 +306,15 @@ def destroy render 'shared/http_status', locals: { code: '404', message: I18n.t('assignments.assignment_not_found', invalid_id: assignment_id) }, status: :not_found - elsif !assignment.groups.empty? - render 'shared/http_status', - locals: { code: :conflict, message: I18n.t('assignments.assignment_has_groupings') }, status: :conflict else - assignment.destroy - render 'shared/http_status', - locals: { code: '200', message: I18n.t('assignments.successful_deletion') }, status: :ok + begin + assignment.destroy + render 'shared/http_status', + locals: { code: '200', message: I18n.t('assignments.average_annotations') }, status: :ok + rescue ActiveRecord::DeleteRestrictionError + render 'shared/http_status', + locals: { code: :conflict, message: I18n.t('assignments.assignment_has_groupings') }, status: :conflict + end end end diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 205d86ea5b..f93cdc2a19 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -74,7 +74,7 @@ class Assignment < Assessment has_many :student_memberships, through: :groupings has_many :submissions, through: :groupings - has_many :groups, through: :groupings + has_many :groups, through: :groupings, dependent: :restrict_with_exception has_many :notes, as: :noteable, dependent: :destroy diff --git a/spec/controllers/api/assignments_controller_spec.rb b/spec/controllers/api/assignments_controller_spec.rb index 89afb9b649..9f8a970b4e 100644 --- a/spec/controllers/api/assignments_controller_spec.rb +++ b/spec/controllers/api/assignments_controller_spec.rb @@ -514,8 +514,8 @@ delete :destroy, params: { id: assignment.id, course_id: course.id } expect(response).to have_http_status(200) expect(Assignment.all).to eq([]) + expect(Assignment.exists?(assignment.id)).to eq(false) end - it 'fails to delete assignment because assignment has groups' do assignment # since lazy let is used for creating an assignment, I invoke it here to trigger its execution # creates a grouping (and thus a group) for the assignment @@ -525,8 +525,8 @@ delete :destroy, params: { id: assignment.id, course_id: course.id } expect(response).to have_http_status(409) expect(Assignment.all.length).to eq(original_size) + expect(assignment.persisted?).to eq(true) end - it 'fails to delete assignment because of invalid id' do assignment # since lazy let is used for creating an assignment, I invoke it here to trigger its execution original_size = Assignment.all.length @@ -534,6 +534,7 @@ delete :destroy, params: { id: assignment.id + 1, course_id: course.id } expect(response).to have_http_status(404) expect(Assignment.all.length).to eq(original_size) + expect(assignment.persisted?).to eq(true) end end end From be27f1dbd0e8f8a1a241048af0cca48e1787678f Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Sun, 7 Jan 2024 01:13:02 +0200 Subject: [PATCH 06/21] fix-small-i18-problem --- app/controllers/api/assignments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/assignments_controller.rb b/app/controllers/api/assignments_controller.rb index 8d29e5aeb1..451239b277 100644 --- a/app/controllers/api/assignments_controller.rb +++ b/app/controllers/api/assignments_controller.rb @@ -310,7 +310,7 @@ def destroy begin assignment.destroy render 'shared/http_status', - locals: { code: '200', message: I18n.t('assignments.average_annotations') }, status: :ok + locals: { code: '200', message: I18n.t('assignments.successful_deletion') }, status: :ok rescue ActiveRecord::DeleteRestrictionError render 'shared/http_status', locals: { code: :conflict, message: I18n.t('assignments.assignment_has_groupings') }, status: :conflict From 3b894412b2cc7e146859982fb1f4a9ce3623825b Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 26 Jan 2024 09:05:39 -0500 Subject: [PATCH 07/21] destroy-action-in-Assignments-controller Added a destroy action to the non-API Assignments controller - and the corresponding route in routes.rb, to be used by the UI (this non-API route will be used for deletion of assignments from the UI). Recall that an assignment should be deleted only if it has no groups. This logic is handled in the model through an appropriate association feature. Nevertheless, this method must NOT be called on an assignment with groups. Its precondition is that the Assignment object with which it is called has no groups. This should be reasonable; for example, the delete button for an assignment should be visible only if the assignment has no groups. --- app/controllers/assignments_controller.rb | 12 ++++++++++++ config/routes.rb | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index fa647bc809..09c1e3a46b 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -657,6 +657,18 @@ def lti_settings render layout: 'assignment_content' end + # NOTE: an assignment should be deleted only if it has no groups. + # However, this logic is handled in the model through an appropriate association feature. + # Nevertheless, this method must NOT be called on an assignment with groups. + # This is ensured because the only endpoint of this method is the 'delete' button on the Assignment-edit + # page, which appears if and only if the assignment has no groups (through erb conditional rendering) + def destroy + @assignment = @record + @assignment.destroy + render template: 'assignments/index' + # flash_message(:success, t('flash.criteria.destroy.success')) + end + private # Configures the automated test files and settings for an +assignment+ provided in the +zip_file+ diff --git a/config/routes.rb b/config/routes.rb index c960e5dd87..7a599aeb86 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -209,7 +209,7 @@ resources :annotation_categories, only: [:show, :destroy, :update] - resources :assignments, except: [:destroy] do + resources :assignments do collection do get 'delete_rejected' get 'batch_runs' From c6aa66d5ee915d88e29284f4c5697bff84832325 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 26 Jan 2024 09:36:11 -0500 Subject: [PATCH 08/21] create-red-button-for-deleting-assignments All elements in the assignment-edit page are found inside the _form.html.erb partial. There, added a new button to delete assignments using the button_to helper, and the button makes a DELETE request to the destroy action of the non-API assignments controller. The button is conditionally rendered based on whether the assignment has any groups (using ERB's templating capabilities). It is visible only if the corresponding assignment has no groups. --- app/views/assignments/_form.html.erb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/views/assignments/_form.html.erb b/app/views/assignments/_form.html.erb index cc3f10ff95..3187fe0f4f 100644 --- a/app/views/assignments/_form.html.erb +++ b/app/views/assignments/_form.html.erb @@ -489,4 +489,17 @@ data: { disable_with: t('working') } %>

<% end %> + + + <% if @assignment.groups.length === 0 %> + <%= button_to "Delete", + course_assignment_path(@current_course, @assignment), + data: { confirm: "Are you sure you want to delete this assignment?" }, + method: 'delete', + class: 'delete-button', + form_class: 'display-inline-block', + title: t('helpers.submit.delete', model: Assignment.model_name.human), + remote: true %> + <% end %> + <% end %> From d69dbb47655a19f1de151eccb4ecf806ecc9cfcf Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 26 Jan 2024 09:40:23 -0500 Subject: [PATCH 09/21] css-class-for-assignment-delete-button CSS class for making the assignment-delete button red, with bold font. This is just temporary and subject to change. Its style does not really align with the general style of Markus' UI. --- app/assets/stylesheets/common/_markus.scss | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/assets/stylesheets/common/_markus.scss b/app/assets/stylesheets/common/_markus.scss index 86e2fe5150..378ef9f6b6 100644 --- a/app/assets/stylesheets/common/_markus.scss +++ b/app/assets/stylesheets/common/_markus.scss @@ -301,6 +301,15 @@ button, min-width: 70px; padding: 0.25em 0.5em; } + + &.delete-button { + background: #f00; + color: #c0c0c0; + font-weight: bold; + margin: 0 2pt; + min-width: 150px; + padding: 0.25em 0.5em; + } } /** Fieldset */ From 0aadea00f6b7f60555ce29482bbed77d19253914 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 26 Jan 2024 14:20:54 -0500 Subject: [PATCH 10/21] conditional-rendering-correction The red button should only be rendered if (1) you are in the assignment-edit page and (2) the current asisgnment has no groups. Previously, only (2) was being checked (i.e. that the assignment has no groups). So if, for example, you went to create a new assignment, the condition of the if statement did not fail and rails attempted to render the button, leading to error because in most cases @assignment is undefined (nil). I added the condition that we are in the context of the edit action of the assignment controller to face this issue (only in that case will the delete button be rendered). --- app/views/assignments/_form.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/assignments/_form.html.erb b/app/views/assignments/_form.html.erb index 3187fe0f4f..c8b4384815 100644 --- a/app/views/assignments/_form.html.erb +++ b/app/views/assignments/_form.html.erb @@ -491,7 +491,7 @@ <% end %> - <% if @assignment.groups.length === 0 %> + <% if (@assignment.groups.length === 0) and (action_name=="edit")%> <%= button_to "Delete", course_assignment_path(@current_course, @assignment), data: { confirm: "Are you sure you want to delete this assignment?" }, From d2052e50a14cbed1acff6ecd487b7c179962480d Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 2 Feb 2024 03:56:29 -0500 Subject: [PATCH 11/21] feat: flash message for successful deletion of assignment Utilised the external class FlashResponder class to automatically place the appropriate success message in the ActionDispatch:Flash object. This strategy already takes place in the Assignments controller in the #create and #update actions. In particular, given the line , the FlashResponder checks the status of the @assignment --that is, if it was just created, updated, or destroyed-- and places the appropriate message in the flash object (which should be defined in the appropriate format in a en.yml file). Then, by definition of how the flash object works, the next (redirected-to) action (and thus the view rendered by the next action) has access to the data stored in flash. In this particular case, the assignment_content layout view renders the _flash_message.html.erb partial, and it is this partial that used the flash object (doing flash[:...]). --- app/controllers/assignments_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index 09c1e3a46b..3d416e92a2 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -665,8 +665,7 @@ def lti_settings def destroy @assignment = @record @assignment.destroy - render template: 'assignments/index' - # flash_message(:success, t('flash.criteria.destroy.success')) + respond_with @assignment, location: -> { course_assignments_path(current_course, @assignment) } end private From 83170fce9a16f110545deeb85c584b96928ec286 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 2 Feb 2024 14:52:04 -0500 Subject: [PATCH 12/21] error handling in assignments#destroy Handling the case where the action is called with an Assignment that still has groups. This is impossible from the UI (since the delete button only shows if the assignment does not have any groups), but it possible from direct HTTP requests. --- app/controllers/assignments_controller.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index 3d416e92a2..984e7be68d 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -664,8 +664,13 @@ def lti_settings # page, which appears if and only if the assignment has no groups (through erb conditional rendering) def destroy @assignment = @record - @assignment.destroy - respond_with @assignment, location: -> { course_assignments_path(current_course, @assignment) } + begin + @assignment.destroy + respond_with @assignment, location: -> { course_assignments_path(current_course, @assignment) } + rescue ActiveRecord::DeleteRestrictionError + flash_message(:error, I18n.t('assignments.assignment_has_groupings')) + redirect_to action: :show + end end private From e539b81f32a4d2aab14b9d38b3d5158449b3cc21 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 2 Feb 2024 16:27:51 -0500 Subject: [PATCH 13/21] tests-for-assignment#destroy Testing behaviour when request is not authorized (fail to destroy), when the request is authorized but the assignment still has groups (fail to destroy), and when the request is authorized and the assignment has no groups (succeeds to destroy). --- .../assignments_controller_spec.rb | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/spec/controllers/assignments_controller_spec.rb b/spec/controllers/assignments_controller_spec.rb index 744a28ba75..6e2144f62a 100644 --- a/spec/controllers/assignments_controller_spec.rb +++ b/spec/controllers/assignments_controller_spec.rb @@ -2014,4 +2014,32 @@ expect(response).to have_http_status(200) end end + + context '#destroy' do + let!(:instructor) { create :instructor } + let!(:course) { instructor.course } + let!(:assignment) { create :assignment } + let(:grouping) { create :grouping, assignment: assignment } + it 'should fail to DELETE because of unauthorized request' do + delete :destroy, params: { course_id: course.id, id: assignment.id } + expect(Assignment.exists?(assignment.id)).to be(true) + end + it 'should fail to DELETE because assignment has groups' do + grouping # lazy initialization + delete_as instructor, :destroy, params: { course_id: course.id, id: assignment.id } + expect(Assignment.exists?(assignment.id)).to be(true) + expect(flash[:error]).to eq(["

#{I18n.t('assignments.assignment_has_groupings')}

"]) + expect(flash.to_hash.length).to eq(1) + expect(flash[:error].length).to eq(1) + expect(response).to have_http_status(302) + end + it 'should successfully DELETE assignment (no groups)' do + delete_as instructor, :destroy, params: { course_id: course.id, id: assignment.id } + expect(Assignment.exists?(assignment.id)).to be(false) + expect(flash[:success]).to eq(I18n.t('flash.actions.destroy.success', + resource_name: assignment.short_identifier)) + expect(flash.to_hash.length).to eq(1) + expect(response).to have_http_status(302) + end + end end From 742ba72fb7542ced7ea1d56ed6c730a16f391a02 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Wed, 7 Feb 2024 01:21:54 -0500 Subject: [PATCH 14/21] destroy-button-improvements Firstly, instead of completely hiding the assignment-delete button for assignments with groups (in their 'edit' pages), now it is visible but disabled, and when the mouse is over it a tooltip appears saying 'Assignment has groupings'. Also, in the case where the button is enabled (i.e. for assignments with no groups), I improved the style by using Markus' predefined constants (found in assets/stylesheets/common/_constants.scss) and defining a new style for the id #assignment-delete (targeting specifically that delete button); the button also is of class .button, utilising predefined styling. The only two files modified for these changes are the partial app/views/assignments/_form.html.erb and the stylesheet app/assets/stylesheets/common/_markus.scss. --- app/assets/stylesheets/common/_markus.scss | 13 +++++-------- app/views/assignments/_form.html.erb | 19 ++++++++++++++++--- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/app/assets/stylesheets/common/_markus.scss b/app/assets/stylesheets/common/_markus.scss index 378ef9f6b6..a812d3a443 100644 --- a/app/assets/stylesheets/common/_markus.scss +++ b/app/assets/stylesheets/common/_markus.scss @@ -301,15 +301,12 @@ button, min-width: 70px; padding: 0.25em 0.5em; } +} - &.delete-button { - background: #f00; - color: #c0c0c0; - font-weight: bold; - margin: 0 2pt; - min-width: 150px; - padding: 0.25em 0.5em; - } +#assignment-delete { + background: $severe-error; + color: $background-main; + font-weight: bold; } /** Fieldset */ diff --git a/app/views/assignments/_form.html.erb b/app/views/assignments/_form.html.erb index c8b4384815..e212d5db8e 100644 --- a/app/views/assignments/_form.html.erb +++ b/app/views/assignments/_form.html.erb @@ -491,15 +491,28 @@ <% end %> - <% if (@assignment.groups.length === 0) and (action_name=="edit")%> + <% if (action_name=="edit") and (@assignment.groups.length === 0) %> <%= button_to "Delete", course_assignment_path(@current_course, @assignment), + { data: { confirm: "Are you sure you want to delete this assignment?" }, method: 'delete', - class: 'delete-button', + class: 'button', + id: 'assignment-delete', form_class: 'display-inline-block', title: t('helpers.submit.delete', model: Assignment.model_name.human), - remote: true %> + remote: true + } + %> + + <% elsif (action_name=="edit") and (@assignment.groups.length !=0) %> + <%= button_to "Delete", + course_assignment_path(@current_course, @assignment), + { + title: I18n.t('assignments.assignment_has_groupings'), + disabled: true, + } + %> <% end %> <% end %> From b2cb58bceb47b269f54c31d909e04b81760e4413 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Wed, 7 Feb 2024 01:51:47 -0500 Subject: [PATCH 15/21] updating-changelog --- Changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index dea3a954c0..8aeb52916a 100644 --- a/Changelog.md +++ b/Changelog.md @@ -19,7 +19,7 @@ - Bug Fix: Prevent assigning inactive graders to a group (#6801) - Added new API routes for the `create` and `destroy` actions of SectionsController (#6873) - Display error message in real-time when running automated tests, rather than waiting for page refresh (#6878) -- - Added `destroy` action to `controllers/api/assignments_controller` (#6880) +- Allow deletion of assignments with no groups (#6880) ## [v2.4.1] - Internal changes only From 23da5541454955122ff9225b94f9aa0312aab9ba Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Wed, 7 Feb 2024 01:54:53 -0500 Subject: [PATCH 16/21] fix-changelog change belonged to unreleased section --- Changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index 8aeb52916a..3ddc0abd21 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,6 +1,7 @@ # Changelog ## [unreleased] +- Allow deletion of assignments with no groups (#6880) - Add workaround for CSP in Safari < 16 (#6947) ## [v2.4.4] @@ -19,7 +20,6 @@ - Bug Fix: Prevent assigning inactive graders to a group (#6801) - Added new API routes for the `create` and `destroy` actions of SectionsController (#6873) - Display error message in real-time when running automated tests, rather than waiting for page refresh (#6878) -- Allow deletion of assignments with no groups (#6880) ## [v2.4.1] - Internal changes only From 70ab80527745ad31dc18db117c1bf980a55866d2 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Mon, 12 Feb 2024 01:01:37 -0500 Subject: [PATCH 17/21] applying-feedback Most notably, simplifying the erb code in the _form partial for assignments -- for when to show the delete button (and when it should be disabled). Includes also other minor changes. --- app/controllers/api/assignments_controller.rb | 3 +- app/controllers/assignments_controller.rb | 7 +--- app/views/assignments/_form.html.erb | 32 +++++++------------ 3 files changed, 14 insertions(+), 28 deletions(-) diff --git a/app/controllers/api/assignments_controller.rb b/app/controllers/api/assignments_controller.rb index 451239b277..553b2d44ca 100644 --- a/app/controllers/api/assignments_controller.rb +++ b/app/controllers/api/assignments_controller.rb @@ -300,8 +300,7 @@ def submit_file end def destroy - assignment_id = params[:id] - assignment = Assignment.find_by(id: assignment_id) + assignment = record if assignment.nil? render 'shared/http_status', locals: { code: '404', message: I18n.t('assignments.assignment_not_found', diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index 984e7be68d..ee48e7c2fa 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -657,11 +657,6 @@ def lti_settings render layout: 'assignment_content' end - # NOTE: an assignment should be deleted only if it has no groups. - # However, this logic is handled in the model through an appropriate association feature. - # Nevertheless, this method must NOT be called on an assignment with groups. - # This is ensured because the only endpoint of this method is the 'delete' button on the Assignment-edit - # page, which appears if and only if the assignment has no groups (through erb conditional rendering) def destroy @assignment = @record begin @@ -669,7 +664,7 @@ def destroy respond_with @assignment, location: -> { course_assignments_path(current_course, @assignment) } rescue ActiveRecord::DeleteRestrictionError flash_message(:error, I18n.t('assignments.assignment_has_groupings')) - redirect_to action: :show + redirect_back fallback_location: { action: :edit, id: @assignment.id } end end diff --git a/app/views/assignments/_form.html.erb b/app/views/assignments/_form.html.erb index e212d5db8e..f1d168b1cf 100644 --- a/app/views/assignments/_form.html.erb +++ b/app/views/assignments/_form.html.erb @@ -490,29 +490,21 @@

<% end %> - - <% if (action_name=="edit") and (@assignment.groups.length === 0) %> - <%= button_to "Delete", + <% if (action_name=="edit") %> + <% has_groups = @assignment.groups.length != 0 %> + <%= button_to t(:delete), course_assignment_path(@current_course, @assignment), { - data: { confirm: "Are you sure you want to delete this assignment?" }, - method: 'delete', - class: 'button', - id: 'assignment-delete', - form_class: 'display-inline-block', - title: t('helpers.submit.delete', model: Assignment.model_name.human), - remote: true + data: { confirm: "Are you sure you want to delete this assignment?" }, + method: 'delete', + class: has_groups ? nil : 'button', + id: has_groups ? nil : 'assignment-delete', + form_class: 'display-inline-block', + title: has_groups ? I18n.t('assignments.assignment_has_groupings') : + I18n.t('helpers.submit.delete', model: Assignment.model_name.human), + disabled: has_groups } %> - - <% elsif (action_name=="edit") and (@assignment.groups.length !=0) %> - <%= button_to "Delete", - course_assignment_path(@current_course, @assignment), - { - title: I18n.t('assignments.assignment_has_groupings'), - disabled: true, - } - %> - <% end %> + <%end %> <% end %> From 05f37349757139e2cfcf5ad232935e0b306a4aa7 Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Mon, 12 Feb 2024 01:14:45 -0500 Subject: [PATCH 18/21] removing-redundant-assertion --- spec/controllers/api/assignments_controller_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/controllers/api/assignments_controller_spec.rb b/spec/controllers/api/assignments_controller_spec.rb index 9f8a970b4e..15745e9fe6 100644 --- a/spec/controllers/api/assignments_controller_spec.rb +++ b/spec/controllers/api/assignments_controller_spec.rb @@ -513,7 +513,6 @@ expect(assignment.groups).to be_empty delete :destroy, params: { id: assignment.id, course_id: course.id } expect(response).to have_http_status(200) - expect(Assignment.all).to eq([]) expect(Assignment.exists?(assignment.id)).to eq(false) end it 'fails to delete assignment because assignment has groups' do From 23e21d35ba25a0b8ae68ca030601be1fe4116f0d Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 16 Feb 2024 09:48:55 -0500 Subject: [PATCH 19/21] applying-feedback Includes defining strings in locale file, improving the css, and small formatting fixes. --- app/assets/stylesheets/common/_markus.scss | 10 +++++----- app/controllers/api/assignments_controller.rb | 20 +++++++------------ app/controllers/assignments_controller.rb | 2 +- app/views/assignments/_form.html.erb | 10 ++++------ config/locales/views/assignments/en.yml | 1 + .../api/assignments_controller_spec.rb | 5 +---- 6 files changed, 19 insertions(+), 29 deletions(-) diff --git a/app/assets/stylesheets/common/_markus.scss b/app/assets/stylesheets/common/_markus.scss index a812d3a443..8de45a1841 100644 --- a/app/assets/stylesheets/common/_markus.scss +++ b/app/assets/stylesheets/common/_markus.scss @@ -301,12 +301,12 @@ button, min-width: 70px; padding: 0.25em 0.5em; } -} -#assignment-delete { - background: $severe-error; - color: $background-main; - font-weight: bold; + &.danger-button { + background: $severe-error; + color: $background-main; + font-weight: bold; + } } /** Fieldset */ diff --git a/app/controllers/api/assignments_controller.rb b/app/controllers/api/assignments_controller.rb index 553b2d44ca..8aa09d25f5 100644 --- a/app/controllers/api/assignments_controller.rb +++ b/app/controllers/api/assignments_controller.rb @@ -301,19 +301,13 @@ def submit_file def destroy assignment = record - if assignment.nil? - render 'shared/http_status', locals: { code: '404', message: - I18n.t('assignments.assignment_not_found', - invalid_id: assignment_id) }, status: :not_found - else - begin - assignment.destroy - render 'shared/http_status', - locals: { code: '200', message: I18n.t('assignments.successful_deletion') }, status: :ok - rescue ActiveRecord::DeleteRestrictionError - render 'shared/http_status', - locals: { code: :conflict, message: I18n.t('assignments.assignment_has_groupings') }, status: :conflict - end + begin + assignment.destroy + render 'shared/http_status', + locals: { code: '200', message: I18n.t('assignments.successful_deletion') }, status: :ok + rescue ActiveRecord::DeleteRestrictionError + render 'shared/http_status', + locals: { code: :conflict, message: I18n.t('assignments.assignment_has_groupings') }, status: :conflict end end diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index ee48e7c2fa..2d52771703 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -658,7 +658,7 @@ def lti_settings end def destroy - @assignment = @record + @assignment = record begin @assignment.destroy respond_with @assignment, location: -> { course_assignments_path(current_course, @assignment) } diff --git a/app/views/assignments/_form.html.erb b/app/views/assignments/_form.html.erb index f1d168b1cf..94d41a6cfc 100644 --- a/app/views/assignments/_form.html.erb +++ b/app/views/assignments/_form.html.erb @@ -490,21 +490,19 @@

<% end %> - <% if (action_name=="edit") %> + <% if action_name == "edit" %> <% has_groups = @assignment.groups.length != 0 %> <%= button_to t(:delete), course_assignment_path(@current_course, @assignment), { - data: { confirm: "Are you sure you want to delete this assignment?" }, + data: { confirm: I18n.t("assignments.deletion_confirmation") }, method: 'delete', - class: has_groups ? nil : 'button', - id: has_groups ? nil : 'assignment-delete', + class: has_groups ? nil : 'danger-button', form_class: 'display-inline-block', title: has_groups ? I18n.t('assignments.assignment_has_groupings') : I18n.t('helpers.submit.delete', model: Assignment.model_name.human), disabled: has_groups } %> - <%end %> - + <% end %> <% end %> diff --git a/config/locales/views/assignments/en.yml b/config/locales/views/assignments/en.yml index 4e59b6f675..742166965c 100644 --- a/config/locales/views/assignments/en.yml +++ b/config/locales/views/assignments/en.yml @@ -66,6 +66,7 @@ en: upload_confirmation: The starter files for this assignment have changed since you last downloaded them. Are you sure you want to continue? use_original_filename: Use original filename successful_deletion: Assignment with id '%{invalid_id}' has been successfully deleted. + deletion_confirmation: Are you sure you want to delete this assignment? timed: before_start_instructions: Once you start this timed assessment, you will have %{duration_string} to complete it without penalty. before_start_instructions_extension: Once you start this timed assessment, you will have %{duration_string} to complete it without penalty (this includes a duration extension of %{extension_string}). diff --git a/spec/controllers/api/assignments_controller_spec.rb b/spec/controllers/api/assignments_controller_spec.rb index 15745e9fe6..3f6fc4a9a2 100644 --- a/spec/controllers/api/assignments_controller_spec.rb +++ b/spec/controllers/api/assignments_controller_spec.rb @@ -509,15 +509,12 @@ end context 'DELETE assignment' do it 'should successfully delete assignment because the assignment has no groups' do - assignment # since lazy let is used for creating an assignment, I invoke it here to trigger its execution expect(assignment.groups).to be_empty delete :destroy, params: { id: assignment.id, course_id: course.id } expect(response).to have_http_status(200) expect(Assignment.exists?(assignment.id)).to eq(false) end it 'fails to delete assignment because assignment has groups' do - assignment # since lazy let is used for creating an assignment, I invoke it here to trigger its execution - # creates a grouping (and thus a group) for the assignment create :grouping, assignment: assignment, start_time: nil expect(assignment.groups).to_not be_empty original_size = Assignment.all.length @@ -527,7 +524,7 @@ expect(assignment.persisted?).to eq(true) end it 'fails to delete assignment because of invalid id' do - assignment # since lazy let is used for creating an assignment, I invoke it here to trigger its execution + assignment # since lazy let is used for creating an assignment, it is invoked here to trigger its execution original_size = Assignment.all.length # Since we only have one assignment, it is guaranteed that assignment.id + 1 is an invalid id delete :destroy, params: { id: assignment.id + 1, course_id: course.id } From 654025356249e3313f7d522446675960c00469dc Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 16 Feb 2024 10:18:47 -0500 Subject: [PATCH 20/21] normalizing-i18 --- config/locales/views/assignments/en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/locales/views/assignments/en.yml b/config/locales/views/assignments/en.yml index 742166965c..f8939236d1 100644 --- a/config/locales/views/assignments/en.yml +++ b/config/locales/views/assignments/en.yml @@ -6,6 +6,7 @@ en: average_annotations: "%{average_annotations} annotations per marked submission" configuration_zip_file: Configuration Zip File deadline_with_extension: You have an extension until %{extension_deadline}. + deletion_confirmation: Are you sure you want to delete this assignment? due_date: final_due_date_passed: The deadline, including late or grace periods, has passed. marking_overwrite_warning_html: "Any previous grading work will be lost." @@ -66,7 +67,6 @@ en: upload_confirmation: The starter files for this assignment have changed since you last downloaded them. Are you sure you want to continue? use_original_filename: Use original filename successful_deletion: Assignment with id '%{invalid_id}' has been successfully deleted. - deletion_confirmation: Are you sure you want to delete this assignment? timed: before_start_instructions: Once you start this timed assessment, you will have %{duration_string} to complete it without penalty. before_start_instructions_extension: Once you start this timed assessment, you will have %{duration_string} to complete it without penalty (this includes a duration extension of %{extension_string}). From ae965852ae2850f6115b26f6a7ad459a6f352dbb Mon Sep 17 00:00:00 2001 From: Mimis Chlympatsos Date: Fri, 16 Feb 2024 10:23:08 -0500 Subject: [PATCH 21/21] removing-unused-locales --- config/locales/views/assignments/en.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/config/locales/views/assignments/en.yml b/config/locales/views/assignments/en.yml index f8939236d1..26cbd34ea6 100644 --- a/config/locales/views/assignments/en.yml +++ b/config/locales/views/assignments/en.yml @@ -2,7 +2,6 @@ en: assignments: assignment_has_groupings: Assignment has groupings. - assignment_not_found: No assignment with id '%{invalid_id}'. average_annotations: "%{average_annotations} annotations per marked submission" configuration_zip_file: Configuration Zip File deadline_with_extension: You have an extension until %{extension_deadline}.