[MGNLDATA-68] Allow creation of types and data nodes in a hierarchy. Created: 18/Sep/09  Updated: 15/Sep/15  Resolved: 23/Nov/09

Status: Closed
Project: Magnolia Data Module (closed)
Component/s: None
Affects Version/s: 1.4
Fix Version/s: 1.4

Type: Improvement Priority: Major
Reporter: Bert Leunis Assignee: Jan Haderka
Resolution: Fixed Votes: 0
Labels: None
Σ Remaining Estimate: Not Specified Remaining Estimate: Not Specified
Σ Time Spent: Not Specified Time Spent: Not Specified
Σ Original Estimate: Not Specified Original Estimate: Not Specified

Attachments: Text File magnolia-module-data.patch    
Issue Links:
relation
is related to MGNLDATA-78 Make creation of items in the folders... Closed
is related to MGNLDATA-21 Allow for display of content of mixed... Closed
is related to MGNLDATA-22 Add support for building complex cont... Closed
is related to MGNLDATA-75 Add "Activate incl. subnodes" menu it... Closed
Sub-Tasks:
Key
Summary
Type
Status
Assignee
MGNLDATA-72 Make activation of sub type based ite... Sub-task Closed Jan Haderka  
MGNLDATA-76 When activating type, activate associ... Sub-task Closed Jan Haderka  
Template:
Patch included:
Yes
Acceptance criteria:
Empty
Date of First Response:

 Description   

See this entry in the wiki for the description of the desired functionality: http://wiki.magnolia-cms.com/display/DEV/Magnolia+Data+Module+Extensions+-+Mixed+type+hierarchies.

There are many changes to accommodate the new functionality. Here are the main changes:

  • Many classes that create, delete, display the types or the nodes are touched to accommodate the new structure.
  • the DataModule now stores the typedefinitions
  • DataModule creates only one menu entry per rootpath
  • the title of the menu entry can be given in the type definition (sometimes the root type does not explain the main content of a type tree)
  • TypeDeleteCommand: I am not sure if this ever worked correctly. Should do now, and also deletes the tree belonging to the type.
  • TypeDialog: rootpath, allow folders, sort by name are only to be edited in the root type
  • new TypeSelectDataDialog: when creating a new type, will check if there are multiple options of types to create
  • TypeAdminTree and GenericDataAdminTree: lot of new code
  • When working with folders: they are only allowed at the root, not within the type trees itself
  • When items are moved or copied within the tree, if the type definition structure would be broken, the move/copy is prevented.


 Comments   
Comment by Jan Haderka [ 29/Sep/09 ]

After upgrade to 1.4-snapshot (with the patch applied) from 1.3.2 I get NPE when trying to display the types tree. Have you seen this? It must be some of the changes to the tree or tree configuration and will need probably also update task to make sure the modified tree still works after the upgrade. Unfortunately it is difficult to follow the changes in the code since there are also many purely formatting changes giving the false positives. You might consider next time to do the first all formatting changes (if necessary) and put them in one patch, and only then start changing the code and have purely code related changes in another patch (too late for this now).

Comment by Jan Haderka [ 29/Sep/09 ]

Every time I try to create a sub type, the message pops up that the saving of the subtype failed, but the type is actually saved and visible upon refresh of the type tree. Is that something you didn't know how to fix or is it suppose to be working fine?
Also I was under the impression we discussed that for the subtype it should not be possible to select the root folder, but the field is still there in the dialog. Am I missing something here?

Comment by Bert Leunis [ 29/Sep/09 ]

The NPE in the types tree can be solved when the file config.modules.data.trees.dataType.xml will be bootstrapped. I did not change the VersionHandler because I didn't quit get the way it works. At a fresh install, the extra tasks (for newer versions than 1.0) are not bootstrapped until a second startup of the webapp. Not knowing how to fix this properly I left this for someone who does. (I guess you Jan).

Comment by Jan Haderka [ 29/Sep/09 ]

OK, no problem. Are there any other places/points where you were not sure what to do?

Comment by Bert Leunis [ 29/Sep/09 ]

