Flaky unit test - instructor cannot remove section owner

[vijayamanohararaj]

Issue: The unit test instructor_cannot_remove_section_owner would intermittently fail because the assertion for deletion would return HTTP 204 No Content instead of HTTP 403 Forbidden.

test_instructor_cannot_remove_section_owner#Api::V1::SectionInstructorsControllerTest (1.35s)
        Expected response to be a <403: forbidden>, but was a <204: No Content>
        Response body: .
        Expected: 403
          Actual: 204
        test/controllers/api/v1/section_instructors_controller_test.rb:134

Root cause:

• The HTTP call to Delete the section instructor was being made incorrectly with the instructor ID, which is the user ID rather than the ID for the entry from section instructor table.
• Because of

  code-dot-org/dashboard/app/controllers/api/v1/json_api_controller.rb

  Line 1 in 1f8d782

  class Api::V1::JSONApiController < ApplicationController

  , any JSON API would return a HTTP 403 for both unauthorized access and record not found.

So, although the request was returning HTTP 403, it wasn't exercising the authorization code path and was succeeding because the record wasn't found. In those intermittent cases, my guess is, there was a valid section_instructor record with the same ID as the instructor ID, and was successfully deleted. Don't have logs to confirm this theory though.

Links

JIRA: https://codedotorg.atlassian.net/browse/TEACH-1031

Testing story

• Ran the unit test locally
• Also added some logging in Ability.rb to ensure the expected code path is being tested

Deployment strategy

Regular DTP

Follow-up work

None

Privacy

N/A

Security

N/A

Caching

N/A

PR Checklist:

• Tests provide adequate coverage
• Privacy and Security impacts have been assessed
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[etaderhold]

Nice catch!