Refactor delete methods in the database #112
Conversation
pstaabp
force-pushed
the
refactor-db-delete
branch
from
9a02f29 to
e620138
Compare
WIP: cleanup the tests WIP: deleting from db--update store tests
pstaabp
force-pushed
the
refactor-db-delete
branch
from
e620138 to
2560ddc
Compare
There was a problem hiding this comment.
I'm concerned about lack of Exceptions on delete calls. Roughly speaking, IMO, the approach should be:
- Fetch the object to be deleted
- If the object doesn't exist, throw NotFoundException
- Delete the object
And on the UI side:
- Check that the object exists in the store
- If the object doesn't exist, display error message and halt
- Make API call
- Handle API response (update store or handle exception message)
| sub removePoolProblem ($self, %args) { | ||
| my $prob = $self->getPoolProblem(info => $args{info}, as_result_set => 1); | ||
| DB::Exception::PoolProblemNotInPool->throw(info => $args{info}) unless defined($prob); | ||
|
|
||
| my $prob2 = $prob->delete; | ||
| return $prob2 if $args{as_result_set}; | ||
| return { $prob2->get_inflated_columns }; | ||
| $self->getPoolProblem(info => $args{info}, as_result_set => 1)->delete; | ||
| return; |
There was a problem hiding this comment.
Not part of this PR, but why is this called remove instead of delete?
There was a problem hiding this comment.
My thought is that we are removing a problem from a pool, not deleting the problem. Happy to switch to delete though.
src/stores/problem_sets.ts
Outdated
| const index = this.db_user_sets.findIndex((set) => set.user_set_id === user_set.user_set_id); | ||
| if (index < 0) { | ||
| logger.error('[user store/deleteUserSet]: the user set was not found in the store'); |
There was a problem hiding this comment.
Shouldn't we be checking that the user set is present before making the API call? This is an error that 'should never happen', but it doesn't make sense to check afterwards.
Also, I worry about having removed the Exceptions on the DB side for attempting to delete objects that don't exist... that should be an error still, and the response wouldn't be 200 OK.
There was a problem hiding this comment.
Will switch the order of these. About the exception, see my comment below.
src/stores/set_problems.ts
Outdated
| const index = this.set_problems.findIndex(prob => prob.set_problem_id === problem.set_problem_id); | ||
| if (index < 0) { | ||
| logger.error('[stores/set_problems/deleteSetProblem]: the set problem was not found in the store'); |
src/stores/set_problems.ts
Outdated
| .findIndex(user_problem => user_problem.user_problem_id === user_problem.user_problem_id); | ||
| if (index < 0) { | ||
| logger.error('[stores/set_problems/deleteUserProblem]: the set problem was not found in the store'); |
src/stores/users.ts
Outdated
| const index = this.users.findIndex((u) => u.user_id === user.user_id); | ||
| // splice is used so vue3 reacts to changes. | ||
| this.users.splice(index, 1); | ||
| return new User(response.data as ParseableUser); | ||
| if (index < 0) { | ||
| logger.error('[user store/deleteUser]: the user was not found in the store'); |
src/stores/users.ts
Outdated
| const index = this.db_course_users.findIndex((u) => u.course_user_id === course_user.course_user_id); | ||
|
|
||
| // splice is used so vue3 reacts to changes. | ||
| this.db_course_users.splice(index, 1); | ||
| const deleted_course_user = new DBCourseUser(response.data as ParseableCourseUser); | ||
| const user = this.users.find(u => u.user_id === deleted_course_user.user_id); | ||
| return new CourseUser(Object.assign({}, user?.toObject(), deleted_course_user.toObject())); | ||
| if (index < 0) { | ||
| logger.error('[user store/deleteCourseUser]: the user was not found in the store'); |
|
There are still exceptions thrown. For example, that you mention above, on |
…g API call. FIX: refactor deleteXXX functions to check for existence before making API call.
pstaabp
force-pushed
the
refactor-db-delete
branch
from
566a80f to
7aff436
Compare
This refactors all of the deleteXXX methods in the database layer.
deleteXXXwould return the deleted object.In addition
Note: The UI never used a deleted object so no UI changes should occur.