[MGNLCDEP-76] Allow for checking dependencies to any workspace Created: 19/Dec/14  Updated: 27/Nov/15  Resolved: 25/Aug/15

Status: Closed
Project: Content Dependencies
Component/s: None
Affects Version/s: 1.4.1
Fix Version/s: 1.6

Type: Improvement Priority: Neutral
Reporter: Frank Sommer Assignee: Jaroslav Simak
Resolution: Fixed Votes: 1
Labels: requires_documentation, support
Remaining Estimate: 0d
Time Spent: 19d 7h
Original Estimate: 0.5d

Attachments: PNG File Screen Shot 2015-09-09 at 11.06.18.png    
Issue Links:
Relates
relates to MGNLCDEP-87 Dependency aware confirmation action ... Closed
dependency
is depended upon by MGNLCDEP-80 Link node paths in dependencies tab 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)
Release notes required:
Yes
Date of First Response:
Sprint: Sprint 2 (Vietnam), Sprint 7 (Kromeriz)
Story Points: 5

 Description   

The contact property is configured in the dependencies module. So this property is included by the dependencies check. But the check to the contacts workspace is not yet implemented. The DefaultDependencies class checks only node identifiers for website and assets workspace. So the dependency is not shown and only a log message is written.

WARN   info.magnolia.module.dependencies.impl.DefaultDependencies 19.12.2014 10:56:50 -- Could not get node by identifier [a97ed3e2-23f4-4950-8aab-ecff805a4884] from [website]


 Comments   
Comment by Frank Sommer [ 29/Apr/15 ]

Furthermore the content dependencies module should not check hard wired only against website and assets workspace. It should be configurable in the module configuration.

Comment by Federico Grilli [ 24/Jun/15 ]

sang.ngo I'm taking a look at your solution and, being this more than a trivial bug fix, I think it would beneficial if you could write a brief concept about it. It doesn't need to be extra-detailed, just an outline of how you intend to solve the issue. Here are some guidelines and several concept pages you can look at for "inspiration" https://wiki.magnolia-cms.com/display/DEV/Concepts. If you don't feel like writing a concept proper it might also do a comment or a more elaborated issue description. Also consider that this will be likely used by our documentation team to update the module's doc. Thanks.

Comment by Federico Grilli [ 24/Jun/15 ]

At any rate, I'm seeing lots of code duplication (e.g. DependencyService and Dependencies and their respective implementations, DependenciesField(Definition/Factory ) and ReferenceField(Definition/Factory) ). I'd try a solution which try to use the existing interfaces and classes. Also, I'd try to keep the workspace/properties configuration under the modules' config node (you can have any object in there) instead of having it nested deep in the dialog tabs config.

Comment by Federico Grilli [ 24/Jun/15 ]

OTOH, on second thoughts, the new interfaces are cleaner and would allow to gradually remove the old Dependencies stuff in a later version. Also I like the fact that ReferenceFieldDefinition keeps all it needs to build a proper reference field whereas now this info is part under config/referenceProperties, part under dialogs/reference@referencesTo. So maybe your solution is the way to go. I'd ask quickly someone in the architect cell to validate your solution anyway.

Comment by Sang Ngo Huu [ 29/Jun/15 ]

Add concept page for this https://wiki.magnolia-cms.com/pages/viewpage.action?pageId=104830084

fgrilli Have you asked architect guy to verify my way?

Comment by Philip Mundt [ 14/Jul/15 ]
  • These changes need version bump to 1.5 Not possible as 1.5 is only compatible to Magnolia 5.4.
    • Instead add new class for info.magnolia.module.dependencies.action.DependencyAwareConfirmationAction and maintain (deprecated) the old class
    • This way we can provide a binary compatible version in 1.4.3
  • info.magnolia.module.dependencies.field.PropertyDefinition javadoc has reference to deprecated class
    • Rename to ReferencePropertyDefinition (otherwise collides with existing class in main)
    • Rename info.magnolia.module.dependencies.field.ConfiguredPropertyDefinition to info.magnolia.module.dependencies.field.ConfiguredReferencePropertyDefinition
  • info.magnolia.module.dependencies.field.ReferenceFieldDefinition javadoc has reference to deprecated class
  • info.magnolia.module.dependencies.Dependencies wrong version in deprecated javadoc, should be 1.4.3
  • Make sure to use:
    @deprecated since <version> use {@link DependencyService} instead

    You want to suggest developers what to use, rather than tell what we do now.

  • I see there is some redundant code in
    • info.magnolia.module.dependencies.action.DependencyAwareConfirmationAction#getConfirmationMessageForItem and
    • info.magnolia.module.dependencies.field.ReferenceField#initContent
    • both are building an Map<String, List<Node>> referenceMap / Map<String, Set<Node>> referenceMapinfo.magnolia.module.dependencies.impl.DefaultDependencyService (move to service)?
  • DefaultDependencyServiceTest should also test another workspace, e.g. contacts
    • Why were #testGetReferencesTo(), #testGetReferencesToFailure() removed?
    • Javadoc has reference to deprecated class
  • Typo at info/magnolia/module/dependencies/impl/DefaultDependencyService.java:61 cannnotcannot
  • info.magnolia.module.dependencies.ContentDependenciesModule:
    • private List<String> referenceProperties is rendererd useless now → deprecate (getter & setter too)
    • Actually whole module class is not required anymore
  • info.magnolia.module.dependencies.impl.DefaultDependencyKeepingPropertiesFinder should also be deprecated
  • Comparators info.magnolia.module.dependencies.impl.NodeComparatorByIdentifier & info.magnolia.module.dependencies.impl.NodeComparatorByPath should go to core (main) (MAGNOLIA-6299)
    • Instead of removing comparator from info.magnolia.module.dependencies.impl.DefaultDependencies make them extend the new classes and deprecate them
    • Comparators don't have to be final
