Allow deletion of Assignment's with no groups #6880

mimischly7 · 2023-12-24T20:29:49Z

Motivation and Context

An authorized instructor should be able to delete an assignment if and only if the assignment does not have any groups.
Although routes.rb already generated the endpoint for destroying an assignment, there was no #destroy action in controllers/api/assignments_controller.rb.

Your Changes

Description:
I implemented a #destroy action in controllers/api/assignments_controller.rb which deletes the Assignment corresponding to the given id if this assignment does not have any groups. The logic for checking whether an assignment has any groups is performed at the model level using the dependent: restrict_with_exception attribute of rails associations. The controller catches the potential exception and responds to the request accordingly.

Type of change (select all that apply):

New feature (non-breaking change which adds functionality)

Testing

Wrote several rspec tests in spec/controller/api/assignments_controller_spec.rb aiming to capture the various scenarios and contexts under which one may attempt to delete an assignment.

Checklist

I have performed a self-review of my own code.
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported on Coveralls.
I have added tests for my changes.
I have updated the Changelog.md file.
I have made changes to the documentation and linked to that pull request below.

Pull request to make documentation changes (if applicable)

MarkUsProject/Wiki#201

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.

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.

Corrected a I18 style problem and updated the Changelog (though this will change again to get the PR number).

coveralls · 2023-12-24T20:51:38Z

Pull Request Test Coverage Report for Build 7932654664

Details

0 of 69 (100.0%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 91.733%

Totals
Change from base Build 7925404029:	0.01%
Covered Lines:	38935
Relevant Lines:	41822

💛 - Coveralls

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.

david-yz-liu

@mimischly7 good! So in addition to this change, I want you to add a button to delete the assignment from the web interface (and more specifically from the assignment edit form.

Add the button and give it a red background using one of the built-in colour constants, so that it's very clearly distinguished from other buttons. Make sure the button is disabled if there are any groups, and that there is a confirmation dialog box. This will need a parallel assignments controller route on the non-API controller.

david-yz-liu · 2024-01-14T21:13:44Z

Changelog.md

@@ -7,6 +7,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)


There's an extra - here. Also, please do a pull from master; we've made a new minor release, so you'll need to add this to the new [unreleased] section

It seems you missed this

Synchronization-with-upstream

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.

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.

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.

@assignment

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).

…ent-destroy-task Merging upstream.

@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 <respond_with @assignment ...>, 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[:...]).

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.

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).

…ent-destroy-task merging-with-upstream

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.

change belonged to unreleased section

…ent-destroy-task updating-with-upstream

david-yz-liu

@mimischly7 great work! Sorry for the delay on reviewing this. I left a few more comments for you to polish this PR.

david-yz-liu · 2024-01-31T14:59:10Z

Changelog.md

@@ -7,6 +7,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)


It seems you missed this

david-yz-liu · 2024-01-31T14:59:54Z

app/assets/stylesheets/common/_markus.scss

@@ -301,6 +301,15 @@ button,
    min-width: 70px;
    padding: 0.25em 0.5em;
  }
+
+  &.delete-button {
+    background: #f00;


For colours, you should always use existing colour constants (start by looking in _constants.scss)

david-yz-liu · 2024-01-31T15:02:34Z

app/assets/stylesheets/common/_markus.scss

@@ -301,6 +301,15 @@ button,
    min-width: 70px;
    padding: 0.25em 0.5em;
  }
