CFM-474_Pipeline_Toolkit_problems #343
Conversation
Ioannis
force-pushed
the
CFM-474_Pipeline_Toolkit_problems
branch
from
07dbd35
to
029f1c3
Compare
| * @return string Display field | ||
| */ | ||
| public function generateDisplayField(\PipelineToolkit\Model\Entity\IdentifierMapper $entity): string { | ||
| return $entity->name ?? $entity->description ?? $entity->flange->description; |
There was a problem hiding this comment.
IdentifierMappers have neither names nor descriptions, so why are we testing for them?
Also we should fail gracefully (ie: return null) if $entity->flange is not set.
More generally, this seems to be a fairly common pattern, so maybe the parent code in StringUtilities should detect the situation?
There was a problem hiding this comment.
Fixed the first. As for the second, you are correct there is a pattern for the models that have no name or description. In general, we probably want to inherit from their parent. I am not sure how we should generalize this yet. @arlen suggested in the past that each model should have a name or description field to facilitate the title construction, etc. I do not think that the StringUtilities is the appropriate place for this.
| * @since COmanage Registry v5.2.0 | ||
| * @return string | ||
| */ | ||
| public function getQualifiedName(): string |
There was a problem hiding this comment.
Does this belong in AppController? It feels more like it belongs in StringUtilities or something?
There was a problem hiding this comment.
I think getQualifiedName belongs in AppController because it returns the fully qualified name of the controller handling the current request. It’s also used by getCurrentTable to resolve the correct table. I understand your point and how it relates to your next comment. To that end, I added the Qualified Name calculation to StringUtilities, but kept this method here to avoid repeatedly accessing the plugin and name values. I find this approach cleaner.
| // Check for missing Breadcrumbs | ||
| // For the Pipeline Toolkit | ||
| if($this->getPlugin() == 'PipelineToolkit') { | ||
| $identifierMapperTable = $this->fetchTable(IdentifierMappersTable::class); | ||
| $identifierMapperEntity = null; | ||
|
|
||
| // Edit/View | ||
| if ($this->viewBuilder()->getVar('vv_obj') && $request->getParam('action') !== 'add') { | ||
| $obj = $this->viewBuilder()->getVar('vv_obj'); | ||
| // Load IdentifierMapper if identifier_mapper_id is set | ||
| if (empty($obj->identifier_mapper_id) && $obj->getSource() !== 'PipelineToolkit.IdentifierMappers') { | ||
| throw new \RuntimeException('Identifier Mapper ID is not set'); | ||
| } | ||
| $identifierMapperEntity = empty($obj->identifier_mapper_id) ? $obj : $identifierMapperTable->get($obj->identifier_mapper_id); | ||
| } else if ($this->request->getQuery('identifier_mapper_id')) { | ||
| $identifierMapperEntity = $identifierMapperTable->get($this->request->getQuery('identifier_mapper_id')); | ||
| } else if ($this->request->getQuery('flange_id')) { | ||
| $identifierMapperEntity = $identifierMapperTable->find() | ||
| ->where(['flange_id' => $this->request->getQuery('flange_id')]) | ||
| ->firstOrFail(); | ||
| } | ||
|
|
||
| if(empty($identifierMapperEntity)) { | ||
| throw new \RuntimeException('Identifier Mapper Record not found'); | ||
| } | ||
| $this->set('vv_identifier_mapper', $identifierMapperEntity); | ||
|
|
||
| // Always add pipeline/flange breadcrumbs from the mapper entity | ||
| $pipelineCrumbs = $this->buildPipelineBreadcrumbs($identifierMapperEntity); | ||
|
|
||
| // Conditionally add the two extra crumbs ONLY when the current entity has identifier_mapper_id | ||
| $extraCrumbs = []; | ||
| $vvObj = $this->viewBuilder()->getVar('vv_obj'); | ||
| if ($request->getParam('action') === 'edit' | ||
| && $vvObj instanceof \Cake\Datasource\EntityInterface | ||
| && (int)($vvObj->get('identifier_mapper_id') ?? 0) > 0) { | ||
|
|
||
| $identifierMapperId = (int)$vvObj->get('identifier_mapper_id'); | ||
|
|
||
| // Identifier Mapper edit | ||
| $extraCrumbs['identifier_mappers:' . $identifierMapperId] = [ | ||
| 'label' => __('Identifier Mappers'), | ||
| 'target' => [ | ||
| 'plugin' => 'PipelineToolkit', | ||
| 'controller' => 'IdentifierMappers', | ||
| 'action' => 'edit', | ||
| $identifierMapperId, | ||
| ], | ||
| ]; | ||
|
|
||
| // Current entity index filtered by identifier_mapper_id | ||
| $controller = (string)$request->getParam('controller'); | ||
| $plugin = (string)($request->getParam('plugin') ?? ''); | ||
| $extraCrumbs[strtolower($controller) . ':index:' . $identifierMapperId] = [ | ||
| 'label' => \App\Lib\Util\StringUtilities::localizeController($controller, $plugin ?: null, true), | ||
| 'target' => [ | ||
| 'plugin' => $plugin ?: null, | ||
| 'controller' => $controller, | ||
| 'action' => 'index', | ||
| '?' => ['identifier_mapper_id' => $identifierMapperId], | ||
| ], | ||
| ]; | ||
| } | ||
|
|
||
| $vv_bc_parents = (array)$this->viewBuilder()->getVar('vv_bc_parents'); | ||
| $this->set('vv_bc_parents', [...$pipelineCrumbs, ...$extraCrumbs, ...$vv_bc_parents]); | ||
| } |
There was a problem hiding this comment.
I don't think we want to add a bunch of plugin specific code to a core controller.
There was a problem hiding this comment.
I generalized the code. Still we only have one use case. The one related to the IdentifierMappers. The more we add the more generic it will eventually become.
| if ($flangeId <= 0) { | ||
| return []; | ||
| } | ||
| } else { | ||
| // 1) Get the flange id | ||
| $flangeId = $entity->get('flange')->id | ||
| ?? $entity->get('flange_id') | ||
| ?? null; | ||
|
|
||
| if (!$flangeId) { | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| $coId = method_exists($this, 'getCOID') ? (int)$this->getCOID() : 0; | ||
|
|
||
| try { | ||
| // 2) Get the Flange to obtain pipeline_id | ||
| $Flanges = $this->fetchTable('Flanges'); | ||
| $flange = $Flanges->get($flangeId); | ||
| $pipelineId = (int)($flange->pipeline_id ?? 0); | ||
|
|
||
| if ($pipelineId <= 0) { | ||
| return []; | ||
| } | ||
|
|
||
| $vv_bc_parents = (array)$this->viewBuilder()->getVar('vv_bc_parents'); | ||
| $parents = []; | ||
|
|
||
| // 3) Pipelines index | ||
| $pipelinesIndexKey = 'pipelines:' . $coId; | ||
| if ($coId > 0 && empty($vv_bc_parents[$pipelinesIndexKey])) { | ||
| $parents[$pipelinesIndexKey] = [ | ||
| 'label' => __('Pipelines'), | ||
| 'target' => [ | ||
| 'plugin' => null, | ||
| 'controller' => 'Pipelines', | ||
| 'action' => 'index', | ||
| '?' => ['co_id' => $coId], | ||
| ], | ||
| ]; | ||
| } | ||
|
|
||
| // 4) Current Pipeline | ||
| $Pipelines = $this->fetchTable('Pipelines'); | ||
| $pipeline = $Pipelines->get($pipelineId); | ||
| $pipelineKey = 'pipelines:' . $pipelineId; | ||
|
|
||
| if (empty($vv_bc_parents[$pipelineKey])) { | ||
| $parents[$pipelineKey] = [ | ||
| 'label' => method_exists($Pipelines, 'generateDisplayField') | ||
| ? $Pipelines->generateDisplayField($pipeline) | ||
| : ($pipeline->{$Pipelines->getDisplayField()} ?? ('Pipeline #' . $pipelineId)), | ||
| 'target' => [ | ||
| 'plugin' => null, | ||
| 'controller' => 'Pipelines', | ||
| 'action' => 'edit', | ||
| $pipelineId, | ||
| ], | ||
| ]; | ||
| } | ||
|
|
||
| // 5) Flanges index for this pipeline | ||
| $flangesIndexKey = 'flanges:index'; | ||
| if (empty($vv_bc_parents[$flangesIndexKey])) { | ||
| $parents[$flangesIndexKey] = [ | ||
| 'label' => __('Flanges'), | ||
| 'target' => [ | ||
| 'plugin' => null, | ||
| 'controller' => 'Flanges', | ||
| 'action' => 'index', | ||
| '?' => ['pipeline_id' => $pipelineId], | ||
| ], | ||
| ]; | ||
| } | ||
|
|
||
| // 6) Current Flange | ||
| $flangeKey = 'flanges:' . $flangeId; | ||
| if (empty($vv_bc_parents[$flangeKey])) { | ||
| $parents[$flangeKey] = [ | ||
| 'label' => method_exists($Flanges, 'generateDisplayField') | ||
| ? $Flanges->generateDisplayField($flange) | ||
| : ($flange->{$Flanges->getDisplayField()} ?? ('Flange #' . $flangeId)), | ||
| 'target' => [ | ||
| 'plugin' => null, | ||
| 'controller' => 'Flanges', | ||
| 'action' => 'edit', | ||
| $flangeId, | ||
| ], | ||
| ]; | ||
| } | ||
| } catch (\Throwable $e) { | ||
| // Fail-soft: no pipeline → no crumbs | ||
| return []; | ||
| } | ||
|
|
||
| return $parents; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think we want to hardcode a bunch of plugin specific logic into a core Trait.
There was a problem hiding this comment.
I think all the models used in the trait are in the core code. I only refer to pipelines and flanges which are not plugins.
…lineToolkit plugins
| // We do not want to hard code the table here. The plugin can set the variable and then we can pick it up | ||
| $pluginTable = null; |
There was a problem hiding this comment.
Is the issue we're trying to address that there's an extra level of hierarchy here? ie:
SqlProvisioner (Entry Point Model) > Provisioning Targets (Pluggable Models) > CO
but
IdentifierMapper (Entry Point Model) > Flanges (Pluggable Model) > Pipelines (Configuration) > CO
?
If that's the case we should be testing for the primary link and using that to calculate the parent, rather than hardcode a bunch of references to Identifier Mapper (what about other Pipeline plugins?), Flange (what if we add another plugin type that follows this model, like LDAP Schema Plugins?), etc. Something like
if($primaryLink != `Co`) {
injectPrimaryLinkCrumbs($primaryLink);
}
For now it's probably sufficient to handle two levels of hierarchy (eg: Provisioning Targets) and three levels (eg: Flanges). I can't think of use cases for four, we can revisit if that comes up.
This pull request fixes the flanges breadcrumb issues. Also it addresses a small bug in the filtering block of the Petitions view.