Details

    • Type: Task Task
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Not version specific
    • Component/s: Development
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Similar issues:
      KULRICE-5323Dataset Consolidation
      KULRICE-111Simplify UI control pluggability
      KULRICE-1306Data/DDL consolidation and bootstrap process
      KULRICE-6321Database script consolidation for release
      KULRICE-10878Simplify ObjectPropertyUtils
      KULRICE-11597Generated lookup/inquiry ids contain invalid characters
      KULRICE-13283Grouping classes and ids bad for row grouping in collection
      KULRICE-8424Consolidate database scripts for release
      KULRICE-2177Consolidate all Rice Struts modules into a single struts module
      KULRICE-7871Checkbox and Radio Group ids are not correct
    • Rice Module:
      KRAD
    • KRAD Feature Area:
      UIF Component
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required
    • Code Review Status:
      Not Required
    • Include in Release Notes?:
      Yes

      Description

      Cleanup item moved from KULRICE-9997:

      Extract ID assignment code (org.kuali.rice.krad.uif.service.impl.ViewHelperServiceImpl#preprocessView) to helper method or class, along with Component State, Create helper methods to reduce nesting and duplicate code

      Related discussion:

      Mark,

      Right. So we could change AssignIdsTasks to use the same code as ViewHelperServiceImpl#preprocessView, that is using the path instead of the index?

      I think the concern with testing, even after we get the things stable, is if a developer makes a change that moves the component (say push it down in a list or something of that sort), the scripts would need to be modified. If we had a way of generating the Ids (for example based on name for inputs), that would allow for more stable tests.

      Thanks,
      Jerry

      From: Mark Fyffe mark.w.fyffe@gmail.com
      Sent: Thursday, December 12, 2013 8:22 AM
      To: 'Jerry Neal'
      Subject: RE: ids

      This does make sense. Now that path is available by virtue of getComponentsForLifecycle(), it should be feasible to universally assign ID based on path.

      There are several cases where components are dynamically created and added to the lifecycle, particularly during the apply model phase. Although it doesn’t do so right now, spawnSubLifecycle can assign the path in these cases making path a viable seed for generating the IDs even for new components. This won’t help the generateAndAssignIds() method used by LightTable and BreadcrumbOptions, however, since the purpose of this method appears to be to discard existing and assign new IDs to all components in the subtree – do you know what purpose this serves?

      One reason IDs keep changing is because we are making changes to when they are assigned, and also changes that affect the order in which lifecycle components are created. Once the development effort settles the IDs should stabilize – however, I’d expect that the upcoming focus on DOM and component cleanup will continue to shake up recorded scripts.

      Mark

      From: Jerry Neal jkneal@iu.edu
      Sent: Wednesday, December 11, 2013 2:16 PM
      To: 'Mark Fyffe'
      Subject: RE: ids

      Hi Mark,

      Thanks for the information! I found the problem. It is due to the way I am running the lifecycle now for component refresh. The index on the phase was not the same and therefore I was getting different ids.

      I do think it would be good to consolidate the ID code. In fact, we might want to add the method to Component. We have been discussing IDs recently due to complaints from testers. The issue is, the IDs we are creating could change if a developer modifies the structure. Recorded scripts will grab these ids (if present), and then not work after updates. For things like inputs, it would be better if we didn’t have the Id on there, and selenium would use the name. However, we need the id in many cases. So we have been talking about how we can make these more static across updates. One approach is to use information from the component. For example, data/input fields have the property path which should be unique in most cases. Actions have the method to call, etc. So we might need different Id generation methods depending on the component. Does that make sense?

      Thanks,
      Jerry

      From: Mark Fyffe mark.w.fyffe@gmail.com
      Sent: Tuesday, December 10, 2013 3:43 PM
      To: Jerry Neal
      Subject: Re: ids

      There are three places now that generate IDs - each will produce a different ID but the results should be repeatable given the same set of circumstances. It will probably be best to consolidate these to a single utility - although it is not currently the case, path is available during the lifecycle, and within pre-process phase, so it should be possible to base all IDs on the path.
      ViewHelperServiceImpl.preprocessView() - Assigns IDs to all components involved in the initialize phase prior to caching the view. Uses path, class name, and parent component ids to maintain an evolving hash code as it traverses the tree. The results should be the same each time the same view is loaded and cached from the DD.
      AssignIdsTask - Assigns IDs during the initialize phase based on the class name and list position for all predecessor phases in the lifecycle phase tree. The results should be the same on each run as long as the components are processed in the same order. IDs are only assigned if not already present. This covers components dynamically created and spawned.
      ComponentUtils.generateAndAssignIds() - Used by LightTable and BreadcrumbOptions. IDs use an evolving hash based on class name and previously assigned ID. Current IDs are discarded and replaced with new values. Results should be repeatable as long as the components provided are consistent.
      In all cases, the ID generated is checked against a set maintained in ViewIndex to prevent duplicate IDs from being generated for the same view. If the ID has already been generated, it is considered a hash collision, so will be seeded again until an ID that hasn't been used for the current view is generated.
      Off the top of my head, I can't think of a reason why a component would get a new ID on refresh. I does seem possible, though, since ID assignment and manipulation takes place during performComponentLifecycle(). Do you have an example of where it is happening?
      Best,
      Mark

      On Tue, Dec 10, 2013 at 2:45 PM, Jerry Neal <jkneal@iu.edu> wrote:
      Hi Mark,

      The new Id generation strategy you put in is based on the component path correct? So if we refreshed a component, the id that gets generated should be the same correct? For some reason, I am not seeing this happen currently.

      Thanks,
      Jerry

        Activity

        Hide
        Mark Fyffe (Inactive) added a comment -

        Reopened temporarily pending code review during F2F.

        Renamed LifecycleElementState#getPath() to #getParentPath()‏
        Removed LifecycleElement#getPath()

        Double-checked unit tests and KRAD sampleapp

        Show
        Mark Fyffe (Inactive) added a comment - Reopened temporarily pending code review during F2F. Renamed LifecycleElementState#getPath() to #getParentPath()‏ Removed LifecycleElement#getPath() Double-checked unit tests and KRAD sampleapp
        Hide
        Mark Fyffe (Inactive) added a comment -

        Removed spawnSubLifecycle and completed regression testing in KRAD sampleapp.

        Spawning sub-lifecycle phases for components that are not yet at the required lifecycle phase is now automated as part of the initializeSuccessors implementation in InitializeComponentPhase, ApplyModelComponentPhase, and FinalizeComponentPhase.

        In regression testing, it was noted that ActionField duplicates properties from Action, which resulted in duplicated lifecycle processing for some pages. Also, it was noted that TreeGroup needed to have bean properties added to support the original getComponentsForLifecycle() and getComponentPrototypes() behavior.

        Show
        Mark Fyffe (Inactive) added a comment - Removed spawnSubLifecycle and completed regression testing in KRAD sampleapp. Spawning sub-lifecycle phases for components that are not yet at the required lifecycle phase is now automated as part of the initializeSuccessors implementation in InitializeComponentPhase, ApplyModelComponentPhase, and FinalizeComponentPhase. In regression testing, it was noted that ActionField duplicates properties from Action, which resulted in duplicated lifecycle processing for some pages. Also, it was noted that TreeGroup needed to have bean properties added to support the original getComponentsForLifecycle() and getComponentPrototypes() behavior.
        Hide
        Mark Fyffe (Inactive) added a comment -

        Reopening for further simplification based on items below from Jerry. To address this, I am converting sub-lifecycle spawning to be automatically and eliminating the need to explicitly spawn when components are dynamically added to the graph.

        Mark,

        A follow-up on this. I’m trying to think of why we are spawning a sub-lifecycles here in the first place? There could be fields added from the remote fields holder, but in most cases these are just the items from the collection, and the init phase should have been run on these.

        Thanks,
        Jerry

        From: Jerry Neal jkneal@iu.edu
        Sent: Friday, December 27, 2013 1:24 PM
        To: 'Mark Fyffe'
        Subject: lineFields

        HI Mark,

        I was wondering about the code in CollectionGroupBuilder:

        List<Field> lineFields = processAnyRemoteFieldsHolder(collectionGroup, lineItems);
        collectionGroup.addLineFields(lineSuffix, lineFields);
        ViewLifecycle.spawnSubLifecyle(model, collectionGroup, "lineFields["+lineSuffix+"]", null, null, false);

        And the corresponding property lineFields in CollectionGroup. Is it necessary to add the property to CollectionGroup because the lifecycle requires a path? What might be slick, is when a lifecycle phase is being run on a component, if the previous phase has not been run it will trigger that phase first. So in this case, these fields will eventually get attached to the lifecycle through the layout manager, and the apply model phase would get triggered on each. I just worry about the confusion the additional holder properties might bring.

        Thanks,
        Jerry

        Another small item related to this request, since it pertains to AssignIdsTask:

        One additional small style thing I just noticed in the id creation code:

        // Iteratively take the product of the hash and another large prime
        hash *= 4507; // until a unique ID has been generated.
        // The use of large primes will minimize collisions, reducing the
        // likelihood of race conditions leading to components coming out
        // with different IDs on different server instances and/or test runs.

        We like to avoid end of line comments. How about this instead?

        // Iteratively take the product of the hash and another large prime
        // until a unique ID has been generated.
        // The use of large primes will minimize collisions, reducing the
        // likelihood of race conditions leading to components coming out
        // with different IDs on different server instances and/or test runs.
        hash *= 4507;

        In this case I would almost suggest moving the information about the use of primes to the method doc, and then having the inline comment just be:

        // Iteratively take the product of the hash and another large prime
        // until a unique ID has been generated.
        hash *= 4507;

        I tend to dislike large blocks of inline comments.

        Also, in general for the inline comments just start with a lower case letter and don’t end with a period:

        // Add the element's path to the hash code.

        To

        // add the element's path to the hash code

        I realize this is a little picky, but we are defining our style at this level so we will be consistent.

        Show
        Mark Fyffe (Inactive) added a comment - Reopening for further simplification based on items below from Jerry. To address this, I am converting sub-lifecycle spawning to be automatically and eliminating the need to explicitly spawn when components are dynamically added to the graph. Mark, A follow-up on this. I’m trying to think of why we are spawning a sub-lifecycles here in the first place? There could be fields added from the remote fields holder, but in most cases these are just the items from the collection, and the init phase should have been run on these. Thanks, Jerry From: Jerry Neal jkneal@iu.edu Sent: Friday, December 27, 2013 1:24 PM To: 'Mark Fyffe' Subject: lineFields HI Mark, I was wondering about the code in CollectionGroupBuilder: List<Field> lineFields = processAnyRemoteFieldsHolder(collectionGroup, lineItems); collectionGroup.addLineFields(lineSuffix, lineFields); ViewLifecycle.spawnSubLifecyle(model, collectionGroup, "lineFields ["+lineSuffix+"] ", null, null, false); And the corresponding property lineFields in CollectionGroup. Is it necessary to add the property to CollectionGroup because the lifecycle requires a path? What might be slick, is when a lifecycle phase is being run on a component, if the previous phase has not been run it will trigger that phase first. So in this case, these fields will eventually get attached to the lifecycle through the layout manager, and the apply model phase would get triggered on each. I just worry about the confusion the additional holder properties might bring. Thanks, Jerry Another small item related to this request, since it pertains to AssignIdsTask: One additional small style thing I just noticed in the id creation code: // Iteratively take the product of the hash and another large prime hash *= 4507; // until a unique ID has been generated. // The use of large primes will minimize collisions, reducing the // likelihood of race conditions leading to components coming out // with different IDs on different server instances and/or test runs. We like to avoid end of line comments. How about this instead? // Iteratively take the product of the hash and another large prime // until a unique ID has been generated. // The use of large primes will minimize collisions, reducing the // likelihood of race conditions leading to components coming out // with different IDs on different server instances and/or test runs. hash *= 4507; In this case I would almost suggest moving the information about the use of primes to the method doc, and then having the inline comment just be: // Iteratively take the product of the hash and another large prime // until a unique ID has been generated. hash *= 4507; I tend to dislike large blocks of inline comments. Also, in general for the inline comments just start with a lower case letter and don’t end with a period: // Add the element's path to the hash code. To // add the element's path to the hash code I realize this is a little picky, but we are defining our style at this level so we will be consistent.
        Hide
        Mark Fyffe (Inactive) added a comment -

        This work is complete on the performance branch. Unit tests have been updated and are passing, and initial regression testing didn't turn over any issues.

        • Added path property to LifecycleElement
        • Removed index from ViewLifecyclePhaseBase.
        • Simplified ID generation to a hash calculated from class name and path.
        • Converted sublifecycle to use a path expression for determining component, rather than passing the component itself.
        • Added strict checks to ensure a component's path refers to the component relative to the view.
        • Set path to populate in pre-process phase, as well as prior to executing tasks on all lifecycle phases.

        Note that strict mode is not slower due to path/element verification, so should be disabled in non-development environments (if not already).

        Show
        Mark Fyffe (Inactive) added a comment - This work is complete on the performance branch. Unit tests have been updated and are passing, and initial regression testing didn't turn over any issues. Added path property to LifecycleElement Removed index from ViewLifecyclePhaseBase. Simplified ID generation to a hash calculated from class name and path. Converted sublifecycle to use a path expression for determining component, rather than passing the component itself. Added strict checks to ensure a component's path refers to the component relative to the view. Set path to populate in pre-process phase, as well as prior to executing tasks on all lifecycle phases. Note that strict mode is not slower due to path/element verification, so should be disabled in non-development environments (if not already).

          People

          • Assignee:
            Mark Fyffe (Inactive)
            Reporter:
            Jerry Neal (Inactive)
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 4 hours Original Estimate - 4 hours
              4h
              Remaining:
              Remaining Estimate - 0 minutes
              0m
              Logged:
              Time Spent - 1 day
              1d

                Structure Helper Panel