Reconsider nested set implementation in some models
|Category:||Data model / ORM|
|Target version:||Release 2.6.0|
|Google Code Legacy ID:||Tested version:|
The following models have parent_id relations and nested set:
- contact_information (inherited from actor)
- repository (inherited from actor)
- rights_holder (inherited from actor)
- donor (inherited from actor)
- user (inherited from actor)
- digital_object (nested set partially removed)
We should investigate in which models the nested set is actually needed and remove it if it's not. Without being a major performance improvement, it could speed up the deletion of those models and simplify the AtoM code.
#3 Updated by José Raddaoui Marín 6 months ago
- Description updated (diff)
My initial suggestion would be to:
Remove parent_id and nested set from:
Use only parent_id in:
Use both in:
#7 Updated by José Raddaoui Marín 5 months ago
- Status changed from New to Code Review
- Assignee set to Steve Breker
- Target version set to Release 2.6.0
Addressed in https://github.com/artefactual/atom/pull/1095 and ready for code review.
Current changes included in that PR in relation to this issue:
- Remove parent_id, lft and rgt columns from: - QubitFunctionObject. - QubitPhysicalObject. - Remove lft and rgt columns from: - QubitActor (and other models extending this one). - QubitAclGroup. - QubitTaxonomy. With this changes, the only models with a full nested set implementation are QubitInformationObject, QubitTerm and QubitMenu as they require a method to order siblings. While QubitActor (and related models), QubitAclGroup and QubitTaxonomy only have a parent_id column, used for the ACL system alongside CTE in the ancestors queries. Additionally, the QubitDigitalObject model has only a parent_id column, in this case to relate the derivatives with their master. - Modify schema. - Build SQL. - Add migration. - Bump version. - Add addGetAncestorsAndSelfForAcl to QubitObjectBuilder: - Based on the self-relationship using parent_id. - Needed for the ACL system to get ancestors in those models that don't have a full nested set implementation. - Also used for models with nested set as it's more efficient and simplifies the code. - Uses a join with a CTE and level order to replicate the ordered results from `ancestors->andSelf()->orderBy('lft')`. - Build models. - Remove calls to non existing methods. - Remove not needed uses and references of lft and rgt columns. - Replace QubitActor by QubitMenu in the build nested set task.
#8 Updated by José Raddaoui Marín 5 months ago
- Status changed from Code Review to QA/Review
- Assignee changed from Steve Breker to Dan Gillean
Merged in qa/2.6.x.
For testing, I'd suggest to do it alongside #10306. Check the upgrade and the install processes and CRUD operations over the entities mentioned in the previous update.
Additionally, take special attention to the ACL system and if it works properly when setting permissions for all entity types and for specific ones.
#10 Updated by José Raddaoui Marín 5 months ago
- Requires documentation changed from Yes to No
No changes needed in the nested set build task docs:
#11 Updated by Dan Gillean 5 months ago
- File burlington-acl.png added
- Status changed from QA/Review to Feedback
- Assignee changed from Dan Gillean to José Raddaoui Marín
For the most part, things seem to be working well!
I can't tell if this is a pre-existing issue or not, because of all the known problems with the ACL in AtoM. However, I have found the following:
- Navigate to a user
- Go to description permissions
- Add custom permissions for one particular institution - deny most permission (see attached screenshot)
- Save, then go to browse > archival descriptions
- Multiple descriptions belonging to other institutions are also missing.
- In the demo data, when I applied this to Burlington (6 holdings), all of Sudbury (17 holdings) and several others disappeared as well.
- When applying custom permissions per institution, only descriptions linked to that institution are affected.
#12 Updated by José Raddaoui Marín 5 months ago
- Assignee changed from José Raddaoui Marín to Dan Gillean
I could reproduce this in 2.5.4 (and 2.5.3) with the "Editor" user. After adding the same permissions you mention over the same institution, only 3 top-level descriptions are shown in the browse page. I think this deserves its own ticket.