Comment by Sang Ngo Huu [ 23/Jul/15 ]

Branch name is MGNLCDEP-76

  • Why were #testGetReferencesTo(), #testGetReferencesToFailure() removed?
    Content are not supported any more. It should be remove in new class
  • Comparators info.magnolia.module.dependencies.impl.NodeComparatorByIdentifier & info.magnolia.module.dependencies.impl.NodeComparatorByPath should go to core (main) (MAGNOLIA-6299)
    I marked it as FIXME, should be change.

Thanks

Comment by Sang Ngo Huu [ 27/Jul/15 ]
  • Comparators info.magnolia.module.dependencies.impl.NodeComparatorByIdentifier & info.magnolia.module.dependencies.impl.NodeComparatorByPath should go to core (main) (MAGNOLIA-6299)
    Fixed, committed code
Comment by Jan Haderka [ 27/Jul/15 ]

Since MGNLCDEP-80 have been reviewed and integrated before this ticket was closed, you need to update the branch now to not only list those dependencies in other workspaces, but to also provide links in them.

Comment by Sang Ngo Huu [ 03/Aug/15 ]

The fixes on MGNLCDEP-76_int branch.

Comment by Jaroslav Simak [ 19/Aug/15 ]

I think this shouldn't go into a minor version as it is a complete overhaul of the Dependencies functionality/API.
Reopened until decision is made to which version this should go in.

Comment by Philip Mundt [ 25/Aug/15 ]
  • info.magnolia.module.dependencies.ContentDependenciesModule#referenceProperties
    Javadoc: specify replacement or refer to new field
  • info.magnolia.module.dependencies.impl.DefaultDependencies#parseIds
    • rename uuid(s) to identifier(s)
    • instead of identifiers.add("jcr:" + uuid); use
      final ItemKey itemKey = new ItemKey(DamConstants.DEFAULT_JCR_PROVIDER_ID, identifier);
      identifiers.add(itemKey.asString());
      
  • info.magnolia.module.dependencies.impl.DefaultDependencies.ContentComparator and info.magnolia.module.dependencies.impl.DefaultDependencies.NodeComparatorByPath
    • Are the deprecation versions correct?
  • I would still vote for extracting
    • info.magnolia.module.dependencies.field.ConfiguredReferencesDefinition.AppLocationFactoryImpl
    • info.magnolia.module.dependencies.field.ConfiguredReferencesDefinition.DamAppLocationFactoryImpl
    • info.magnolia.module.dependencies.field.ConfiguredReferencesDefinition.PathResolverImpl
    • info.magnolia.module.dependencies.field.ConfiguredReferencesDefinition.AssetPathResolverImpl
    • and actually
    • info.magnolia.module.dependencies.field.ReferencesDefinition.AppLocationFactory
    • info.magnolia.module.dependencies.field.ReferencesDefinition.PathResolver
    • into subpackages?
  • Ctors in info.magnolia.module.dependencies.impl.DefaultDependencies (I know I deprecated them) shouldn't they instantiate member variables? E.g.:
    /**
     * @deprecated since 1.2 Please use {@link #DefaultDependencies(DependencyKeepingNodeDataFinder)} instead.
     */
    @Deprecated
    public DefaultDependencies() {
        this(Components.getComponent(DependencyKeepingPropertiesFinder.class), new Provider<ContentDependenciesModule>() {
            @Override
            public ContentDependenciesModule get() {
                return Components.getComponent(ContentDependenciesModule.class);
            }
        });
    }
    
  • info.magnolia.module.dependencies.field.DependenciesFieldDefinition shouldn't we NOT be initializing appMappings in the ctor anymore? They're deprecated...
  • Specify replacement or "no replacement"
    • info.magnolia.module.dependencies.field.DependenciesField#APP_TYPE
    • info.magnolia.module.dependencies.field.DependenciesField#PAGES_APP
    • info.magnolia.module.dependencies.field.DependenciesField#ASSETS_APP
  • Lower clover coverage threshold

Conceptual remark:

  • Page references open up the tree view of page app (which I think makes sense, because one can easily activate the page if required)
  • Asset references open up the detail view, which makes it hard to activate the asset (as per closing the detail view, there is no way to find the path to the asset)

Edit: add bullet points for better readability

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