[MGNLUI-2111] LinkFieldDefinition ignores the targetTreeRootPath property Created: 16/Sep/13  Updated: 02/Oct/14  Resolved: 22/May/14

Status: Closed
Project: Magnolia UI
Component/s: forms
Affects Version/s: 5.1
Fix Version/s: 5.2.5

Type: Bug Priority: Critical
Reporter: Antti Hietala Assignee: Milan Divilek
Resolution: Fixed Votes: 3
Labels: support
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
duplicate
is duplicated by MGNLUI-2869 Unable to reference a browser subapp ... Closed
relation
is related to MGNLUI-3189 With targetTreeRootPath set in a Link... Closed
is related to MGNLUI-2913 Consider deprecation of targetWorkspa... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Release notes required:
Yes
Date of First Response:

 Description   

LinkFieldDefinition is using appName for launching specified app used for selection. However that app is then ignoring all other properties which is especially nasty in case of targetWorkspace and targetTreeRootPath.

As workaround you can create a separate app and configure it the way you want this view to behave, then reference it in appName.



 Comments   
Comment by Daniel Lipp [ 07/May/14 ]
  • non-trivial changes need to be covered by unit-tests
  • Loggers should be "private static final" - not public (e.g. fix in ContentApp)
Comment by Mikaël Geljić [ 13/May/14 ]
  • LinkFieldDefinition has targetWorkspace / targetTreeRootPath properties. If specified, we try to support these by overriding the defaults from the configured target app's choose dialog (see appName) — [Commenter's note: whatever sense it possibly makes to override workspace for the target app!]
  • Given that choose dialogs are set at app level, and are a feature of content apps, let's just not introduce subAppId quite yet; there is no sense of a subapp's choose dialog for now.
  • It is agreed that supporting these properties causes an API change in App, but we limit side-effects by adding a void implementation to BaseApp, same as the existing #openChooseDialog() API. Admittedly, choose dialog APIs have nothing to do on generic apps, but this is to be reconsidered in a future major iteration.
Comment by Frank Sommer [ 13/May/14 ]

Here some thoughts of me about this topic:

  1. targetWorkspace makes no sense for me, this should not overridable
  2. targetTreeRootPath could make sense, if you want to restrict the editor to a sub tree
  3. subAppId could make sense, if you have in your content app various browse sub apps
    • we have currently this requirement / issue (see MGNLUI-2869)
Comment by Mikaël Geljić [ 13/May/14 ]

Hi Frank,
I do concur that conceptually choose dialogs belong/refer to browser subapps. However for the time being, there is (arguably indeed) no notion of subApp in the way choose dialogs work. The ChooseDialogDefinition is a field of ContentAppDescriptor, the openChooseDialog method even stands on the App interface.

We know that we definitely have to reconsider and simplify this, put the pieces where they belong as soon as possible, but that cannot be a maintenance step. Hence the conclusions from my previous comment. We do want to fix MGNLUI-2869 once for good (I'll move fixVersion accordingly if I may).

Side note, I also thought targetWorkspace could simply be deprecated, it definitely sounds obsolete in this place.

Comment by Mikaël Geljić [ 20/May/14 ]

remaining type-mapping to deleted class info.magnolia.ui.contentapp.choosedialog.ContentAppChooseDialogPresenter in ui-contentapp.xml in 5.3

Comment by Christopher Zimmermann [ 20/May/14 ]

Update obsolete type mapping in ui-contentapp module descriptor.

<type-mapping>
<type>info.magnolia.ui.dialog.choosedialog.ChooseDialogPresenter</type>
<implementation>info.magnolia.ui.dialog.choosedialog.ChooseDialogPresenterImpl</implementation>
</type-mapping>

Comment by Aleksandr Pchelintcev [ 21/May/14 ]

Re-opening because:
At info.magnolia.ui.contentapp.ContentApp#118-119:
ensureChooseDialogField(chooseDialogDefinition, targetTreeRootPath);
presenter.start(callback, chooseDialogDefinition, overlayLayer, selectedId);

The first line makes sure there's a field set for choose dialog definition. However, it doesn't set it directly to the definition provided as an arg, but rather clones it and returns the clone. As a result - the call ensureChooseDialogField is not applied and the definition ends up with no field. One should either assign the chooseDialogDefinition with a result of the ensureChooseDialogField call, or investigate if cloning the definition still makes sense and maybe skip it.

Comment by Milan Divilek [ 21/May/14 ]

Wrong workbench is resolved for "Choose dialog".

Clean installation of 5.2.5 EE bundle

  • Configured Assets Workbench root path to demo-features (Configuration-/modules/dam/apps/assets/subApps/browser/workbench/path to /demo-features). This should change also rootPath of "Choose dialog", because workbench of "Choose dialog" extends browser subApp workbench (Configuration-/modules/dam/apps/assets/chooseDialog/field/workbench/extends = ../../../subApps/browser/workbench).
  • confirmed that Assets app only showed demo-features contents.
  • Page editor / about/subsection-articles/article
  • Edit first content (Text Image)
  • Choose Image, "Select new..."
  • Choose dialog rootPath is "/" -> content from whole DAM repository is shown
Comment by Jaroslav Simak [ 21/May/14 ]

IMHO it behaves as described in documentation (look at default value of targetTreeRootPath property). But i'll check tmr with Mika to be 100% sure.

Comment by Milan Divilek [ 22/May/14 ]

Documentation also says that we support targetWorkspace property (default value is website), but workspace is used from "Choose dialog" workbench. If targetTreeRootPath is not defined in link then we should use root path from "Choose dialog" workbench.

Comment by Mikaël Geljić [ 22/May/14 ]

There is indeed an issue, targetTreeRootPath is always "/" it seems, if not defined, so since it is "set", it will override the default from the ChooseDialogDefinition.
We must remove default value of the LinkFieldDefinition#targetTreeRootPath field.

By the way, another one: we should not call #setPath() (or set anything) on the actual singleton definition, but rather move that to the #ensureChooseDialogField() method, so that we set the path on the cloned def. This would definitely cause concurrency problems if we have multiple link fields with different targetTreeRootPath and/or different users opening choose dialogs at the same time.

Generated at Mon Feb 12 08:53:18 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.