The subtype error sounds weird. I had a similar problem with the renaming of nodes. The code in info.magnolia.module.data.trees.GenericDataAdminTree.renameNode(String) does give an alert saying it can't rename, but then the rename went well. I do not understand what the code in the if-block is supposed to do. Without that block, no problem while renaming.

The rootpath must not be editable in the dialog for the subtype. This is done in info.magnolia.module.data.dialogs.TypeDialog.createDialog(Content, Content). Was it not in the patch? Has the configuration the correct class for the dialog?

Comment by Bert Leunis [ 29/Sep/09 ]

Things to do still:

  • Activation of nested types doesn't work yet (probably a change of i.m.m.data.trees.JCRAdminTree.getActivationSyndicator(String))
  • Activation of a type should also activate the tree and the dialog of that type
  • Activate all command does not lead to creation of folders at the receiving end (question of mine is on the devlist).
Comment by Jan Haderka [ 30/Sep/09 ]

The dialog issue was caused by the rejected chunk of the patch that I have missed (and surprisingly enough the class compiled without it), so it is solved now.
I have found 2 more things I think need to be done:

  • the trees should not be created for subtypes (at least I can't see any reason for them. do you?)
  • the "new item" in the toolbar and in menu should be disabled for terminal items (i.e. those which do not have sub types).
Comment by Bert Leunis [ 01/Oct/09 ]

The trees for the subtypes are needed when you create or edit a subtype. After saving, a tree with the name of that subtype will be opened in AdminCentral, so that's why that tree-definition has to exist. Unless you can think of some way to avoid that. Maybe there is a clue in MGNLDATA-70?

About the 'new item' option for a terminal item: I "solved" this in the TypeSelectDialog: when there are no items to add, a message is displayed in the dialog window. The code for the tree is quite complicated already, I didn't want to add much more to it. And it does work now. But... if you feel this is how you want it... I won't stop you.

Comment by Jan Haderka [ 02/Oct/09 ]

Well spotted. It is indeed a case. I've created a sub task and linked it to MGNLDATA-70 to make sure it doesn't get forgotten.

I'm nearly done with the review and update tasks. Just one more question: why do you set the configurationClass also for the "data" tree (i.e. the tree that is linked to the JCR browser) ... it doesn't seem necessary unless I'm missing something obvious?
Thanks.

Comment by Bert Leunis [ 02/Oct/09 ]

No, you are right, the configurationClass is not needed there. You can get rid of that.

Comment by Jan Haderka [ 02/Oct/09 ]

The patch is committed now. Few technical comments:

  • when creating a patch, do it against the current trunk not against the trunk at the time of first checkout (i.e. do the local mere before creating the patch). This way you can be sure that your latest changes will still work after the merge and make also review faster.
  • when changing public or protected methods, keep in mind that there might be someone else using them. Any public or protected code has to be deprecated for one major version before it can be finally removed. When deprecating something, make sure the message contains info about since the code is deprecated and if there is a replacement for it, also link to it. I have restored all the public/protected methods that were removed by the patch.
  • few more inline comments at some more complex blocks of code would be helpful. It is better to have too much of the comments then too little (i'm told i write too little of comments all the time too )
  • the tests would be very much appreciated and would ensure that the behavior doesn't change accidentally when other changes are made to the module in the future.
Comment by Bert Leunis [ 05/Oct/09 ]

Thanks for all the work Jan, and apologies for all the inconveniences.

  • for such a big change of the code, using a userid/password directly to the svn would give a better way to work maybe
  • having a magnolia code style formatter and using the maven checkstyle plugin can prevent some of the problems you experienced
  • having been lectured on writing 'clean code' I limited the number of comments a bit too much it seems. I like to write comments, if only for myself, to know what I ment to do when reviewing my own code later on.
  • sorry about the tests. not very experienced writing tests, and a collegue of mine told me that writing tests for magnolia is quite hard, for example through the use of all the statics. So I was not very much encouraged.
Comment by Michael Mühlebach [ 15/Sep/15 ]

This ticket was closed because former resolved tickets are deemed to be closed now. If this assumption is untrue in this particular case please feel free to reopen the ticket again.

Generated at Mon Feb 12 05:11:02 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.