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

Fixed Error 500 after sign and accept asset #12955

Merged

Conversation

inietov
Copy link
Collaborator

@inietov inietov commented May 1, 2023

Description

A customer reported that when a user accepts an Asset a 500 is triggered.

I found that this was happening because the asset that is being assigned have associated a model that is soft-deleted. Which is something that doesn't supposed to happen. I discovered that using the API is possible to assign a model that doesn't exist when creating a new asset. So instead of patching the already occurred error I fix the root cause in the API.

I'm also adding a little conditional to the code retrieving the model name, so it doesn't hard crashes to users that already have models soft-deleted assigned to their assets, but that shouldn't be a problem from now on.

Fixes internal freshdesk 35225

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Test Configuration:

  • PHP version: 8.2
  • MySQL version: 8.0.31
  • Webserver version: PHP Dev Server
  • OS version: Debian 11

Checklist:

@what-the-diff
Copy link

what-the-diff bot commented May 1, 2023

PR Summary

  • Added model existence check
    The addition of a check helps in verifying if the model exists.
  • Error handling for non-existent models
    In case the model doesn't exist, an error message is displayed and the function exits early, preventing further unnecessary processing.

@snipe
Copy link
Owner

snipe commented May 3, 2023

Wouldn't it be better to solve this via the validation rules?

We currently have 'model_id' => 'required|integer|exists:models,id',

@inietov
Copy link
Collaborator Author

inietov commented May 3, 2023

Wouldn't it be better to solve this via the validation rules?

We currently have 'model_id' => 'required|integer|exists:models,id',

That rule considers soft-deleted items as existent. laravel/framework#1820 https://laracasts.com/discuss/channels/general-discussion/validator-rule-for-exists-and-not-soft-deleted-row

But I can try to add a custom validator, or tweaking the rule... let me see what I can do then :).

@snipe
Copy link
Owner

snipe commented May 3, 2023

Thanks, @inietov! I'll merge this one for now just to resolve the issue, but I'd definitely love to find out if we can handle this via validator.

@snipe snipe merged commit fc53b56 into snipe:develop May 3, 2023
4 checks passed
@inietov inietov mentioned this pull request May 4, 2023
1 task
snipe added a commit that referenced this pull request May 8, 2023
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