Uploaded image for project: 'Kuali Rice Development'
  1. Kuali Rice Development
  2. KULRICE-12849

Able to create a TravelAccount with a fiscal officer that does not exist, even if a defaultExistenceChecks exists for it.

    Details

    • Type: Bug Fix
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Sprint:
      Core 2.5.0-m6 Sprint 1
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required
    • Code Review Status:
      Not Required
    • Include in Release Notes?:
      Yes
    • Story Points:
      3

      Description

      Links to test locally:
      Labs - Maintenance Views: http://localhost:8080/krad-dev/kr-krad/labs?viewId=LabsMenuMaintenanceView
      Existence Demo: http://localhost:8080/krad-dev/kr-krad/kradsampleapp?viewId=KradMaintenanceSample-PageR6C1

      The fiscalOfficer on the travel account is defaulted to a PersonImpl object, so when validateReferenceExists is called the referenceDataObject that is returned is a PersonImpl object that has all the fields set to empty strings or null. true is returned from validateReferenceExists since the object itself is not null. Another issue may be that when getReferenceIfExists is called, DataObjectWrapperBase.fetchRelationship does nothing since org.kuali.rice.kim.api.identity.Person is a related type that is not supported by DataObjectService.

      Code in DictionaryValidationServiceImpl (comments are a bit confusing in this method):

      public boolean validateReferenceExists(Object dataObject, String referenceName) {
           // attempt to retrieve the specified object from the db
           Object referenceDataObject = getLegacyDataAdapter().getReferenceIfExists(dataObject, referenceName);
      
           // if it isn't there, then it doesn't exist, return false
           if (KRADUtils.isNotNull(referenceDataObject)) {
               return true;
           }
      
           // otherwise, it is there, return true
           return false;
      }
      

        Attachments

          Issue Links

            Activity

            Hide
            sedgar Steve Edgar (Inactive) added a comment -

            This is an interesting case. TraveAccount.getFiscalOfficer() also acts as a setter. And, the line ...

            fiscalOfficer = KimApiServiceLocator.getPersonService().updatePersonIfNecessary(foId, fiscalOfficer);

            ... will return a PersomImpl, with most of the fields either null or empty, if the foid does not exist in KIM.

            This means an "early read" on that field, via pre/post "bind", before the Controller is invoked, will cause that field to be non-null.

            This is in contrast to how accountType works. Reads on that field do not set anything, and a "reference check" fails if an "invalid code" is looked up, leaving accountType null. This allows the validation to kick in on the form, and highlight that field.

            I think the best change, which does not affect "other code", is to only allow TraveAccount.getFiscalOfficer() to fill in fiscalOfficer if the PersonImpl returned contains a principalId.

            I have a commit ready for this, but first, a few questions ...

            The validation already in place on the form will highlight the Travel Account Type Code field, if an invalid code is entered. However, no error headings appear, as they do with client side validation. (This is a situation not related to this case.) The same will now be true with the Fiscal Officer User ID field.

            However, if the one clicks on the highlighted field, a "hint" appears over the field. Are error headings desired? If so, I'm assuming that would be another case, as it appears to be a "feature addition" to the way things currently work.

            Should there be an AFT for this case, and if so would QA do that, or would I?

            Should there be a code review for this case, and if so, who should review?

            (Adding Claus as a Watcher.)

            Show
            sedgar Steve Edgar (Inactive) added a comment - This is an interesting case. TraveAccount.getFiscalOfficer() also acts as a setter. And, the line ... fiscalOfficer = KimApiServiceLocator.getPersonService().updatePersonIfNecessary(foId, fiscalOfficer); ... will return a PersomImpl, with most of the fields either null or empty, if the foid does not exist in KIM. This means an "early read" on that field, via pre/post "bind", before the Controller is invoked, will cause that field to be non-null. This is in contrast to how accountType works. Reads on that field do not set anything, and a "reference check" fails if an "invalid code" is looked up, leaving accountType null. This allows the validation to kick in on the form, and highlight that field. I think the best change, which does not affect "other code", is to only allow TraveAccount.getFiscalOfficer() to fill in fiscalOfficer if the PersonImpl returned contains a principalId. I have a commit ready for this, but first, a few questions ... The validation already in place on the form will highlight the Travel Account Type Code field, if an invalid code is entered. However, no error headings appear, as they do with client side validation. (This is a situation not related to this case.) The same will now be true with the Fiscal Officer User ID field. However, if the one clicks on the highlighted field, a "hint" appears over the field. Are error headings desired? If so, I'm assuming that would be another case, as it appears to be a "feature addition" to the way things currently work. Should there be an AFT for this case, and if so would QA do that, or would I? Should there be a code review for this case, and if so, who should review? (Adding Claus as a Watcher.)
            Hide
            sedgar Steve Edgar (Inactive) added a comment -

            Have received no further feedback on this case, and so am resolving.

            Show
            sedgar Steve Edgar (Inactive) added a comment - Have received no further feedback on this case, and so am resolving.

              People

              • Assignee:
                sedgar Steve Edgar (Inactive)
                Reporter:
                shahess Shannon Hess
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: