[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: |
|
||||||||||||
| 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. 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.
|
| Comment by Sang Ngo Huu [ 12/Oct/15 ] |
|
Hi pmundt, 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:
|
| 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 The branch is |
| 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 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:
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: |
| 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. |