-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fixed #8709 - Bulk Deletion of Categories, Suppliers, Manufa... #17573
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
base: develop
Are you sure you want to change the base?
Fixed #8709 - Bulk Deletion of Categories, Suppliers, Manufa... #17573
Conversation
…ories-suppliers-manufacturers
Oops, forgot to rename maintenances - done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the stuff we went over when we went through this PR - I'm feeling pretty good about it, myself. Please let me and/or @snipe know when you've been able to address those - or ping me in Slack if you have any specific questions.
'licenses as licenses_count', | ||
]); | ||
|
||
if ($manufacturer->assets_count > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda feel like all these checks could get put in something like:
public function throwIfNotDeletable()
{
//grab counts, throw the new exceptions as appropriate
}
And then you can make is_deletable()
just be like this:
public function is_deletable()
{
try {
$this->throwIfNotDeletable();
} catch (BaseExceptionWhateverItsCalled $e) {
return false;
} catch (Exception $e) {
//do something else?
}
}
This reverts commit fdb0651.
@uberbrady / @snipe this should be ready to go now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments inline
Should be good to go now. |
Alright, this looks like more than it is. I've gone ahead and refactored the existing delete controller methods into actions with new exceptions to be thrown when one of the disallowed conditions has been met.
There's few reasons for this.
The actions make this so that the bulk controllers and the standard controllers are all nearly identical, the only differences are the foreach and the error gathering.
As we continue to expand our use of actions (fingers crossed) those Exceptions are re-usable and create a simple pattern for what to do when one of these things happen
If we need to make a change to how we delete one of these things, it's painfully simple to go do that in one place and have it work the same across the board (for instance, there were some inconsistencies while I was doing this, like not always also deleting the picture associated with the deleted object)
On the frontend, I went ahead and just duplicated
asset-bulk-actions.blade.php
to similar ones for the new supported models, I started writing a bunch of conditionals in the original blade with the intention of re-using it and decided it was starting to look too messy.What should probably happen eventually is a single component with good props that can handle all of these, but the amount of refactor that would've required seemed too much for this PR considering it's already fairly large and has taken some time.
I'm sure there will be some questions and I look forward to the conversation!