Ta deletion #7304

anubhajoshi01 · 2024-11-20T22:35:03Z

Proposed Changes

(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)

Added functionality to give instructors the ability to delete TAs from courses on the Users/Graders tab. This involved adding a delete button on the actions menu, along with a destroy function on the controller. NOTE: A Ta cant be deleted if they have a note
...

Screenshots of your changes (if applicable)

The new delete button

After succesful delete

Associated documentation repository pull request (if applicable)

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	X
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)	X
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
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 by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

…onality

… ta-deletion

for more information, see https://pre-commit.ci

…-Anubha into ta-deletion

coveralls · 2024-11-21T17:21:40Z

Pull Request Test Coverage Report for Build 11966359745

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

80 of 80 (100.0%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 91.706%

Totals
Change from base Build 11922195671:	0.01%
Covered Lines:	40904
Relevant Lines:	43933

💛 - Coveralls

david-yz-liu · 2024-11-22T03:01:05Z

app/models/ta.rb

@@ -1,15 +1,22 @@
 # TA user for a given course.
 class Ta < Role
  has_one :grader_permission, dependent: :destroy, foreign_key: :role_id, inverse_of: :ta
+


revert this change

david-yz-liu · 2024-11-22T03:06:27Z

app/controllers/tas_controller.rb

+    @role = record
+    begin
+      @role.destroy!
+      flash_now(:success, I18n.t('flash.tas.destroy.success', user_name: @role.user_name))


put this line in an else block (look up the syntax for else with begin/rescue) as this indicates that the code should only run if no error was triggered

Also, this is a good time to use head to send an appropriate status code in the response. For DELETE, use :ok for a success, :conflict for the delete restriction error, and :bad_request for other errors

david-yz-liu · 2024-11-22T03:07:48Z

compose.yaml

@@ -1,138 +0,0 @@
-services:


Nothing in this file should be changed. This is the kind of thing you should be looking out for in your self-review.

david-yz-liu · 2024-11-22T03:09:21Z

config/locales/models/users/en.yml

@@ -43,5 +43,6 @@ en:
        one: Grader
        other: Graders
      user:
+        deleted: "(deleted)"


I think this is more general. Please move this label to be under common/en.yml, and name it deleted_placeholder, since it isn't just the word "deleted" but the parentheses as well

david-yz-liu · 2024-11-22T03:09:34Z

config/locales/views/users/en.yml

@@ -42,5 +42,6 @@ en:
      help_html: The API key is used for authentication for the MarkUs API. See <a href="https://github.com/MarkUsProject/Wiki/blob/%{markus_version}/RESTful-API.md">MarkUs Wiki</a> for information on using the API.
      reset: Reset API key
      unavailable: Unavailable
+    deleted: "(deleted)"


this is redundant; see above comment

david-yz-liu · 2024-11-22T03:11:20Z

spec/controllers/tas_controller_spec.rb

+        create(:grade_entry_student_ta, ta: ta, grade_entry_student: student.grade_entry_students.first)
+
+        sign_in instructor
+        delete :destroy, params: { course_id: course.id, id: ta.id }


similar to my above comment, use delete_as instead of sign_in + delete

david-yz-liu · 2024-11-22T03:11:42Z

spec/controllers/tas_controller_spec.rb

+      end
+
+      it 'deletes associated grader permisison' do
+        expect(GraderPermission.where(role_id: ta.id).count).to eq(0)


you can use exists? here as well, instead of the .count equaling 0

david-yz-liu · 2024-11-22T03:14:31Z

app/models/annotation.rb

-    if include_creator
-      data[:creator] = creator.display_name
-    end
+    data[:creator] = include_creator && creator ? creator.display_name : I18n.t('users.deleted')


Don't merge the include_create check with the creator check; keep the overall if statement the way it was before, but then inside use just creator ? ... This ensures that the :creator key is not included at all when include_creator is false, which is the existing (and intended) behaviour.

Also use creator.present? instead of just creator to be more explicit.

david-yz-liu · 2024-11-22T03:17:05Z

app/assets/stylesheets/common/_markus.scss

@@ -1458,3 +1458,21 @@ canvas {
 .jcrop-centered {
  display: inline-block;
 }
+
+// For Actions row


I reviewed this and overall while I appreciate the desire to make the options look nice, I don't think this change is consistent with the rest of the MarkUs UI. Please instead follow the style of existing tables: use the tags table as a model (under an assignment's Settings -> Tags page).

david-yz-liu · 2024-11-22T03:18:03Z

app/assets/javascripts/Components/ta_table.jsx

@@ -36,6 +36,13 @@ class TATable extends React.Component {
      });
  }

+  removeTA = ta_id => {
+    $.ajax({


This is correct, but for new methods prefer using fetch instead of $.ajax (we're slowly trying to move away from jQuery entirely).

See https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch, and note you can pass a method configuration option

…-Anubha into ta-deletion

anubhajoshi01 and others added 8 commits November 20, 2024 17:21

add a delete button on users graders page and implement delete functi…

b77523c

…onality

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

f57cf05

… ta-deletion

merge master

efcb9d2

[pre-commit.ci] auto fixes from pre-commit.com hooks

1c89f73

for more information, see https://pre-commit.ci

normalize keys

58e95e4

Merge branch 'ta-deletion' of https://github.com/anubhajoshi01/Markus…

e3017f1

…-Anubha into ta-deletion

remove inverse ta from grade entry students

3e158bb

fix shared annotation test

60f049e

add to changelog

9914133

anubhajoshi01 requested a review from david-yz-liu November 22, 2024 01:44

david-yz-liu reviewed Nov 22, 2024

View reviewed changes

anubhajoshi01 added 4 commits November 21, 2024 23:05

add to changelog

5a5cd84

Merge branch 'ta-deletion' of https://github.com/anubhajoshi01/Markus…

2460921

…-Anubha into ta-deletion

Merge branch 'ta-deletion' of https://github.com/anubhajoshi01/Markus…

229502c

…-Anubha into ta-deletion

Merge branch 'ta-deletion' of https://github.com/anubhajoshi01/Markus…

7739554

…-Anubha into ta-deletion

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ta deletion #7304

Ta deletion #7304

anubhajoshi01 commented Nov 20, 2024 •

edited

Loading

coveralls commented Nov 21, 2024 •

edited

Loading

david-yz-liu Nov 22, 2024

david-yz-liu Nov 22, 2024

david-yz-liu Nov 22, 2024

david-yz-liu Nov 22, 2024

david-yz-liu Nov 22, 2024

david-yz-liu Nov 22, 2024

david-yz-liu Nov 22, 2024

david-yz-liu Nov 22, 2024

david-yz-liu Nov 22, 2024

david-yz-liu Nov 22, 2024

Ta deletion #7304

Are you sure you want to change the base?

Ta deletion #7304

Conversation

anubhajoshi01 commented Nov 20, 2024 • edited Loading

Proposed Changes

Type of Change

Checklist

Questions and Comments

coveralls commented Nov 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11966359745

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

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

anubhajoshi01 commented Nov 20, 2024 •

edited

Loading

coveralls commented Nov 21, 2024 •

edited

Loading