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

fix: fix error when updating Telegram ID for an existing phone number #1178

Merged
merged 5 commits into from May 10, 2021

Conversation

zwliew
Copy link
Contributor

@zwliew zwliew commented May 6, 2021

Problem

Closes issue #1143

Solution

Bug Fixes:

  • Try updating either phone number or telegram ID. If no update occurs, insert a new row.

Tests

These tests have been verified on staging.

Users should be able to update telegram ID for an existing phone number

  1. Change your telegram_id to a different number in the telegram_subscribers table.
  2. Run the /updatenumber command on the Telegram bot
  3. The bot should say Your phone number and Telegram ID have been updated. and telegram_id should be updated in the telegram_subscribers table.

Users should be able to update phone number for an existing telegram ID

  1. Change your phone_number to a different number in the telegram_subscribers table.
  2. Run the /updatenumber command on the Telegram bot
  3. The bot should say Your phone number and Telegram ID have been updated. and phone_number should be updated in the telegram_subscribers table.

Users should be able to create a new telegram_subscribers row

  1. Delete the row corresponding to your account in the telegram_subscribers table.
  2. Run the /updatenumber command on the Telegram bot
  3. The bot should say Your phone number and Telegram ID have been updated. and there should be a new row reflecting your phone_number and telegram_id in the table.

@zwliew
Copy link
Contributor Author

zwliew commented May 6, 2021

I thought of one additional edge case when thinking about the fix. However, given that it sounds like a really rare scenario, it might not be worth handling it.

Imagine this scenario:

There are 2 rows in the telegram_subscribers table.

phone_number telegram_id
A 000
B 111

What if a user tries to update with the following parameters: phone_number=B, telegram_id=000?

I couldn't think of a clear cut answer. One solution would be to delete one of the rows and update the other. Another would be to treat both rows as the same and merge them.

Steps to achieve this scenario:

  1. A user creates 2 Telegram accounts and subscribes them to the bot as shown above.
  2. Then, the user with telegram_id=111 changes their phone_number to C on Telegram only. But, the user doesn't update this in Postman. Now, there is a desync between Postman's telegram_subscribers table and the canonical Telegram mapping.
  3. Lastly, the user with telegram_id=000 changes their phone_number to B. This time, the user runs /updatenumber to update Postman. However, since phone_number=B and telegram_id=000 both already exist in the table as 2 separate rows, this is an issue. It is unclear which row to update, and which row to delete/merge, in order to maintain uniqueness of the keys.

Decision
Ignore. This scenario will result in an error and will be caught by Sentry, so we'll be able to resolve it manually should we face it.

@zwliew zwliew force-pushed the fix-telegram-update-telegram-id branch from 1be2302 to 8c5ec1a Compare May 6, 2021 06:10
@lamkeewei
Copy link
Contributor

I thought of one additional edge case when thinking about the fix. However, given that it sounds like a really rare scenario, it might not be worth handling it.

Imagine this scenario:

There are 2 rows in the telegram_subscribers table.

phone_number telegram_id
A 000
B 111
What if a user tries to update with the following parameters: phone_number=B, telegram_id=000?

I couldn't think of a clear cut answer. One solution would be to delete one of the rows and update the other. Another would be to treat both rows as the same and merge them.

Steps to achieve this scenario:

  1. A user creates 2 Telegram accounts and subscribes them to the bot as shown above.
  2. Then, the user with telegram_id=111 changes their phone_number to C on Telegram only. But, the user doesn't update this in Postman. Now, there is a desync between Postman's telegram_subscribers table and the canonical Telegram mapping.
  3. Lastly, the user with telegram_id=000 changes their phone_number to B. This time, the user runs /updatenumber to update Postman. However, since phone_number=B and telegram_id=000 both already exist in the table as 2 separate rows, this is an issue. It is unclear which row to update, and which row to delete/merge, in order to maintain uniqueness of the keys.

I think this seem reasonably rare that we don't have to handle it.

zwliew added 4 commits May 7, 2021 14:13
Split the update into 4 cases:
1. same phone number and telegram ID
2. same phone number, different telegram ID
3. different phone number, same telegram ID <-- broken since
   phone_number is a primary key, and sequelize doesn't allow updates
   for primary keys
4. different phone number, different telegram ID

Minor improvements:
1. Make the bot replies more granular based on what actually happened in
   the backend.
Split into 2 scenarios instead.
@zwliew zwliew force-pushed the fix-telegram-update-telegram-id branch from 8c5ec1a to 3cd28fc Compare May 7, 2021 06:13
@lamkeewei lamkeewei merged commit 1a7e742 into develop May 10, 2021
@lamkeewei lamkeewei deleted the fix-telegram-update-telegram-id branch May 10, 2021 04:56
lamkeewei added a commit that referenced this pull request May 10, 2021
* develop:
  fix: fix error when updating Telegram ID for an existing phone number (#1178)
  chore: upgrade React; use new JSX transform; sort imports (#1129)
  fix(backend): docker build and tsc output directory structure (#1177)
  feat: refactor msg template components; add telegram character limit (#1148)
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
lamkeewei added a commit that referenced this pull request May 18, 2021
* develop:
  chore: upgrade dependencies (#1180)
  Backend tests - updating template for channels (#1175)
  feat(rich-text): support image as links (#1158)
  fix: fix error when updating Telegram ID for an existing phone number (#1178)
  chore: upgrade React; use new JSX transform; sort imports (#1129)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants