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 ErrorException: Attempt to read property "asset_tag" on null (rollbar #3541) #13547

Merged

Conversation

inietov
Copy link
Collaborator

@inietov inietov commented Aug 30, 2023

Description

When trying to generate a label on a deleted asset the system returns a 500 error. I'll put an early return if no assets are found, so the user returns to the asset view with the warning that says the asset is currently deleted.

Fixes rollbar RB-3541

Type of change

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

How Has This Been Tested?

Test Configuration:

  • PHP version: 8.1
  • MySQL version: 8.0.31
  • Webserver version: PHP Dev server
  • OS version: Debian 12

@@ -39,6 +39,10 @@ public function render(callable $callback = null)
$assets = $this->data->get('assets');
$offset = $this->data->get('offset');
$template = $this->data->get('template');

if ($assets->isEmpty()){
return redirect()->back();
Copy link
Owner

Choose a reason for hiding this comment

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

Should we be doing a ->with('error', 'Blah blah blah no valid assets') here as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thought about that, but if I added that error message the page looks kinda cluttered as there's already an error message showing that the asset doesn't exist.

@what-the-diff
Copy link

what-the-diff bot commented Aug 30, 2023

PR Summary

  • Improved Asset Handling
    The introduced change ensures that the system will function more efficiently by checking whether there's any content in assets. If assets is found to be empty, it will subsequently prompt a redirection, enhancing the overall system stability and user experience. This ensures we nullify the possibility of errors due to missing or empty assets.

@marcusmoore
Copy link
Collaborator

I noticed the "old" label engine just displays an empty page (on develop) instead of redirecting back. Should we consolidate the behavior between the two engines?

@marcusmoore
Copy link
Collaborator

There's a flash of text on the white screen before the redirect which would be nice to avoid. I haven't dug into how the new engine works so I'm not in a place to say how to avoid that right now.

CleanShot 2023-08-30 at 13 10 50

CleanShot 2023-08-30 at 13 14 07@2x

@inietov
Copy link
Collaborator Author

inietov commented Aug 30, 2023

With this changes the old label engine is also redirected if the asset is deleted. I did saw that text, but I thought it had something to do with my local environment, something like not using a 'proper' server. But gonna investigate that

@marcusmoore
Copy link
Collaborator

Ah...Label implements View so I think Laravel has started the process of handing off the view as a response. I think checking up a level in the controller and redirecting there would get rid of the flash of text.

@inietov
Copy link
Collaborator Author

inietov commented Aug 30, 2023

Nice! I was already on the BulkAssets controller, just figuring out how to handle it there...

@snipe
Copy link
Owner

snipe commented Sep 5, 2023

@inietov Is this ready to go yet?

@inietov
Copy link
Collaborator Author

inietov commented Sep 5, 2023

I think that now is ready. Sorry

@snipe
Copy link
Owner

snipe commented Sep 5, 2023

Thanks!

@snipe snipe merged commit a67888f into snipe:develop Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants