Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return 404 if course not found and 403 for permission issues and update tests #5012

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Apr 11, 2022

Related to #4959

Changes proposed in this Pull Request

Previously we were returning a 403 from the course bulk actions API if the user did not have permission to edit the course or if the course was not found. Now we'll send a 404 if the course is not found.

Testing instructions

Here's a sample request exported from vscode's thunder client. You can import this to your client and change the URL, Cookie, Nonce, Course Ids and Student Ids to check easily
thunder-collection_Sensei.json.zip

@Imran92 Imran92 added this to the 4.4.0 - Student Management milestone Apr 11, 2022
@Imran92 Imran92 requested a review from a team April 11, 2022 14:54
@Imran92 Imran92 self-assigned this Apr 11, 2022
Copy link
Member

@merkushin merkushin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
I only want to suggest slightly improve tests with the new error code.

@@ -161,7 +161,7 @@ public function testAddUsersToCourses_CourseNotFoundGiven_ReturnsForbiddenRespon
$response = $this->server->dispatch( $request );

/* Assert. */
$this->assertSame( 403, $response->get_status() );
$this->assertSame( 404, $response->get_status() );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see also check of the new error code (sensei_course_student_batch_action_missing_course) in tests for 404.
Maybe something like that:

	public function testAddUsersToCourses_CourseNotFoundGiven_ReturnsCourseNotFoundResponse() {
		// ...
		$expected = [
			'status_code' => 404,
			'error_code' => 'sensei_course_student_batch_action_missing_course',
		];
		self::assertSame( $expected, $this->exportErrorResponse( $response ) );
	}

	private function exportErrorResponse( $response ): array {
		return [
			'status_code' => $response->get_status(),
			'error_code' => $response->get_data()['code'] ?? null,
		];
	}

@Imran92
Copy link
Contributor Author

Imran92 commented Apr 12, 2022

Thank for the suggestion @merkushin :) I have updated the tests to assert with error code as well. Can you kindly take a look again?

Copy link
Member

@merkushin merkushin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Imran92 Imran92 merged commit e9c681b into feature/student-management Apr 12, 2022
@Imran92 Imran92 deleted the update/return-not-found-status-if-course-not-found branch April 12, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants