-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add option to delete company #1088
Conversation
@zhx828 |
753c608
to
d6369ea
Compare
51d99a9
to
068ab6c
Compare
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.
Some minor requests. Overall looks good!
src/main/java/org/mskcc/cbio/oncokb/web/rest/CompanyResource.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mskcc/cbio/oncokb/web/rest/CompanyResource.java
Outdated
Show resolved
Hide resolved
879c4c7
to
c0d6d1c
Compare
@jfkonecn let me know once it's ready to review again. |
It's ready for review |
next time you could request me to review once ready on the top right corner of this page, I will be notified. Thanks |
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.
Looks good. Thanks!
public void delete(Long id) { | ||
log.debug("Request to delete Company : {}", id); | ||
Optional<CompanyDTO> maybeCompanyDTO = this.findOne(id); | ||
if (!maybeCompanyDTO.isPresent()) { | ||
return; |
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.
Could you check what's the current behavior when id is not available? Do we throw an exception?
c0d6d1c
to
a2f7a1e
Compare
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.
LGTM, thanks!
In oncokb#1088, we introduced simpleConfirmModalType which indicates whether it's to update or delete company. When changing license type with user associated, we need to specify the simpleConfirmModalType to UPDATE_COMPANY.
In oncokb#1088, we introduced simpleConfirmModalType which indicates whether it's to update or delete company. When changing license type with user associated, we need to specify the simpleConfirmModalType to UPDATE_COMPANY.
In oncokb#1088, we introduced simpleConfirmModalType which indicates whether it's to update or delete company. When changing license type with user associated, we need to specify the simpleConfirmModalType to UPDATE_COMPANY.
This fixes oncokb/oncokb#3617