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

reuse apps #321

Merged
merged 4 commits into from
Jun 3, 2024
Merged

reuse apps #321

merged 4 commits into from
Jun 3, 2024

Conversation

calellowitz
Copy link
Collaborator

This adds some safety features around reusable apps.

  1. Prevents changing an opportunity from inactive to active if either of its apps are already in use.
  2. Removes apps already in use in active opportunities from the list of available apps when setting up an opportunity

Copy link
Contributor

@pxwxnvermx pxwxnvermx left a comment

Choose a reason for hiding this comment

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

We might need to think about the Deliver Units which are a part of the Payment Units in inactive opportunities, even if the user can reuse those apps they won't be able to create new payment units because the deliver units are already used.

The filtering for already used app looks good to me.

@calellowitz
Copy link
Collaborator Author

We might need to think about the Deliver Units which are a part of the Payment Units in inactive opportunities

Great point. I will update the PR for that

Copy link
Member

@sravfeyn sravfeyn 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, may be some basic tests could make it easier for future modifications.

@calellowitz calellowitz merged commit cd236fa into main Jun 3, 2024
2 checks passed
@calellowitz calellowitz deleted the ce/reuse-apps branch June 3, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants