Skip to content
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

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

jfkonecn
Copy link
Contributor

@jfkonecn jfkonecn commented Feb 28, 2024

This fixes oncokb/oncokb#3617

@jfkonecn
Copy link
Contributor Author

@zhx828
I'm still working this, but I'm making a draft so you can see the configuration changes I made.

@jfkonecn jfkonecn requested a review from zhx828 February 28, 2024 14:09
@jfkonecn jfkonecn force-pushed the add-option-to-delete-company branch 3 times, most recently from 753c608 to d6369ea Compare March 1, 2024 14:38
@jfkonecn jfkonecn marked this pull request as ready for review March 1, 2024 14:41
@jfkonecn jfkonecn force-pushed the add-option-to-delete-company branch 2 times, most recently from 51d99a9 to 068ab6c Compare March 1, 2024 15:48
Copy link
Member

@zhx828 zhx828 left a 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!

@jfkonecn jfkonecn force-pushed the add-option-to-delete-company branch 2 times, most recently from 879c4c7 to c0d6d1c Compare March 4, 2024 20:14
@zhx828
Copy link
Member

zhx828 commented Mar 5, 2024

@jfkonecn let me know once it's ready to review again.

@jfkonecn
Copy link
Contributor Author

jfkonecn commented Mar 5, 2024

@jfkonecn let me know once it's ready to review again.

It's ready for review

@zhx828
Copy link
Member

zhx828 commented Mar 5, 2024

@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

Copy link
Member

@zhx828 zhx828 left a 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;
Copy link
Member

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?

@jfkonecn jfkonecn added the feature Feature tag for release label Mar 7, 2024
@jfkonecn jfkonecn requested a review from zhx828 March 7, 2024 13:03
@jfkonecn jfkonecn force-pushed the add-option-to-delete-company branch from c0d6d1c to a2f7a1e Compare March 7, 2024 13:05
Copy link
Member

@zhx828 zhx828 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@jfkonecn jfkonecn merged commit 308c937 into master Mar 7, 2024
8 checks passed
@jfkonecn jfkonecn deleted the add-option-to-delete-company branch March 7, 2024 16:18
zhx828 added a commit to zhx828/oncokb-public that referenced this pull request May 1, 2024
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.
jfkonecn pushed a commit to jfkonecn/oncokb-public that referenced this pull request May 13, 2024
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.
jfkonecn pushed a commit to jfkonecn/oncokb-public that referenced this pull request Jun 3, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature tag for release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to delete company
2 participants