+
+  &.delete-button {


A more appropriate class name is danger-button (connoting a "dangerous" action, which might not just be limited to deletion in the future)

app/assets/stylesheets/common/_markus.scss

david-yz-liu · 2024-01-31T15:05:13Z

app/controllers/api/assignments_controller.rb

+      assignment_id = params[:id]
+      assignment = Assignment.find_by(id: assignment_id)
+      if assignment.nil?
+        render 'shared/http_status', locals: { code: '404', message:
+          I18n.t('assignments.assignment_not_found',
+                 invalid_id: assignment_id) }, status: :not_found


This logic is not necessary; look at other methods in this class. In particular, the record method will give you the Assignment instance corresponding to params[:id]. There's existing code that computes this and handles missing objects more generally across all controllers.

Given our discussion on Friday on whether we should check in the controller whether the requested record is actually valid, even though the SessionHandler class method check_record (as a before_action method) already checks that, I decided to keep the extra check. Let me know if you want me to remove it.

david-yz-liu · 2024-01-31T15:10:03Z

app/views/assignments/_form.html.erb

+
+<!--Conditional Rendering-->
+  <% if (@assignment.groups.length === 0) and (action_name=="edit")%>
+    <%= button_to "Delete",


Remember that no string literals should appear in your code. Always define strings in locale files.

david-yz-liu · 2024-01-31T15:10:18Z

app/views/assignments/_form.html.erb

+                  form_class: 'display-inline-block',
+                  title: t('helpers.submit.delete', model: Assignment.model_name.human),
+                  remote: true %>
+  <% end  %>


there is an extra space here

david-yz-liu · 2024-01-31T15:10:57Z

app/views/assignments/_form.html.erb

+                  class: 'delete-button',
+                  form_class: 'display-inline-block',
+                  title: t('helpers.submit.delete', model: Assignment.model_name.human),
+                  remote: true %>


the remote: true is not necessary (I encourage you to look up this option to understand why)

I think I see why. If I understand correctly, by setting remote:true we allow the generated form to be submitted asynchronously (with AJAX), but this requireds that we handle it appropriately in the respond_to block in our action with a case for javascript; since we are not doing this in the action, there is no point.

david-yz-liu · 2024-01-31T15:11:05Z

app/views/assignments/_form.html.erb

+                  title: t('helpers.submit.delete', model: Assignment.model_name.human),
+                  remote: true %>
+  <% end  %>
+


delete this line

This line hasn't been deleted

david-yz-liu · 2024-01-31T15:12:03Z

spec/controllers/api/assignments_controller_spec.rb

+        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([])


Remove this assertion. The next line is more specific to what you're actually testing

…ent-destroy-task pulling-master

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.

david-yz-liu · 2024-02-16T03:00:59Z

app/views/assignments/_form.html.erb

+                  title: t('helpers.submit.delete', model: Assignment.model_name.human),
+                  remote: true %>
+  <% end  %>
+


This line hasn't been deleted

david-yz-liu · 2024-02-16T03:01:21Z

app/assets/stylesheets/common/_markus.scss

@@ -303,6 +303,12 @@ button,
  }
 }

+#assignment-delete {


Okay this is better, but (1) make it a class, and (2) call the class danger-button, which is more general.

david-yz-liu · 2024-02-16T03:02:12Z

app/controllers/api/assignments_controller.rb

@@ -299,6 +299,24 @@ def submit_file
      upload_file(grouping, only_required_files: assignment.only_required_files)
    end

+    def destroy
+      assignment = record
+      if assignment.nil?


This check isn't necessary, you can remove it. The check_record helper is sufficient to check for this

david-yz-liu · 2024-02-16T03:02:28Z

app/controllers/assignments_controller.rb

@@ -657,6 +657,17 @@ def lti_settings
    render layout: 'assignment_content'
  end

+  def destroy
+    @assignment = @record


Use record (as with the other methods)

david-yz-liu · 2024-02-16T03:03:09Z

app/views/assignments/_form.html.erb

@@ -489,4 +489,22 @@
                   data: { disable_with: t('working') } %>
    </p>
  <% end %>
+
+  <% if (action_name=="edit") %>


If statements don't need parentheses in Ruby. Also, put spaces around ==

david-yz-liu · 2024-02-16T03:03:25Z

app/views/assignments/_form.html.erb

+    <%= button_to t(:delete),
+                  course_assignment_path(@current_course, @assignment),
+                  {
+                    data: { confirm: "Are you sure you want to delete this assignment?" },


This string also needs to be internationalized

david-yz-liu · 2024-02-16T03:04:29Z

app/views/assignments/_form.html.erb

+                    data: { confirm: "Are you sure you want to delete this assignment?" },
+                    method: 'delete',
+                    class: has_groups ? nil : 'button',
+                    id: has_groups ? nil : 'assignment-delete',


Following my above comment, don't use an id here, instead conditionally add a second class to 'button' above

david-yz-liu · 2024-02-16T03:05:03Z

app/views/assignments/_form.html.erb

+                    disabled: has_groups
+                  }
+    %>
+  <%end %>


There is a space missing after the <%

david-yz-liu · 2024-02-16T03:06:07Z

spec/controllers/api/assignments_controller_spec.rb

@@ -502,6 +507,35 @@
        expect(response).to have_http_status(403)
      end
    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


This isn't necessary, as assignment is immediately used on the next line

david-yz-liu · 2024-02-16T03:06:14Z

spec/controllers/api/assignments_controller_spec.rb

+        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


Same comment as above

Includes defining strings in locale file, improving the css, and small formatting fixes.

…s into assignment-destroy-task merge-origin

