-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KFSPTS-31812 fix asset global doc when asset representative is not set or is non Cornell person #1609
Conversation
…t or is non Cornell person
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've identified the changes in base code that caused this issue; see my comment below. I'm fine with having a temporary workaround added in, though you may want to perform a quick check on other AssetLocation
-related areas to see if any other quick adjustments are needed.
As for this PR's current workaround, the changes look good overall, though there are a few small areas that should be cleaned up further.
src/main/java/edu/cornell/kfs/module/cam/document/service/impl/CuAssetGlobalServiceImpl.java
Show resolved
Hide resolved
src/main/java/edu/cornell/kfs/module/cam/document/service/impl/CuAssetGlobalServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/cornell/kfs/module/cam/document/service/impl/CuAssetGlobalServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/cornell/kfs/module/cam/document/service/impl/CuAssetGlobalServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/cornell/kfs/module/cam/document/service/impl/CuAssetGlobalServiceImpl.java
Outdated
Show resolved
Hide resolved
Thanks Chad, that explanation makes sense. I think I have cleaned up the areas mentioned. I found a few locations that use asset representative person object and found in the code it appears to be doing null checks as expected. It is used in asset transfer tags and JSPs, I was able to do minimal testing with those docs locally and I did not get an error. I can ask Karen to be sure to test asset transfers when validating this change. This is ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, unit tests pass and my concerns have been addressed. Merging.
…t or is non Cornell person (#1609)
This fix prevents an NPE when the asset representative is not set. or is not a cornell employee. It is valid for a representative to not be a Cornell employee when the asset is off site.
I looked in base code to see if this was caused by the person change or something else. I could not determine what change caused this bug to present itself. I am a little uncomfortable about that.
We were able to reproduce this bug in KualiCo Monsters' environment, it has been reported