Details

    • Type: Task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.4
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Epic Link:
    • Rice Module:
      KRAD
    • Sprint:
      2.4.0-m2 Sprint 2
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required
    • Code Review Status:
      Not Required
    • Include in Release Notes?:
      Yes

      Description

      • Properties missing from the copy method, all properties should be copied (there is one exception I have found, viewIndex on View should not be copied)
      • Copy properties method does not call super
      • Nested components are copied without calling .copy
      • Some classes missing copy properties method (ex ‘GroupValidationMessages’). Verify all classes that implement Component and LayoutManager have this method implemented
      • Change added setters to protected, move to correct location, and add javadoc (see message I sent earlier)
      • Scope issues, for example the following code:

      if(addLineActions != null) {
      List<Action> addLineActions = Lists.newArrayListWithExpectedSize(getAddLineActions().size());
      for (Action addLineAction : this.addLineActions)

      { addLineActions.add((Action)addLineAction.copy()); }

      }

      collectionGroupCopy.setAddLineActions(addLineActions);

      Notice the assignment is outside the declaration block of the local addLineActions, so it picks up the global addLineActions for the setter which causes no change. I think it is best to use a different variable name for the local variable so there is no confusion, like this:

      if(addLineActions != null) {
      List<Action> addLineActionsCopy = ists.newArrayListWithExpectedSize(getAddLineActions().size());
      for (Action addLineAction : this.addLineActions)

      { addLineActionsCopy.add((Action)addLineAction.copy()); }

      collectionGroupCopy.setAddLineActions(addLineActionsCopy);
      }

      One way I would recommend to review the methods, is right click on the tab for class and select ‘Split Vertically’. The will give you another editor tab for the same class to the right, you can then scroll up to see the fields listed in the class and compare to the statements in copyProperties. A good practice would be to make the setter calls in the same order as the fields are declared, which would make it easier to determine if something is missing.

      That is all we need for 2.3, and I will fill more comfortable with it. Please update first because I have already fixed several issues.

      After 2.3, I would like to do some further cleanup. One issue is our copy method with the generics is not working as planned. One of the goals of using the generics is so you don’t have to cast the type for each setter. For example instead of having to do this:

      groupCopy.setDisclosure((Disclosure) this.disclosure.copy());

      We want to be able to just do this:

      groupCopy.setDisclosure(this.disclosure.copy());

      Other cleanup (post 2.3):

      • Only need one public <T> T copy() { per hierarchy. Since this is in DictionaryBeanBase, not needed in any subclass (Component, LayoutManager, etc).

      • Create Copyable interface in org.kuali.rice.krad.datadictionary. Make DictionaryBeanBase an other objects with the copy method (not subclasses) implement

      • ‘replacement’ property on PropertyReplacer needs to be coped. Could be a component, List of components, Map of components, or Layout manager. Need to check for interface

      • Create helper methods in ComponentUtils for copying a List of Copyables and Map of Copyables so logic does not need to be duplicated everywhere, then change statements in copy properties methods to use the new helper methods

      • For nested objects that we are setting directly (not calling copy), you don’t need the null check before (just can set the value as null). This will shorten our copy methods a bit

      • Copy properties method does not use @see comment consistently, and sometimes method is after validate method

      • Formatting wise I would rather see these lines broken up:

      super.copyProperties(component);
      ContainerBase containerBaseCopy = (ContainerBase) component;
      containerBaseCopy.setDefaultItemPosition(this.defaultItemPosition);

      to

      super.copyProperties(component);
      // blank line
      ContainerBase containerBaseCopy = (ContainerBase) component;
      // blank line
      containerBaseCopy.setDefaultItemPosition(this.defaultItemPosition);

      • Remove use of Google Util lists
      • Changed addded setter methods to use protected method

        Attachments

          Activity

          jkneal Jerry Neal (Inactive) created issue -
          jkneal Jerry Neal (Inactive) made changes -
          Field Original Value New Value
          Epic Link KULRICE-10089 [ 120211 ]
          jkneal Jerry Neal (Inactive) made changes -
          Rank Ranked higher
          jcoltrin Jessica Coltrin (Inactive) made changes -
          Sprint 2.4.0-m2 Sprint 1 [ 54 ]
          matthew.wuertz Matthew Wuertz (Inactive) made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          spatterson Shem Patterson (Inactive) made changes -
          Workflow custom [ 205655 ] Copy of custom for rice [ 208344 ]
          spatterson Shem Patterson (Inactive) made changes -
          Workflow Copy of custom for rice [ 208344 ] custom [ 218092 ]
          spatterson Shem Patterson (Inactive) made changes -
          Workflow custom [ 218092 ] Rice Workflow [ 227840 ]
          jkneal Jerry Neal (Inactive) made changes -
          Sprint 2.4.0-m2 Sprint 1 [ 54 ]
          jcoltrin Jessica Coltrin (Inactive) made changes -
          Sprint 2.4.0-m2 Sprint 2 [ 63 ]
          matthew.wuertz Matthew Wuertz (Inactive) made changes -
          Status In Progress [ 3 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          jkneal Jerry Neal (Inactive) made changes -
          Component/s User Experience (UX) [ 13465 ]
          jcoltrin Jessica Coltrin (Inactive) made changes -
          Fix Version/s 2.4.0-m2 [ 17036 ]
          jcoltrin Jessica Coltrin (Inactive) made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              matthew.wuertz Matthew Wuertz (Inactive)
              Reporter:
              jkneal Jerry Neal (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 2 days, 4 hours
                2d 4h
                Remaining:
                Remaining Estimate - 2 days, 4 hours
                2d 4h
                Logged:
                Time Spent - Not Specified
                Not Specified