[MGNLCDEP-85] Extend dependency aware confirmation alert for subnodes Created: 24/Jul/15  Updated: 15/Apr/16  Resolved: 14/Oct/15

Status: Closed
Project: Content Dependencies
Component/s: None
Affects Version/s: 1.5
Fix Version/s: 1.6.1

Type: Improvement Priority: Neutral
Reporter: Richard Gange Assignee: Sang Ngo Huu
Resolution: Fixed Votes: 0
Labels: support
Remaining Estimate: 3.5h
Time Spent: 4d 7h 50m
Original Estimate: 3d

Issue Links:
Relates
relates to MGNLCDEP-87 Dependency aware confirmation action ... Closed
causality
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)
Date of First Response:
Sprint: Saigon 14
Story Points: 5

 Description   

At the moment the dependency aware confirmation works only for selected nodes.
That means, the editor doesn't get any information when a subnode is referenced.

For example a folder of assets gets deleted.



 Comments   
Comment by Jaroslav Simak [ 05/Oct/15 ]

Please do not change API in minor versions:

protected String getConfirmationMessageForItem(Node node) throws RepositoryException {
Comment by Philip Mundt [ 12/Oct/15 ]

MVN adds content-dependency config for tours and category workspace but does not add module-descriptor dependency to tours or categories module.

  • It should instead add optional dependencies
  • and use a conditional install tasks!

This causes failure of EE instances when doing standard delete operations!!!

Comment by Sang Ngo Huu [ 12/Oct/15 ]

Hi pmundt,
Thanks for your notification,

I removed tours and category configuration out of update task. Two workspaces just used for demo projects, and if other modules want to have dependecies feature, they should be depend on dependencies module and update configuration by themself.

I also updated UI test info.magnolia.eeintegrationtests.uitest.ContentDependenciesUITest.contentDependenciesAreDetected().

Please help me review it.

Comment by Philip Mundt [ 12/Oct/15 ]

Problem still remains due to added config for the contacts workspace. Same options:

  1. Remove install task
  2. Add optional dependency and wrap install task in conditional task
Comment by Sang Ngo Huu [ 12/Oct/15 ]

I removed too, IMO, I just think contact app is default app.

Comment by Philip Mundt [ 13/Oct/15 ]

Changes cause migration diff as the member variable updateConfirmDeleteAction was changed, but this task was actually triggered when updating to 1.3.3.

The only more or less clean solution I see atm, is re-adding all configuration for contacts workspace but wrapping those tasks in info.magnolia.module.delta.IsModuleInstalledOrRegistered + adding the optional dependency to the module descriptor.

I believe we cannot remove the configuration when the module is actually installed. Someone might actually be using this...

Comment by Sang Ngo Huu [ 13/Oct/15 ]

You are right, we cannot remove configuration after it was installed. So I reverted, and add trigger task for configuring it.

And MGNLCDEP-82 is also fixed by resolving this problem.

The branch is MGNLCDEP-85_FixMigrationTest, please help me check.
Thanks

Comment by Philip Mundt [ 13/Oct/15 ]

So now you re-added the update of the property class=info.magnolia.module.dependencies.action.DependencyAwareConfirmationActionDefinition for the action confirmDeleteContact in the contacts-app. However, there is no reference configuration for that action. This has actually been missing since MGNLCDEP-87 if I'm not mistaken.

We need to conditionally add that config too (this is actually what I meant with all configuration).

Please add an optional dependency to contacts to the module descriptor.

When removing the other configs (for tours and categories) the key-value-pairs in mgnl-i18n/content-dependencies-app-messages_en.properties were not removed. Please remove:

confirmDeletion.category.label=<strong>Category:</strong>
confirmDeletion.tour.label=<strong>Tours:</strong>
[...]
confirmDeleteFolder.category.label=<strong>Category:</strong>
confirmDeleteFolder.tour.label=<strong>Tours:</strong>

I highly doubt the we-configure-everything-for-everything of method info.magnolia.module.dependencies.setup.DependenciesModuleVersionHandler#getUpdateReferencesConfigFor(String) is the correct approach! I would vote for only configuring what we actually install:

  • /modules/pages/apps/pages/subApps/browser/actions/confirmDeletion/references
    • website
    • A page can reference another page, but there are currently no references present from contacts-to-website and assets-to-website
  • /modules/dam-app/apps/assets/subApps/browser/actions/confirmDeleteAsset/references
    • website
    • An asset can be referenced by a page, but currently there are no references from contacts-to-assets nor assets-to-assets
  • /modules/contacts/apps/contacts/subApps/browser/actions/confirmDeleteContact/references
    • website
    • conditional
    • The only components that references a contact atm is the stkTeaserContact from the website. We do not reference a contact from assets nor from contacts-to-contacts

You will also have to add additional key-value-pairs for assets app and contacts-app:

confirmDeleteContact.website.label=<strong>Pages:</strong>
confirmDeleteAsset.website.label=<strong>Pages:</strong>

Please make sure to actually test each deletion action shows the correct notification (also to validate all label show up correctly).

Comment by Sang Ngo Huu [ 14/Oct/15 ]

I finished update code for your review.

I agreed with your idea we-configure-everything-for-everything. Last time, I just want to help user no need to configure more. Totally, I remove code.

I also remove redundant messages which was not removed from previous ticket.

I added more UI test for deleting Asset, Contact and Asset folder as well.

Please help me check.

Thanks

Comment by Sang Ngo Huu [ 14/Oct/15 ]

There is the document page for new feature belong to this ticket:
https://wiki.magnolia-cms.com/display/VN/How+to+configure+dependency+aware+confirmation+alert+for+subnodes

Comment by Philip Mundt [ 14/Oct/15 ]

When contacts is not installed, the MVH will still trigger getUpdateReferencesConfigFor("/modules/contacts/apps/contacts/subApps/browser/actions/confirmDeleteContact/references") (which is okay, because it uses the NodeExistsDelegateTask inside, but I would still prefer the same approach as above (line 61) - it's easier to comprehend).

Comment by Philip Mundt [ 14/Oct/15 ]

When QAing make sure all mirgation deltas are gone.

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