cal.com icon indicating copy to clipboard operation
cal.com copied to clipboard

feat: Orgs: Auto-redirect after deleting team members

Open Amit91848 opened this issue 1 year ago • 10 comments

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 avatar Jun 02 '24 13:06 Amit91848

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

A member of the Team first needs to authorize it.

vercel[bot] avatar Jun 02 '24 13:06 vercel[bot]

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

github-actions[bot] avatar Jun 02 '24 13:06 github-actions[bot]

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.

graphite-app[bot] avatar Jun 02 '24 13:06 graphite-app[bot]

Do you think we should also show whatever redirects have been created for an org, similar to the table in ooo?

Amit91848 avatar Jun 04 '24 10:06 Amit91848

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

sean-brydon avatar Jun 04 '24 10:06 sean-brydon

LGTM - tested and it works perfectlly. Thanks for the tests , great job as always @Amit91848

sean-brydon avatar Jun 18 '24 12:06 sean-brydon

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.

hariombalhara avatar Jun 26 '24 08:06 hariombalhara

Thanks for the review @hariombalhara , I have a few thoughts

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.

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 Replacement so 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?

Amit91848 avatar Jun 27 '24 12:06 Amit91848

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.

hariombalhara avatar Jun 27 '24 12:06 hariombalhara

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.

Amit91848 avatar Jun 27 '24 13:06 Amit91848

@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 avatar Jul 08 '24 11:07 Amit91848

@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.

hariombalhara avatar Jul 29 '24 08:07 hariombalhara

i wonder if we can define a generic "backup" system where if people go OOO or their account gets deleted it will redirect

PeerRich avatar Jul 29 '24 08:07 PeerRich

This PR is being marked as stale due to inactivity.

github-actions[bot] avatar Aug 13 '24 00:08 github-actions[bot]

Hey @hariombalhara any updates on this?

Amit91848 avatar Aug 16 '24 08:08 Amit91848

@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.

hariombalhara avatar Aug 23 '24 06:08 hariombalhara

This PR is being marked as stale due to inactivity.

github-actions[bot] avatar Sep 07 '24 00:09 github-actions[bot]

Closing it as this is not too prio for now

anikdhabal avatar Oct 22 '24 12:10 anikdhabal