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.

Sign up for GitHub

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

Jump to bottom

Add primary partner name to project workflow notification #3210

Merged
merged 1 commit into from
May 21, 2024
Merged

Add primary partner name to project workflow notification #3210

merged 1 commit into from
May 21, 2024

Conversation

atGit2021
Copy link
Contributor

@atGit2021 atGit2021 commented May 13, 2024 •
edited by sethmcknight

@atGit2021 atGit2021 self-assigned this May 13, 2024
@atGit2021 atGit2021 requested a review from CarsonF as a code owner May 13, 2024 23:59
bryanjnelson
bryanjnelson approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@bryanjnelson bryanjnelson left a comment

Choose a reason for hiding this comment

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

LGTM, @atGit2021!

@CarsonF - I'm leaving the Cypher for your review.

src/components/project/project.rules.ts Outdated Show resolved Hide resolved
@atGit2021 atGit2021 mentioned this pull request May 16, 2024
Closed
CarsonF
CarsonF requested changes May 20, 2024
View reviewed changes
Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

I wouldn't do all this refactoring in this same PR with the business logic change.
Additionally, it needs to be rewritten all together to match progress report workflow.
This is the only way EdgeDB will be able to use "previous step".

src/components/project/project-rules/project.rules.ts Outdated Show resolved Hide resolved
src/components/project/project-rules/project.rules.ts Outdated Show resolved Hide resolved
@atGit2021
Copy link
Contributor Author

I wouldn't do all this refactoring in this same PR with the business logic change.
Additionally, it needs to be rewritten all together to match progress report workflow.
This is the only way EdgeDB will be able to use "previous step".

@CarsonF are you referring to creating the new module and splitting out the two repo files in order to port to edgedb? If so, should I revert all the edge related work and only test to make sure it works on the neo4j side?

@CarsonF
Copy link
Member

CarsonF commented May 20, 2024

@CarsonF are you referring to creating the new module and splitting out the two repo files in order to port to edgedb? If so, should I revert all the edge related work and only test to make sure it works on the neo4j side?

Yes and yes

@atGit2021 atGit2021 requested a review from CarsonF May 20, 2024 19:47
@atGit2021
Copy link
Contributor Author

@CarsonF I tried to get a rid of a conflict that was only showing on the remote side, but not sure if I did it correctly and why I don't see these errors on my local.

@CarsonF CarsonF force-pushed the 1100-Add-PartnerName-to-Notification branch from 8bcf9a2 to cbb82b7 Compare May 21, 2024 19:57
@CarsonF CarsonF force-pushed the 1100-Add-PartnerName-to-Notification branch from cbb82b7 to 2574adf Compare May 21, 2024 20:07
CarsonF
CarsonF approved these changes May 21, 2024
View reviewed changes
Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

LGTM!

@atGit2021
Copy link
Contributor Author

@CarsonF tests look good!

@CarsonF CarsonF changed the title Added partner name to the notification email Add primary partner name to project workflow notification May 21, 2024
@CarsonF CarsonF merged commit ae683f4 into develop May 21, 2024
15 checks passed
@CarsonF CarsonF deleted the 1100-Add-PartnerName-to-Notification branch May 21, 2024 23:28
adam-soltech pushed a commit that referenced this pull request May 29, 2024
@atGit2021 @adam-soltech
Add primary partner name to project workflow notification (#3210)
6bb689c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarsonF CarsonF CarsonF approved these changes

@bryanjnelson bryanjnelson bryanjnelson approved these changes

Assignees

@atGit2021 atGit2021

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@atGit2021 @CarsonF @bryanjnelson