-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
feat: ooo.new integration with DEEL #18423
base: main
Are you sure you want to change the base?
feat: ooo.new integration with DEEL #18423
Conversation
…o shashank/ooo_new_deel_integration
@ShashankGupta10 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (12/30/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (12/30/24)1 label was added to this PR based on Keith Williams's automation. |
Since this is my first PR here at cal, Would love some feedback if I did make some mistakes.
Drafted the PR for these reasons. Edit - will fix the code for type checks and tests that are failing first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type checks are failing, @ShashankGupta10 can you please fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have proper handling using tryCtach
packages/app-store/deel/api/add.ts
Outdated
await throwIfNotHaveAdminAccessToTeam({ teamId: Number(teamId) ?? null, userId: req.session.user.id }); | ||
const installForObject = teamId ? { teamId: Number(teamId) } : { userId: req.session.user.id }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be null check for session.user.id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the check at lines 9-11 is for that purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please attach a loom video as well @ShashankGupta10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure but try if you can get some test (dummy) key id for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried but could not find anything actually. Will try again and get back to you tomorrow
…o shashank/ooo_new_deel_integration
268c2fc
to
497e0b0
Compare
…m/ShashankGupta10/cal.com into shashank/ooo_new_deel_integration
This is what I found.
This is "IF YOUR ORGANIZTION IS CONNECTED TO DEEL" from the cal app-store
Any ideas on how we should proceed? I will make the changes in this PR accordingly References: |
not sure tbh |
@PeerRich sorry for tagging again but help needed! |
i dont know personally. there is a chance one of our team members has to take over this PR and get the IDs from deel |
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?