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

Project Workflow: All Controllers -> FinancialApprovers #3207

Merged
merged 2 commits into from
May 10, 2024
Merged

Project Workflow: All Controllers -> FinancialApprovers #3207

merged 2 commits into from
May 10, 2024

Conversation

atGit2021
Copy link
Contributor

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

@atGit2021 atGit2021 self-assigned this May 9, 2024
@atGit2021 atGit2021 requested a review from CarsonF as a code owner May 9, 2024 22:33
CarsonF
CarsonF requested changes May 9, 2024
View reviewed changes
src/components/project/project.rules.ts Outdated Show resolved Hide resolved
@atGit2021
Copy link
Contributor Author

atGit2021 commented May 10, 2024 •
edited

@CarsonF The only thing missing per the requirements is the desire to have the partner name added to the email notification template. I pushed up the latest commit since that will require a little bit more work as Partner is not currently passed into the template. I also assume that when Sheri says "Partner Name" added to the template, we are actually talking about the Organization name? Please verify
Also, the subject line of the template is already very "full" so I added the project type to the body of the template; hope that is ok

CarsonF
CarsonF approved these changes May 10, 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.

This looks good to me so far. If you want to merge and deploy this and circle back to more email info that sounds like a good idea too.

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

This looks good to me so far. If you want to merge and deploy this and circle back to more email info that sounds like a good idea too.

Ok, once @bryanjnelson approves, I'll squash/fixup and merge this PR and then start working a new PR for the Partner name info for the email notification

@CarsonF CarsonF force-pushed the 1100-Controller-Notifications branch from c7b3a98 to 5bae9fb Compare May 10, 2024 19:32
@CarsonF
Copy link
Member

CarsonF commented May 10, 2024

Ok, once @bryanjnelson approves, I'll squash/fixup and merge this PR and then start working a new PR for the Partner name info for the email notification

I got it. I'd like there to be two distinct commits here since there are two distinct changes here. I don't like "fix up previous & add new thing" in one commit.

@CarsonF CarsonF changed the title Controller Notifications Project Workflow: All Controllers -> FinancialApprovers May 10, 2024
@CarsonF CarsonF enabled auto-merge May 10, 2024 19:34
@CarsonF CarsonF merged commit 12ccab3 into develop May 10, 2024
21 checks passed
@CarsonF CarsonF deleted the 1100-Controller-Notifications branch May 10, 2024 19:53
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 Awaiting requested review from bryanjnelson

Assignees

@atGit2021 atGit2021

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@atGit2021 @CarsonF