Use precalculated hasChecklists flag for checklist navigation item#9698
Use precalculated hasChecklists flag for checklist navigation item#9698manuelmeister wants to merge 3 commits intoecamp:develfrom
Conversation
carlobeltrame
left a comment
carlobeltrame
left a comment
There was a problem hiding this comment.
Not only courses may use checklists, and not only checklists determine whether a camp is a course or not. Can we name this a little more neutrally, e.g. hasChecklists?
manuelmeister
changed the title
manuelmeister
changed the title
manuelmeister
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
✅ Feature branch deployment ready!
|
| if (!$this->checklists->contains($checklist)) { | ||
| $this->checklists[] = $checklist; | ||
| $checklist->camp = $this; | ||
| $this->hasChecklists = true; |
There was a problem hiding this comment.
Do we also set the boolean to false when we don't have checklists?
Did you try it with a computed property instead of caching this on the camp?
| #[\Override] | ||
| public function onBefore($data, Operation $operation, array $uriVariables = [], array $context = []): void { | ||
| $camp = $data->camp; | ||
| if (null === $camp || $data->isPrototype) { |
There was a problem hiding this comment.
Why don't you do that if it is a prototype?
| } | ||
|
|
||
| $camp->hasChecklists = 1 < $this->em->getRepository(Checklist::class)->count([ | ||
| 'camp' => $camp, |
There was a problem hiding this comment.
do we have an index on those 2 fields, maybe even a combined index?
| ('84d41dc61c69', 'Knoblauch', 4, NULL, '2023-08-08 13:33:09', '2023-08-08 13:33:09', '6e3d8bf92360', NULL, '943f3caf8ed8', '5d28f99890bc'), | ||
| ('7655793a3287', 'Zucker', 800, 'g', '2023-08-08 13:33:22', '2023-08-08 13:33:22', '6e3d8bf92360', NULL, '943f3caf8ed8', '5d28f99890bc'), | ||
| ('076f418a0608', 'Zauberhüte', NULL, NULL, '2024-04-01 18:08:01', '2024-04-01 18:08:01', 'f4268fba5b4b', NULL, 'c66f2f897116', '5d28f99890bc'); | ||
| INSERT INTO public.material_item (id, article, quantity, unit, createtime, updatetime, materiallistid, periodid, materialnodeid, campid, done) VALUES |
There was a problem hiding this comment.
This seems unrelated to this PR
There was a problem hiding this comment.
Maybe someone forgot to actually do this as described in the Readme in dev-data
There was a problem hiding this comment.
The idea of the setup is that you don't have to create a dump for each schema change. (and then have even more conflicts when we have 2 PR with schema changes)
You only have to create a new dump to either reset the data on dev, or if you want to add new data (or change existing dev data without using a migration, which is very sensible then, as you don't want to change production. But we can do data migrations only on dev too if we want to.
The rest is handled by the migrations.
For the review it is nice if you update the snapshot in a first commit, and then apply your manual changes to data.sql.
(Or generate the snapshot twice)
3 participants
With this we can save us an initial call to checklists.