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

fix: change order of eventtypes and routingforms #17615

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

Conversation

vijayraghav-io
Copy link
Contributor

@vijayraghav-io vijayraghav-io commented Nov 13, 2024

What does this PR do?

/claim #17614

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.

Copy link

vercel bot commented Nov 13, 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 Nov 13, 2024
@graphite-app graphite-app bot requested a review from a team November 13, 2024 10:46
@github-actions github-actions bot added event-types area: event types, event-types Low priority Created by Linear-GitHub Sync ui area: UI, frontend, button, form, input 💎 Bounty A bounty on Algora.io labels Nov 13, 2024
@dosubot dosubot bot added the routing-forms area: routing forms, routing, forms label Nov 13, 2024
@dosubot dosubot bot added this to the Community Only milestone Nov 13, 2024
Copy link

graphite-app bot commented Nov 13, 2024

Graphite Automations

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

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

"Add community label" took an action on this PR • (11/13/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 • (11/13/24)

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

Praashh
Praashh previously approved these changes Nov 13, 2024
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!!

@vijayraghav-io
Copy link
Contributor Author

@PeerRich , updated for eventTypes, routingForms, workflows.
Bookings already have sort as per the tab.

upcoming: { startTime: "asc" },
recurring: { startTime: "asc" },
past: { startTime: "desc" },
cancelled: { startTime: "desc" },
unconfirmed: { startTime: "asc" },

As of now, I couldn't find anything more to be included.
Please let know if any more entities to be included.

Copy link
Contributor

github-actions bot commented Nov 13, 2024

E2E results are ready!

@vijayraghav-io
Copy link
Contributor Author

vijayraghav-io commented Nov 13, 2024

@Praashh , all tests are passing. you can approve again

@Praashh
Copy link
Contributor

Praashh commented Nov 13, 2024

@Praashh , all tests are passing. you can approve again

let me give it a final touch.

Praashh
Praashh previously approved these changes Nov 13, 2024
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!! thank you so much @vijayraghav-io

Copy link
Contributor

@keithwillcode keithwillcode left a comment

Choose a reason for hiding this comment

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

@vijayraghav-io @Praashh Just double-checking we are sure there aren't pieces of code that depend on the sort order for some reason?

(Marking requested changes just to block merging first)

@vijayraghav-io
Copy link
Contributor Author

vijayraghav-io commented Nov 13, 2024

@vijayraghav-io @Praashh Just double-checking we are sure there aren't pieces of code that depend on the sort order for some reason?

Hi @keithwillcode , double checked again that - the sole purpose of these updated (sort order) functions is to ultimately render the lists <li in UI through trpc handlers/endpoints. Verified that these functions have no references or are not called anywhere else.
Also there is no dependency on sort order in the UI for these entities.

@keithwillcode
Copy link
Contributor

@vijayraghav-io @Praashh Just double-checking we are sure there aren't pieces of code that depend on the sort order for some reason?

Hi @keithwillcode , double checked again that - the sole purpose of these updated (sort order) functions is to ultimately render the lists <li in UI through trpc handlers/endpoints. Verified that these functions have no references or are not called anywhere else. Also there is no dependency on sort order in the UI for these entities.

Great. Thanks, @vijayraghav-io

keithwillcode
keithwillcode previously approved these changes Nov 13, 2024
@keithwillcode keithwillcode dismissed their stale review November 13, 2024 17:38

Vijay addressed concern

@vijayraghav-io
Copy link
Contributor Author

Thank you! @keithwillcode 🙏

@anikdhabal anikdhabal marked this pull request as draft November 14, 2024 16:29
@anikdhabal anikdhabal self-assigned this Nov 14, 2024
@vijayraghav-io
Copy link
Contributor Author

updated due to changes after #17689 was merged

@vijayraghav-io
Copy link
Contributor Author

Added a new test - to ensure new eventType added will be first in the list

@vijayraghav-io vijayraghav-io marked this pull request as ready for review November 21, 2024 06:22
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.

We also need a test case for the routing form.

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.

Great Work @vijayraghav-io sir, LGTM!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 Bounty claim 💎 Bounty A bounty on Algora.io community Created by Linear-GitHub Sync event-types area: event types, event-types Low priority Created by Linear-GitHub Sync ready-for-e2e routing-forms area: routing forms, routing, forms ui area: UI, frontend, button, form, input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-4568] add new routers and event-types to the top, not the bottom
5 participants