Spec design follow-up from "Add REST API to update pages settings"
The following discussion from !147227 (merged) should be addressed:
-
@jzeng88 started a discussion: (+3 comments)
@jzeng88: Looking at the failing undercoverage test, I see that there was no hits on
58: forbidden(response.message) hits: 0
. The reason why I didn't include a test for this case was because it felt redundant given that if a user is authorized & authenticated on the API layer, the UpdateService shouldn't be returning a forbidden in a realistic scenario.
@drew: I'm going to set pipeline:skip-undercoverage and set MWPS again.
Ultimately, the same permission check is implemented twice:
def execute unless can_update_page_settings? return ServiceResponse.error(message: _('...'), reason: :forbidden) end ... end private def can_update_page_settings? current_user&.can_read_all_resources? && can?(current_user, :update_pages, project) end
patch ':id/pages' do authenticated_with_can_read_all_resources! authorize! :update_pages, user_project
per our own development guidance. But because it's the exact same permission check, we're not going to get to that branch of the endpoint logic. There's tests in the UpdateService spec for the Finder-layer permission, and tests in the API spec for the API-layer permission. But this concern is of Finder-level logic in the API-level test that we can't get to because of our self-prescribed redundancy.