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

feat: ensure consistent handling of "not found" errors #1520

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

Junjiequan
Copy link
Contributor

Description

This PR aims to correct inconsistent and wrong error handling messages.

e.g, The following code returns Unauthorized, even if the true reason is the proposal is not found.

  private async checkPermissionsForProposal(
    request: Request,
    id: string,
    group: Action,
  ) {
    const proposal = await this.proposalsService.findOne({
      proposalId: id,
    });

    const canDoAction = this.permissionChecker(group, proposal, request);

    if (!canDoAction) {
      throw new ForbiddenException("Unauthorized to this proposal");
    }
    return proposal;
  }

Motivation

Wrong error handling increases debug difficulties and misleading

Fixes

  • Bug fixed (#X)

Changes:

  • changes made

Tests included

  • Included for each change/fix?
  • Passing?

Documentation

  • swagger documentation updated (required for API changes)
  • official documentation updated

official documentation info

@Junjiequan Junjiequan changed the title fix: ensure consistent handling of "not found" errors feat: ensure consistent handling of "not found" errors Nov 25, 2024
@Junjiequan Junjiequan force-pushed the fix-inconsistent-error-handling branch from 0b5d662 to 3aa673f Compare November 25, 2024 19:45
@Junjiequan Junjiequan force-pushed the fix-inconsistent-error-handling branch from 3aa673f to da84a99 Compare November 26, 2024 12:44
@Junjiequan Junjiequan merged commit 0355fdc into master Nov 27, 2024
8 checks passed
@Junjiequan Junjiequan deleted the fix-inconsistent-error-handling branch November 27, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants