Refactor delete methods in the database by pstaabp · Pull Request #112 · openwebwork/webwork3

pstaabp · 2022-08-18T12:58:57Z

This refactors all of the deleteXXX methods in the database layer.

Previously deleteXXX would return the deleted object.
At the db layer, nothing is returned.
At the mojolicious layer, a message saying XXX was deleted successfully.

In addition

all perl tests now delete objects then check that they are successfully deleted.
the client-side store tests also delete the object then checks the database for successful deletion.

Note: The UI never used a deleted object so no UI changes should occur.

WIP: cleanup the tests WIP: deleting from db--update store tests

drdrew42

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)

drdrew42 · 2022-08-29T16:53:32Z

lib/DB/Schema/ResultSet/ProblemPool.pm

 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;


Not part of this PR, but why is this called remove instead of delete?

My thought is that we are removing a problem from a pool, not deleting the problem. Happy to switch to delete though.

drdrew42 · 2022-08-29T17:17:39Z

src/stores/problem_sets.ts

+				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');


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.

Will switch the order of these. About the exception, see my comment below.

drdrew42 · 2022-08-29T17:17:56Z

src/stores/set_problems.ts

+			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');


Check before API

drdrew42 · 2022-08-29T17:18:12Z

src/stores/set_problems.ts

+				.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');


Check before API

drdrew42 · 2022-08-29T17:18:34Z

src/stores/users.ts

 				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');


Check before API

drdrew42 · 2022-08-29T17:19:12Z

src/stores/users.ts

 				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');


Check before API

pstaabp · 2022-08-31T11:40:13Z

There are still exceptions thrown. For example, that you mention above, on ProblemPool, the getProblemPool will throw an exception if it doesn't exist. This is how all of the deleteXXX methods work as well.

…g API call. FIX: refactor deleteXXX functions to check for existence before making API call.

pstaabp mentioned this pull request Aug 21, 2022

Add tests for user_sets and user_set versioning #113

Open

pstaabp force-pushed the refactor-db-delete branch from 9a02f29 to e620138 Compare August 24, 2022 21:04

pstaabp added 6 commits August 26, 2022 13:12

WIP: change deleteXXX in ResultSet classes.

d50e6ab

Change the deleteXXX mojolicious tests.

8ba540e

WIP: deleting from db--update store tests

fd6613a

WIP: cleanup the tests WIP: deleting from db--update store tests

WIP: send message from mojolicious layer and more cleanup.

0c05083

FIX: update permission

b014471

FIX: regression after rebase.

2560ddc

pstaabp force-pushed the refactor-db-delete branch from e620138 to 2560ddc Compare August 26, 2022 17:18

drdrew42 reviewed Aug 29, 2022

View reviewed changes

FIX: refactor deleteXXX functions to check for existence before makin…

7aff436

…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 August 31, 2022 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor delete methods in the database#112

Refactor delete methods in the database#112
pstaabp wants to merge 7 commits intoopenwebwork:mainfrom
pstaabp:refactor-db-delete

pstaabp commented Aug 18, 2022

Uh oh!

drdrew42 left a comment

Uh oh!

drdrew42 Aug 29, 2022

Uh oh!

pstaabp Aug 31, 2022

Uh oh!

drdrew42 Aug 29, 2022

Uh oh!

pstaabp Aug 31, 2022

Uh oh!

drdrew42 Aug 29, 2022

Uh oh!

drdrew42 Aug 29, 2022

Uh oh!

drdrew42 Aug 29, 2022

Uh oh!

drdrew42 Aug 29, 2022

Uh oh!

pstaabp commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

pstaabp commented Aug 18, 2022

Uh oh!

drdrew42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstaabp commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants