User tests: Successful: Unsuccessful:
If you uninstall a component with categories, the categories will be deleted by the component. But the deletion just removes the entries without proper nested sets handling. So the rgt/lft values are wrong.
This patch fixes this issue.
How to test:
=> the rgt value is now correct
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
in reality we should probably be using JTableCategory to do the deletions since that will handle updating the tree indexes (lft/rgt) too.
In theory, it should do that. In practice I couldn't find where it does.
I checked both the models and the table. Do you know where it does the rebuild (serious question)?
JTableNested::delete() deals with it (JTableCategory
extends this class and doesn't override the method).
I'm not sure, if server survive a delete-call when there are thausands of categories, which will be rebuild after every deletion. Here we make the rebuild only once...
IMO we shouldn't be using rebuild as a general function but rather use the API the "right" way. It would mean a few more queries (we should be able to do a query to get a list of all the parent categories for the extension and delete those one by one which by default deletes its children too), but it would be less prone to error. #7551 demonstrates one case where altering stuff in a nested table then running rebuild can cause issues.
JTableNested::delete() deals with it ( JTableCategory extends this class and doesn't override the method).
Blind me. I looked at JTableNested::delete() but was looking for a call to rebuild(). Didn't expect it doing the SQL directly.
Category | ⇒ | Libraries |
Easy | No | ⇒ | Yes |
Status | Pending | ⇒ | Needs Review |
Easy | Yes | ⇒ | No |
In theory I agree with @mbabker that we should use the API to delete the entries. In praxis this would kill every server. Even when deleting just a few dozen categories, I had very good servers crap out. Our nested sets implementation for editing is absolutely horrible in terms of performance. The fact that we need rebuild() at all is a problem in itself. As long as we don't fix our nested sets implementation, this is the only viable solution. (And even then I fear it is not going to be enough)
From my perspective, this is good to go. Since this is one of these "can't be tested"-changes, I could give this a placebo-"tested-successfully", but otherwise we can simply merge this in.
Michael is of course correct, but I don't see any reason not to merge this PR in the meantime. Improving JTableNested is another (important) issue which should be looked at separately.
Status | Needs Review | ⇒ | Pending |
I have tested this item
Works as described in test scenario. rgt value changed successfully. @icampus
tested it @icampus
followed the description. rgt value changed sucessfully after the patch.
I have tested this item
tested it @icampus
followed the description. rgt value changed sucessfully after the patch.
Status | Pending | ⇒ | Ready to Commit |
Labels |
Added:
?
|
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-08-13 16:58:27 |
Closed_By | ⇒ | rdeutz |
Labels |
Removed:
?
|
In theory it should work, in reality we should probably be using
JTableCategory
to do the deletions since that will handle updating the tree indexes (lft/rgt) too.