Skip to content

fix/duplicate-event-types#5888

Merged
zomars merged 24 commits intomainfrom
fix/duplicate-event-types
Dec 10, 2022
Merged

fix/duplicate-event-types#5888
zomars merged 24 commits intomainfrom
fix/duplicate-event-types

Conversation

@alannnc
Copy link
Copy Markdown
Contributor

@alannnc alannnc commented Dec 6, 2022

What does this PR do?

  • Creates new Dialog for duplicating event types
  • Creates TRPC duplicate for eventTypes mutation

Loom

https://www.loom.com/share/30bb65dec37447c2b0113ca8b675d9d4

Fixes #5835

Environment: Staging(main branch)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

  • Click on action button for a single event type and Duplicate it
  • Choose a new name and click on Continue
  • Validate every config was duplicated for the event

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Dec 10, 2022 at 9:55PM (UTC)

Comment thread apps/web/pages/event-types/index.tsx Outdated
type: type.schedulingType,
teamId: group.teamId,
locations: encodeURIComponent(JSON.stringify(type.locations)),
dialog: "duplicate-event-type",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

new dialog with form and specific action for use case


// inject selection data into url for correct router history
const openModal = (group: EventTypeGroup, type: EventType) => {
const openDuplicateModal = (eventType: EventType) => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Some times we are better off creating a single function for a specific use case that using one and if else everything

Comment thread apps/web/public/static/locales/en/common.json
"radio": "Radio",
"kbar_search_placeholder" : "Start typing to search"
"kbar_search_placeholder": "Start typing to search",
"event_type_duplicate_copy_text": "{{slug}}-copy"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So we can to "event-slug-copy"

slug: z.string(),
title: z.string(),
description: z.string(),
length: z.number(),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is all we need for duplicating and event, form inputs and we can get the rest from DB.

if (!eventType) {
throw new TRPCError({ code: "NOT_FOUND" });
}
// Validate user is owner of event type or in the team
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TODO

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this necessary? i am not able to think of a scenario where an user would be able to create an event that is not created by the user itself or it's members. Can you elaborate this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's always a good practice to take security measures both on front end and on server. In this case I only look for an event type based on its id without making sure who the owner of that it is. With this I make sure if someone hijack the frontend request it doesn't leak any other user's sensitive data.

const eventType = await ctx.prisma.eventType.findUnique({
      where: {
        id: originalEventTypeId,
      }, 
      ...

Copy link
Copy Markdown
Contributor

@harshsinghatz harshsinghatz Dec 6, 2022

Choose a reason for hiding this comment

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

Yeah, that makes a lot of sense, I will try to think about this(security edge cases) more carefully in future PRs.
Thanks for explaining @alannnc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No worries, anytime. What you think about this new approach? It's cleaner IMO and if you can tell me if its working for you it will be a lot of help. @harshsinghatz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for late replies, our timezones don't match lol. Your notifications starts popping up when i am half asleep.
Regarding the approach I feel there are more duplications which would have been avoided but i think overall it's more readable.
Have you implemented it completely or is it still in progress?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's complete right now, obviously just missing webhooks/workflows. But we can add that later.

data: customInputsData,
});
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@CarinaWolli Which can be the best way to clone/copy webhooks/workflows?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For workflows, this worked for me:

    const ids = _workflows.map(workflow => workflow.workflowId);
    const connectWorkflowIds = ids.map(id => { return {workflow: { connect: { id: id } } }});
    
        const data: Prisma.EventTypeCreateInput = {
      ...rest,
      title: newEventTitle,
      slug: newSlug,
      locations: locations ?? undefined,
      team: team ? { connect: { id: team.id } } : undefined,
      users: users ? { connect: users.map((user) => ({ id: user.id })) } : undefined,
      recurringEvent: recurringEvent || undefined,
      bookingLimits: bookingLimits ?? undefined,
      metadata: metadata === null ? Prisma.DbNull : metadata,
      workflows: {
        create: connectWorkflowIds,
      }
    };

@alannnc alannnc added the ♻️ autoupdate tells kodiak to keep this branch up-to-date label Dec 8, 2022
Copy link
Copy Markdown
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

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

LGTM

@zomars zomars merged commit c1e23bf into main Dec 10, 2022
@zomars zomars deleted the fix/duplicate-event-types branch December 10, 2022 22:48
@PeerRich PeerRich added the core area: core, team members only label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

♻️ autoupdate tells kodiak to keep this branch up-to-date core area: core, team members only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-524] Duplicating an event only duplicates the title & description

5 participants