ItemSubApp: create new AbstractSubApp class to handle editing of items (MGNLUI-138)

[MGNLUI-187] itemsubapp: actions for saving node from forms Created: 19/Nov/12  Updated: 11/Feb/13  Resolved: 27/Nov/12

Status: Closed
Project: Magnolia UI
Component/s: None
Affects Version/s: 5.0
Fix Version/s: 5.0

Type: Sub-task Priority: Neutral
Reporter: Espen Jervidalo Assignee: Espen Jervidalo
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
relation
is related to MGNLUI-229 ActionDefinitions used for ItemSubApp... Closed
is related to MGNLUI-228 AppBuilder vs ContentAppBuilder Closed
Template:
Date of First Response:

 Description   

see referenced git commits, and also:
https://git.magnolia-cms.com/gitweb/?p=magnolia_ui.git;a=commit;h=9bbc91cbf63da076e776ac17351a72d578230696



 Comments   
Comment by Jan Haderka [ 26/Nov/12 ]
  • FormDialogPresenterImpl
    +        /**
    +         * This is needed to acces properties from the parent. Currently only the i18basename.
    +         *
    +         */
    +        Dialog dialog = new Dialog(dialogDefinition);
    
    • pls use javadoc comment style in javadoc only. Inline comment here is perfectly appropriate.
  • FormPresenterImpl
    +     * @param item
    +     * @param callback
    +     * @return
    
    • if you are not going to provide info about params or return type don't even add this to javadoc. It will be generated empty in javadoc anyway.
  • CreateItemAction
    /**
    49  * CreateItemAction.
    50  */
    51 public class CreateItemAction extends ActionBase<CreateItemActionDefinition> {
    
    • while this kind of javadoc will make checkstyle to shut up, it's not enough for me.
       /**
      70          * The ItemSubApp only gets a location containing nodePath and View Type.
      71          * When creating a new node, we either create it here and pass the new path to the subapp or
      72          * we pass all needed parameters to the location. This is less messy, but not optimal.. at all.
      73          */
      
    • again either this is javadoc and should be placed in javadoc or it is an inline comment and should use inline comment style
       public void execute() throws ActionExecutionException {
      ...
      81         } catch (RepositoryException e) {
      82             log.error("Could not create child node on parent.", e);
      83         }
      
    • This is sooo wrong! Obviously Action could not be executed because of RepoException, so you should rethrow it as AEE and not just log in log files where no one will see it.
  • CreateItemActionDefinition
    • Class level javadoc is not correct.
    • Also action def that uses appId, subAppId and NodeType seems to me like something that will be used around much more often then just to CreateItems so perhaps name should be more generic
  • ItemWorkbenchPresenter
                 public void onCancel() {
    +                // should switch to ViewType.VIEW
                 }
    
    • thx for explanation, but is there a ticket for it created already? And really this is just a "TODO" w/o "TODO" so if you really need to leave it like this you should mark it as todo (And create ticket in jira)
  • config.modules.ui-admincentral.workbenchActionRegistry.xml
    +      <sv:node sv:name="MetaData">
    +        <sv:property sv:name="jcr:primaryType" sv:type="Name">
    +          <sv:value>mgnl:metaData</sv:value>
    +        </sv:property>
    +        <sv:property sv:name="jcr:createdBy" sv:type="String">
    +          <sv:value>admin</sv:value>
    +        </sv:property>
    +      </sv:node>
    +    </sv:node>
    
    • remove, no metadata should be in new bootstrap files
  • SaveContactFormAction
    +                // Can't use this anymore, breaks when renaming node, ContentChangedEvent is still using the old path
    +                //generateUniqueNodeNameForContact(node);
    
    • either this we need to do something about it, in which case it is TODO and there should be jira ticket or it is not a TODO, we plan to do nothing about it and it should be removed completely from the code
      +        } else {
      +            //validation errors are displayed in the UI.
      +        }
      
    • else block just to make inline comment is terrible waste of space and your and compiler's time.
      generateUniqueNodeNameForContact()
      
    • we have already utilities in core to generate unique names. Why not reuse them rather then inventing your own code?
  • CancelFormActionDefinition
    • class level javadoc explaining why we need empty class implementing empty interface?
  • FormActionFactoryImpl
    • class level javadoc
  • SaveFormActionDefinition
  • class level javadoc
  • this def defines only label and name. I doubt it is relevant only to save so it should be perhaps renames to something more generic
  • ItemWorkbenchPresenter
    +        //final FormPresenter formPresenter = formPresenterFactory.createFormPresenterByName(workbenchDefinition.getFormName());
    +        final FormPresenter formPresenter = formPresenterFactory.createFormPresenterByDefinition(workbenchDefinition.getFormDefinition());
    
    • Why are you adding commented lines of code?
  • ui-admincentral.xml
    +    <component>
    +      <type>info.magnolia.ui.vaadin.form.FormView</type>
    +      <implementation>info.magnolia.ui.vaadin.form.FormViewImpl</implementation>
    +    </component>
    
    • why is ui-vaadin component defined in ui-admincentral and not in ui-vaadin?
  • ContactsModule
    • some field labels seems to be capitalized (Office Fax Nr) some not (Office phone) ... I don't really care which style you choose (pbly Antti or Andreas should say), but they need to be all written in same style
    • button labels seem to be completely w/o capitalisation and all lower case
  • SaveContactFormAction, SaveContactFormActionDefinition, FormActionBuilder, FormBuilder, FormConfig
    • class level javadoc, btw 2nd class defines nothing ... why is it needed at all?
  • in case i missed some class, all that have just name of the class w/ dot at the end as a javadoc need to be updated.
Comment by Espen Jervidalo [ 27/Nov/12 ]

> ui-admincentral.xml
> why is ui-vaadin component defined in ui-admincentral and not in ui-vaadin?

I honestly don't know. has been that way. I think at some point it was part of admincentral and got refactored. There is currently no type mappings in any of the vaadin widget modules.

Comment by Espen Jervidalo [ 27/Nov/12 ]

see MGNLUI-228 concerning ContactsModule

Comment by Espen Jervidalo [ 27/Nov/12 ]

see MGNLUI-233 concerning workbench actionregistry

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