cal.com
cal.com copied to clipboard
feat: Orgs: Auto-redirect after deleting team members
What does this PR do?
- Fixes #15259
- Fixes CAL-3831
https://github.com/calcom/cal.com/assets/74371312/1c1a3245-8e83-415b-9b3d-949d93a0e05c
Mandatory Tasks (DO NOT REMOVE)
- [x] 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
- [ ] I have added or modified automated tests that prove my fix is effective or that my feature works (PRs might be rejected if logical changes are not properly tested)
@Amit91848 is attempting to deploy a commit to the cal Team on Vercel.
A member of the Team first needs to authorize it.
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.
Graphite Automations
"Add community label" took an action on this PR • (06/02/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 • (06/02/24)
1 reviewer was added to this PR based on Keith Williams's automation.
Do you think we should also show whatever redirects have been created for an org, similar to the table in ooo?
Do you think we should also show whatever redirects have been created for an org, similar to the table in ooo?
Thats not a bad idea! Deffo not needed for this feature but would be a nice to have if you have time
LGTM - tested and it works perfectlly. Thanks for the tests , great job as always @Amit91848
If acme.cal.com/member1 is replaced by acme.cal.com/member2 but /member2/my-event doesn't exist and /member1/my-event exists, going to /member1/my-event would take to 404 /member2/my-event.
I think we should redirect such 404 pages to the member2 profile page. That would be a special handling in addition to TempOrgRedirect.
To keep things simple we could also redirect all event-types of member1 to member2 profile page from where the user can choose which event suits them. It can't be done through TempOrgRedirect currently. We might have to introduce a new RedirectType which could be RedirectType.UserToProfile
Another problem could be that the username could be allocated to some new member later on. So, I think we shouldn't show the user as deleted in this case. They should be shown as 'Replaced by Member2'. Admin can then delete the redirect(e.g. when a new member with same username joins) or replace that with another Member(say Member3). We could tackle this later but for that to happen we really need to have separate entity for Replacement so that we can show that in UI later. A direct entry to TempOrgRedirect is unmanageable.
Thanks for the review @hariombalhara , I have a few thoughts
If
acme.cal.com/member1is replaced byacme.cal.com/member2but/member2/my-eventdoesn't exist and/member1/my-eventexists, going to/member1/my-eventwould take to 404/member2/my-event.
Currently only profile page is being redirected to ig this would result in a 404 for /member1/my-event itself.
I think we should redirect such 404 pages to the member2 profile page. That would be a special handling in addition to TempOrgRedirect.
I agree this would be a better way to handle it.
To keep things simple we could also redirect all event-types of member1 to member2 profile page from where the user can choose which event suits them. It can't be done through TempOrgRedirect currently. We might have to introduce a new RedirectType which could be
RedirectType.UserToProfile
Yes assigning individual event-types of a user to event-types of other users would be way too much.
Another problem could be that the username could be allocated to some new member later on. So, I think we shouldn't show the user as deleted in this case. They should be shown as 'Replaced by Member2'.
Not sure wdym by this, where do you want to show Replaced by Member2?
Admin can then delete the redirect(e.g. when a new member with same username joins) or replace that with another Member(say Member3). We could tackle this later but for that to happen we really need to have separate entity for
Replacementso that we can show that in UI later
I suggested earlier we can show this in a table format where owners and admins can delete and update redirects instead of making it fixed to a single user. Lmk what you think.
A direct entry to TempOrgRedirect is unmanageable
This pr relies on tempOrgRedirect completely, I also noticed this issue #15575 aiming to deprecate tempOrgRedirect. Should I wait till final decisions are made for it before continuing with this pr?
Not sure wdym by this, where do you want to show Replaced by Member2?
I meant to show it somewhere in Members listing only. Right now it just stops existing. But we can have a placeholder in there, still for this purpose. This is just my thought though.
I suggested earlier we can show this in a table format where owners and admins can delete and update redirects instead of making it fixed to a single user. Lmk what you think.
Yeah I think something like this would be necessary in the end. As I shared earlier, we can postpone the 'showing in UI' part but we have to keep the data in a table about the replacement, so that UI can be built later over it.
This pr relies on tempOrgRedirect completely, I also noticed this issue https://github.com/calcom/cal.com/issues/15575 aiming to deprecate tempOrgRedirect. Should I wait till final decisions are made for it before continuing with this pr? I don't mind introducing a new RedirectType that I shared earlier, as it would make sense in long term as well. So, we can implement it in current table only.
TempOrgRedirect would have all it's data in future too but in a better schema that understands relationship with different entities better. It would be like holding raw redirects due to any reasons. Any redirect on the booking page, it would be aware of it.
I meant to show it somewhere in Members listing only. Right now it just stops existing. But we can have a placeholder in there, still for this purpose. This is just my thought though.
Yeah I think something like this would be necessary in the end. As I shared earlier, we can postpone the 'showing in UI' part but we have to keep the data in a table about the replacement, so that UI can be built later over it.
Imo showing it directly in member listing would require unnecessary extra work as the profiles are being deleted, to show those we would need to keep it or add an entry to profile as placeholder and maybe also another field to know that it was deleted.
If its okay with you then we can introduce the new ui in this pr itself.
I don't mind introducing a new RedirectType that I shared earlier, as it would make sense in long term as well. So, we can implement it in current table only.
New RedirectType would definitely make things easier.
@hariombalhara made changes so that redirect created for member1 to member2 would redirect /acme.cal/member1 to /acme.cal/member2 and also redirect /acme.cal/member1/{event-type} to /acme.cal/member2/. Added a new RedirectType and used that for both.
Only remaining part is ui. Not sure where to show it tho. Org members table is has infinite scrolling, so it would be a issue to show it below the members table for big orgs.
@Amit91848 I will review and test it out soon. It is a tricky thing and it needs to be implemented correctly.
cc @PeerRich for review as well.
i wonder if we can define a generic "backup" system where if people go OOO or their account gets deleted it will redirect
This PR is being marked as stale due to inactivity.
Hey @hariombalhara any updates on this?
@Amit91848 I would like to keep it on the back burner for now as it isn't a priority and this isn't a trivial solution.
This PR is being marked as stale due to inactivity.
Closing it as this is not too prio for now