mimischly7 added 3 commits December 24, 2023 20:44

finishing-details

5fd31f2

Corrected a I18 style problem and updated the Changelog (though this will change again to get the PR number).

mimischly7 marked this pull request as draft December 24, 2023 20:29

mimischly7 requested a review from david-yz-liu December 24, 2023 20:56

mimischly7 added 3 commits December 24, 2023 22:57

update-changelog

fe7dfd6

merge-with-upstream

351a03a

mimischly7 marked this pull request as ready for review January 6, 2024 22:34

fix-small-i18-problem

be27f1d

david-yz-liu reviewed Jan 14, 2024

View reviewed changes

mimischly7 added 5 commits January 26, 2024 06:41

Merge branch 'master' into assignment-destroy-task-pt2

e74d214

Synchronization-with-upstream

css-class-for-assignment-delete-button

d69dbb4

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.

mimischly7 requested a review from david-yz-liu January 26, 2024 19:56

mimischly7 added 6 commits February 2, 2024 03:55

Merge branch 'master' of github.com:MarkUsProject/Markus into assignm…

b183f66

…ent-destroy-task Merging upstream.

error handling in assignments#destroy

83170fc

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.

tests-for-assignment#destroy

e539b81

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).

Merge branch 'master' of github.com:MarkUsProject/Markus into assignm…

0e1d8b3

…ent-destroy-task merging-with-upstream

mimischly7 removed the request for review from david-yz-liu February 7, 2024 06:23

mimischly7 added the enhancement label Feb 7, 2024

mimischly7 self-assigned this Feb 7, 2024

updating-changelog

b2cb58b

mimischly7 requested a review from david-yz-liu February 7, 2024 06:52

fix-changelog

23da554

change belonged to unreleased section

mimischly7 removed their assignment Feb 7, 2024

Merge branch 'master' of github.com:MarkUsProject/Markus into assignm…

ead7c57

…ent-destroy-task updating-with-upstream

david-yz-liu reviewed Feb 10, 2024

View reviewed changes

mimischly7 added 3 commits February 11, 2024 23:27

Merge branch 'master' of github.com:MarkUsProject/Markus into assignm…

8d8a896

…ent-destroy-task pulling-master

applying-feedback

70ab805

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.

removing-redundant-assertion

05f3734

mimischly7 requested a review from david-yz-liu February 12, 2024 08:20

Merge branch 'master' into assignment-destroy-task

b612a5a

david-yz-liu reviewed Feb 16, 2024

View reviewed changes

mimischly7 added 5 commits February 16, 2024 09:06

merge-master

3779d5e

applying-feedback

23e21d3

Includes defining strings in locale file, improving the css, and small formatting fixes.

Merge branch 'assignment-destroy-task' of github.com:mimischly7/Marku…

90603ac

…s into assignment-destroy-task merge-origin

normalizing-i18

6540253

removing-unused-locales

ae96585

mimischly7 requested a review from david-yz-liu February 16, 2024 16:09

david-yz-liu approved these changes Feb 17, 2024

View reviewed changes

david-yz-liu merged commit a9a4504 into MarkUsProject:master Feb 17, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow deletion of Assignment's with no groups #6880

Allow deletion of Assignment's with no groups #6880

mimischly7 commented Dec 24, 2023 •

edited

Loading

coveralls commented Dec 24, 2023 •

edited

Loading

david-yz-liu left a comment

david-yz-liu Jan 14, 2024

david-yz-liu Jan 31, 2024

david-yz-liu left a comment

david-yz-liu Jan 31, 2024

david-yz-liu Jan 31, 2024

david-yz-liu Jan 31, 2024

david-yz-liu Jan 31, 2024

mimischly7 Feb 12, 2024

david-yz-liu Jan 31, 2024

david-yz-liu Jan 31, 2024

david-yz-liu Jan 31, 2024

mimischly7 Feb 12, 2024

david-yz-liu Jan 31, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Jan 31, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Feb 16, 2024

david-yz-liu Feb 16, 2024

Allow deletion of Assignment's with no groups #6880

Allow deletion of Assignment's with no groups #6880

Conversation

mimischly7 commented Dec 24, 2023 • edited Loading

Motivation and Context

Your Changes

Testing

Checklist

Pull request to make documentation changes (if applicable)

coveralls commented Dec 24, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7932654664

Details

💛 - Coveralls

david-yz-liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-yz-liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimischly7 commented Dec 24, 2023 •

edited

Loading

coveralls commented Dec 24, 2023 •

edited

Loading