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

fix: consider sendToAll settings in routingForms response #18226

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix: consider sendToAll settings in routingForms response #18226

wants to merge 9 commits into from
+125 −14

Conversation

vijayraghav-io
Copy link
Contributor

@vijayraghav-io vijayraghav-io commented Dec 17, 2024 •
edited
Loading

What does this PR do?

Before Fix:
https://www.loom.com/share/d800f724d7a64636bf30a16b4dc4d999?sid=ffacca4c-bb87-4338-b134-c44cee8ec0a4

After Fix:
https://www.loom.com/share/ad8eb69159344c3f99bce9dd8f9cb505?sid=741fccb5-3278-48d1-8fb4-520bf32cb572

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • - N/A - I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

@vercel Vercel
Copy link

vercel bot commented Dec 17, 2024

@vijayraghav-io is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Dec 17, 2024
@graphite-app graphite-app bot requested a review from a team December 17, 2024 08:00
@github-actions github-actions bot added emails area: emails, cancellation email, reschedule email, inbox, spam folder, not getting email routing-forms area: routing forms, routing, forms 🐛 bug Something isn't working labels Dec 17, 2024
@github-actions GitHub Actions
Copy link
Contributor

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "update to consider sendToAll". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@vijayraghav-io vijayraghav-io changed the title update to consider sendToAll fix: consider sendToAll settings in routingForms response Dec 17, 2024
@graphite-app Graphite App
Copy link

graphite-app bot commented Dec 17, 2024 •
edited
Loading

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (12/17/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (12/17/24)

1 label was added to this PR based on Keith Williams's automation.

"Add ready-for-e2e label" took an action on this PR • (12/18/24)

1 label was added to this PR based on Keith Williams's automation.

@vijayraghav-io
Copy link
Contributor Author

Self-reviewed, Tested and verified locally with and without toggling "Add all team members, including future members", working as expected, sending mails to new members added.

Will attach a loom video in some time.
Also will try to add test.

@Praashh
Copy link
Contributor

Praashh commented Dec 17, 2024

Self-reviewed, Tested and verified locally with and without toggling "Add all team members, including future members", working as expected, sending mails to new members added.

Will attach a loom video in some time. Also will try to add test.

cool, please add test cases 🙏

@vijayraghav-io
Copy link
Contributor Author

cool, please add test cases 🙏

@Praashh, added test cases 🙏

Praashh
Praashh previously approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@Praashh Praashh left a comment

Choose a reason for hiding this comment

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

LGTM !!

anikdhabal
anikdhabal requested changes Dec 18, 2024
View reviewed changes
packages/app-store/routing-forms/trpc/response.handler.ts
Comment on lines +103 to +106
const whereClause: Prisma.MembershipWhereInput = { teamId: form.teamId };
if (!settings?.sendToAll) {
whereClause.userId = { in: settings.sendUpdatesTo };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, shouldn't we find and update sendUpdatesTo to include future members? Why are we currently not adding future members to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to avoid having an prisma update request on App_RoutingForms_Form table every time there is a response on the routing Form. (in cases of sendToAll)

If we take UI approach to update sendUpdatesTo when switch is selected in RoutingForm, we may miss on the scenario where - new team member is added after the switch in RoutingForm is selected and the sendUpdatesTo wont have future members until the switch is re-toggled.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 18, 2024 •
edited
Loading

E2E results are ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anikdhabal anikdhabal anikdhabal requested changes

@Praashh Praashh Praashh left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
🐛 bug Something isn't working community Created by Linear-GitHub Sync emails area: emails, cancellation email, reschedule email, inbox, spam folder, not getting email ready-for-e2e routing-forms area: routing forms, routing, forms
Projects
None yet
Milestone
No milestone
3 participants
@vijayraghav-io @Praashh @anikdhabal