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

feat: Multiple Private links per Event Type #15896

Merged
merged 55 commits into from
Oct 2, 2024

Conversation

Souptik2001
Copy link
Contributor

@Souptik2001 Souptik2001 commented Jul 24, 2024

What does this PR do?

Implements single-use links feature!

Disclaimer: Contains schema changes and migrations. ⚠️

https://www.loom.com/share/cb93616c775a4704bdc7590c6c1aa58e?sid=06bb210e-fd7b-4772-827a-e1d77e460ff3

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have added a Docs issue here 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.

How should this be tested?

  • Please check the above loom video, which contains the detailed walkthrough. 👆

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>
Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>
Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>
Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>
Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>
Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>
Copy link

vercel bot commented Jul 24, 2024

@Souptik2001 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added event-types area: event types, event-types Medium priority Created by Linear-GitHub Sync ✨ feature New feature or request ❗️ migrations contains migration files labels Jul 24, 2024
@graphite-app graphite-app bot requested a review from a team July 24, 2024 18:38
@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Jul 24, 2024
Comment on lines 561 to 563
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
updatedFields[typedKey] = undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason getting this type error on this line, and at all not able to fix this, therefore added this. If you have some solution please let me know. 🙏
FYI - few lines below if you see for this line the same ts-ignore is added, maybe it was also getting the same error.

Type error: Expression produces a union type that is too complex to represent.

Copy link

graphite-app bot commented Jul 24, 2024

Graphite Automations

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

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

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

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

"Add foundation team as reviewer" took an action on this PR • (10/01/24)

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

@Souptik2001
Copy link
Contributor Author

Also one weird thing which is happening is that, for the newly added texts, like the "single_use_link_title", etc., when we change the tab the strings get's changed to the placeholder key, as you can see in the video.

Not sure if I am missing something related to this. 🤔

Copy link
Contributor

@anikdhabal anikdhabal 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 @Souptik2001

I've noticed in the Loom video that after destroying a single-use link, the page shows a 404 error, which is expected. However, it doesn't look good. Could you manage our own custom 404 page instead?

@anikdhabal
Copy link
Contributor

Also one weird thing which is happening is that, for the newly added texts, like the "single_use_link_title", etc., when we change the tab the strings get's changed to the placeholder key, as you can see in the video.

Not sure if I am missing something related to this. 🤔

Every time you add a new translation key to the locales, you need to restart the server in order to see the text string

Copy link
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

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

Hey, please make this a setting toggle component, just like the other options. When enabled, it should create one link at that time. If users want more links, they can add them using the 'Add New Link' button, similar to how it currently works.

@Souptik2001
Copy link
Contributor Author

@anikdhabal

I've noticed in the Loom video that after destroying a single-use link, the page shows a 404 error, which is expected. However, it doesn't look good. Could you manage our own custom 404 page instead?

Actually it is using the default 404 page only. I have not created my own 404 page. As per my best guess, it is happening due to this PR - #15696
Please correct me if I am wrong. 🙏

Every time you add a new translation key to the locales, you need to restart the server in order to see the text string

I have actually restarted the server. Infact when I was recording the video, I was even not in dev mode. And as I said it is initially appearing fine (correct text is coming), but as soon as I change the tab and come back to the tab it is somehow changing to the translation key. I tried so much, but couldn't find a way to solve it. 😅

Copy link
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

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

After watching the Loom video, I feel some changes are needed. Please address it first

@Souptik2001
Copy link
Contributor Author

Hey, please make this a setting toggle component, just like the other options. When enabled, it should create one link at that time. If users want more links, they can add them using the 'Add New Link' button, similar to how it currently works.

@anikdhabal Before making this major change, just wanted to confirm few things. Please let me know what do you think about this..

I initially thought about making it a toggle, but then thought that this was more like an "adding feature" than a "toggling feature".

