-
Notifications
You must be signed in to change notification settings - Fork 3
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
WFPREV-127 Add activity endpoints #468
Conversation
…eature/WFPREV-127
…eature/WFPREV-127
…eature/WFPREV-127
…eature/WFPREV-127
private String activityDescription; | ||
|
||
@NotNull | ||
@Column(name = "activity_start_date") |
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.
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.
Thanks, done
server/wfprev-api/src/main/java/ca/bc/gov/nrs/wfprev/data/entities/ActivityEntity.java
Show resolved
Hide resolved
Looks good. Had some question around nullable thing, please feel free to resolve conversation once confirm. |
@yzlucas yeah I need to add Postman and GraalVM config, I'll add them with my commit for the nullable = false attributes |
private String activityName; | ||
|
||
@NotNull | ||
@Column(name = "activity_description", length = 4000) |
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.
please also add this as nullable = false
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.
Thanks, done
private UUID activityFundingSourceGuid; | ||
|
||
@NotNull | ||
@Column(name = "activity_name", length = 4000) |
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.
same here
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.
Thanks, done
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.
Are you missing configurations for GraalVM?
try { | ||
// Verify project fiscal exists and belongs to project | ||
ProjectFiscalEntity projectFiscal = projectFiscalRepository.findById(UUID.fromString(fiscalGuid)) | ||
.orElseThrow(() -> new EntityNotFoundException(FISCAL_NOT_FOUND + fiscalGuid)); |
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.
You could use MessageFormat.format("{0}: {1}", FISCAL_NOT_FOUND, fiscalGuid);
So you don't need to add the Colon and Space to the constant string. Same for other spots. Optional of course, what's here is fine.
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.
Good tip, I've updated now
public void assignAssociatedEntities(ActivityModel resource, ActivityEntity entity) { | ||
if (resource.getActivityStatusCode() != null) { | ||
String forestAreaCode1 = resource.getActivityStatusCode().getActivityStatusCode(); | ||
ActivityStatusCodeEntity activityStatusCode = loadActivityStatusCode(forestAreaCode1); |
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.
You're only calling the load<code>
methods in this one spot. Why have them in their own methods? You can just use the code directly here?
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.
Changed to access the code directly
public ActivityStatusCodeEntity loadActivityStatusCode(String activityStatusCode) { | ||
return activityStatusCodeRepository | ||
.findById(activityStatusCode) | ||
.orElseThrow(() -> new IllegalArgumentException("ActivityStatusCode not found: " + activityStatusCode)); |
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.
You're calling "or else throw" but the thrown error is never handled anywhere up the chain. Are we assuming this is just a global throw that should fail the whole process?
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.
Catch statements added to ActivityController
activityRepository.deleteById(UUID.fromString(activityGuid)); | ||
} | ||
|
||
public void assignAssociatedEntities(ActivityModel resource, ActivityEntity entity) { |
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.
Binding these codes to objects from resource to entity should be handled by hibernate already. Thats what the @ManyToOne(fetch = FetchType.EAGER, optional = true)
bits are doing. Why are we manually applying them here from the same value? What am I missing?
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 assigning them manually is needed as when I break at line 197, the code models do not have each field populated, just their relevant code string
When these codes do not get manually assigned the insert gives the error org.hibernate.TransientPropertyValueException: object references an unsaved transient instance - save the transient instance before flushing : ca.bc.gov.nrs.wfprev.data.entities.ActivityEntity.activityStatusCode -> ca.bc.gov.nrs.wfprev.data.entities.ActivityStatusCodeEntity
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.
Ah. This will be an issue with the relationship fetch/cascade rules. We can look into that later
I see Lucas mentioned above and you replied that it's coming, so ignore this comment! |
04511ab
to
b5db41d
Compare
|
No description provided.