Util to generateId, remove extra userEmail args #999
Conversation
dexamundsen
commented
Sep 12, 2024
dexamundsen
commented
- create ServiceUtils.newArtifactId() to wrap id creation
- remove --info option from github workflows. It removed unnecessary console info
- use lastModifiedBy = createdBy during artifact.build. This ensured that the values are same during object creation.
- Add createdBy to the cohortbuilder itself instead of passing it as additional param
| default: | ||
| throw new IllegalArgumentException("Unknown JobExecution enum type: " + this); | ||
| } | ||
| return switch (this) { |
There was a problem hiding this comment.
enhanced switch from java 17
There was a problem hiding this comment.
In general, it's not a good idea to make this sort of sweeping cleanup in a PR that is doing other, unrelated things. It makes this change harder to review because I don't know if any differences I come across are intended or not, and can cause cherry-picks, rollbacks, etc. to be more difficult. Don't bother pulling it out of this PR but something to keep in mind in the future.
There was a problem hiding this comment.
Sure -- put these changes together since all of them are cleanup / optimizations. the entire PR is low priority and we could revert the entire thing. hence put them together. dao changes were left separate because that was not a cleanup, but refactored (for later use in next pr), same with snyk ones
| .underlay(body.getUnderlayName()), | ||
| SpringAuthentication.getCurrentUser().getEmail()); | ||
| .underlay(body.getUnderlayName()) | ||
| .createdBy(SpringAuthentication.getCurrentUser().getEmail())); |
There was a problem hiding this comment.
Useremail is only used for this field in nested functions. Add it to builder and remove the param
There was a problem hiding this comment.
The user email is not just used for the cohort created by field, it's also used for the activity log.
There was a problem hiding this comment.
Correct, the activity log used cohort.getCreatedBy(), which might also be more descriptive imo
| @@ -327,10 +327,7 @@ public String createNextRevision( | |||
| .setIsMostRecent(true) | |||
| .version(cohort.getMostRecentRevision().getVersion() + 1) | |||
| .createdBy(userEmail) | |||
| .lastModifiedBy(userEmail) | |||
There was a problem hiding this comment.
lastmodifiedby is set in build() during creation if its is null
| @@ -192,18 +192,21 @@ public Builder isDeleted(boolean isDeleted) { | |||
| } | |||
|
|
|||
| public Study build() { | |||
| // true if the id is empty or null | |||
| if (StringUtils.isEmpty(id)) { | |||
This reverts commit af2aa19.
| default: | ||
| throw new IllegalArgumentException("Unknown JobExecution enum type: " + this); | ||
| } | ||
| return switch (this) { |
There was a problem hiding this comment.
In general, it's not a good idea to make this sort of sweeping cleanup in a PR that is doing other, unrelated things. It makes this change harder to review because I don't know if any differences I come across are intended or not, and can cause cherry-picks, rollbacks, etc. to be more difficult. Don't bother pulling it out of this PR but something to keep in mind in the future.
service/src/main/java/bio/terra/tanagra/app/controller/CohortsApiController.java
Show resolved
Hide resolved
| .underlay(body.getUnderlayName()), | ||
| SpringAuthentication.getCurrentUser().getEmail()); | ||
| .underlay(body.getUnderlayName()) | ||
| .createdBy(SpringAuthentication.getCurrentUser().getEmail())); |
There was a problem hiding this comment.
The user email is not just used for the cohort created by field, it's also used for the activity log.