If we add the toggle feature, then certain complexities/questions popup like -

  • What will happen when the user deletes the last remaining link? Will the toggle automatically close?
  • Will this "adding" behavior not make it little bit confusing with the private link feature, as the user will not be able to see the "add new link" button in the first place.

Considering these only I thought to simply make it like this.

Just taking suggestions. Please let me know your thoughts on this. 🙌

@anikdhabal
Copy link
Contributor

@Souptik2001 take this as an example:-

60min._.Event.Type._.Cal.com.and.21.more.pages.-.Personal.-.Microsoft.Edge.2024-07-25.01-41-40.mp4

And when user deletes the last remaining link, the "add new link" button visible in the first place. How is now looking
Screenshot 2024-07-25 014538

Currently, the description is not structured; it needs to be more descriptive.

This is my thought on the improvements, but I suggest waiting until someone from the design approves your implementation and design.

Tagging @ciaranha to check this once.

@anikdhabal anikdhabal requested a review from ciaranha July 24, 2024 20:29
@Souptik2001
Copy link
Contributor Author

Thanks @anikdhabal got it!

Ok waiting for @ciaranha's confirmation before proceeding on your suggested changes!

@Souptik2001
Copy link
Contributor Author

Hey @ciaranha Any update on this? 🙌

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.

Thank you for your contribution.

to be merged, the logical changes in the backend need unit/integration tests to be added.

@Souptik2001
Copy link
Contributor Author

Thanks @keithwillcode . Added e2e tests covering all scenarios of single-use links -

  • Single use links can be added.
  • Single use links can be removed.
  • Single use links should be auto-destroyed on use.

Please let me know if anything else needs to be added.

Here's a video of the last test -

https://www.loom.com/share/a24e5fa1ba5f4469b01af12429e914b8?sid=1b898285-d84d-44c4-b867-3d6aca7685f0

cc: @anikdhabal

@Amit91848
Copy link
Contributor

Made some changes @Souptik2001 type checks and unit tests should pass now. Thank you for your contribution.

Tested and LGTM now 🚀. Will wait for others approval as well

Copy link
Contributor

github-actions bot commented Sep 22, 2024

E2E results are ready!

@Alexander-Frost
Copy link

Alexander-Frost commented Sep 25, 2024

Hey guys -- this PR helps fix an issue we are facing as well. Can we get 👀 on it to merge this in?

@CarinaWolli @anikdhabal @keithwillcode

@anikdhabal
Copy link
Contributor

Hey guys -- this PR helps fix an issue we are facing as well. Can we get 👀 on it to merge this in?

Hey @Alexander-Frost , by this week man🙏

@calcom calcom deleted a comment from Souptik2001 Sep 25, 2024
@Alexander-Frost
Copy link

Bumping this!

Copy link
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

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

Hey @Souptik2001 Can you replace single-use links with multiple private links in all places? Apart from this, all things are okay 🙏

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>
@Souptik2001
Copy link
Contributor Author

Hey @anikdhabal, Replaced single-use-lniks with multiple-private-links (type of words) throughout the PR. ✅

@anikdhabal
Copy link
Contributor

@Souptik2001 sorry man, all things are okay now. Could you pls fix the conflicts?

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>
@github-actions github-actions bot added the Stale label Oct 1, 2024
@graphite-app graphite-app bot requested a review from a team October 1, 2024 11:31
@Souptik2001
Copy link
Contributor Author

@anikdhabal No problem! 🙂

Resolved the conflicts. ✅

@anikdhabal anikdhabal merged commit 2a1dc61 into calcom:main Oct 2, 2024
31 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working community Created by Linear-GitHub Sync event types Created by Linear-GitHub Sync event-types area: event types, event-types ✨ feature New feature or request Medium priority Created by Linear-GitHub Sync ❗️ migrations contains migration files organizations area: organizations, orgs ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-4151] Private URLs aren't working for org links [Feature Request] Single-use link
8 participants