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

Rename org short code feature added #550

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yash-kh
Copy link
Contributor

@yash-kh yash-kh commented Jul 9, 2024

What does this PR do?

Rename org short code Option added in:
settings -> Organization -> Org profile

Fixes #307

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Copy link

graphite-app bot commented Jul 9, 2024

Graphite Automations

"Auto-assign PRs to author" took an action on this PR • (07/09/24)

1 assignee was added to this PR based on Rahul Mishra's automation.

Copy link
Member

@McPizza0 McPizza0 left a comment

Choose a reason for hiding this comment

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

I checked the code, everything looks ok

but i realized we will have some big issues if we enable this

the orgshortcode is passed to all api queries
and used to lookup org info from a redis cache

if we allow orgs to change shortcode, the old shortcode will still work for a while (till cache expires)
this isnt a problem

the problem is other users might still be using the old shortcode in the url
so they wouldnt know that the orgshortcode has changed

once the cache expires, the next api call they make will fail

need to work out the best way to redirect logged in org members to the new URL, without the risk of them writing a long email, pressing send, and the api call failing...

@BlankParticle thoughts?

@BlankParticle
Copy link
Member

BlankParticle commented Jul 10, 2024

there are lot of stuff tied to org short code, if we want to change org short code we need a list of burnt orgshort codes and make older codes redirect to new codes.

Invalidating cache is not a big issue tho

That also means every query would need to check if org shortcode has been redirected before proceding

I think we should delay this feature until we have a good flow planned

@yash-kh
Copy link
Contributor Author

yash-kh commented Jul 10, 2024

I checked some cases, if the user uses the URL with the old code it redirects to:
Screenshot 2024-07-10 at 6 08 49 PM

And yeah, the old code still works for the active user it's not updating automatically the inbox and conversations are opening fine.
Upon refresh, its same error.

@yash-kh
Copy link
Contributor Author

yash-kh commented Jul 10, 2024

Also, the orgProcedure uses shortCode which will be an issue.

@McPizza0
Copy link
Member

@BlankParticle I dont think we need to burn shortcodes the same way we burn usernames
but we do need a way to safely redirect logged in users to their new org shortcode
the realtime system is not viable for this

we already have a method to invalidate an org's shortcode cache
but again
that could mean if a user is in the middle of writing an email, they would end up getting errors and losing the email they wrote.

@McPizza0
Copy link
Member

@yash-kh 99% of queries use the orgProcedure method, so we can class it as "it would break everything"

@BlankParticle
Copy link
Member

I think we should put this in backlog for now

@BlankParticle BlankParticle added the backlog these issues would not be addressed until a certain checkpoint is reached label Jul 15, 2024
@McPizza0 McPizza0 marked this pull request as draft July 15, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog these issues would not be addressed until a certain checkpoint is reached
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(settings): Add an option to rename an organization's shortcode.
3 participants