Enable the deletion of Grade Entry Forms that have no grades #6915
Conversation
for more information, see https://pre-commit.ci
… delete-mark-spreadsheet Resolve any conflicts with new changes
…Markus into delete-mark-spreadsheet
Bruce-8
changed the title
app/models/grade_entry_item.rb
Outdated
| @@ -5,7 +5,7 @@ class GradeEntryItem < ApplicationRecord | |||
|
|
|||
| has_one :course, through: :grade_entry_form | |||
|
|
|||
| has_many :grades, dependent: :delete_all | |||
| has_many :grades, dependent: :destroy | |||
There was a problem hiding this comment.
Okay I realized after reviewing again that this change will also change what happens when the user tries to destroy a single column (on the grade entry form "update" page). Previously, we actually did allow the user to delete the column even if that would result in non-nil grades being deleted.
I don't want to change that existing behaviour in this PR. So, please revert the added Grade destroy callback and this line, and instead add a callback to GradeEntryForm itself to check for non-nil grades. See also my comment on the form below about how I want you to check for this.
Sorry for the change of plans!
| @@ -89,3 +89,12 @@ | |||
| data: { disable_with: t('working') } %> | |||
| </p> | |||
| <% end %> | |||
|
|
|||
| <% if @grade_entry_form.id != nil %> | |||
There was a problem hiding this comment.
use .nil? and an unless
| @@ -89,3 +89,12 @@ | |||
| data: { disable_with: t('working') } %> | |||
| </p> | |||
| <% end %> | |||
|
|
|||
| <% if @grade_entry_form.id != nil %> | |||
| <% deletable = @grade_entry_form.count_non_nil == 0 %> | |||
There was a problem hiding this comment.
While I like that you tried to use an existing method, that particular method does a lot more work than necessary. I encourage you to use an association and a query directly; note that @grade_entry_form already has a grades association, and you can filter out the non-nil grades, and use exists? to check whether there are any results. You can do this both for this disabling check and for the actual before_destroy callback itself (see my above comment).
| <%= button_to t(:delete), | ||
| course_grade_entry_form_path(@current_course.id, @grade_entry_form.id), | ||
| method: :delete, | ||
| disabled: !deletable |
There was a problem hiding this comment.
This is good, but please also add a tooltip with a message explains why the button is disabled (see Mimis' PR for assignment button deletion).
… delete-mark-spreadsheet
There was a problem hiding this comment.
@Bruce-8 good work. I just left one more comment.
Regarding the failing test: I don't believe it's related to your pull request, but you should be able to fix it here by passing ignore_query: true to the have_current_path method. Please do that in this PR.
| @@ -89,3 +89,13 @@ | |||
| data: { disable_with: t('working') } %> | |||
| </p> | |||
| <% end %> | |||
|
|
|||
| <% unless @grade_entry_form.id.nil? %> | |||
There was a problem hiding this comment.
Oh after you modified this I realized exactly what you were checking; use the new_record? method instead to check whether a model has been saved or not.
Pull Request Test Coverage Report for Build 8040446126Details
💛 - Coveralls |
Motivation and Context
Currently, there is no
destroyoption for marks spreadsheets (aka grade entry forms). This pull request adds both a web UI feature and new API routes for the deletion of marks spreadsheets, under the condition that there are no non-nil grades for that marks spreadsheet.Your Changes
Description:
destroyaction method tograde_entry_forms_controllerfor both the regular and API version.routes.rbto include routes for the added action methods.Type of change (select all that apply):
Testing
Questions and Comments (if applicable)
Checklist