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: UI for telegram user notifications #1649

Merged
merged 3 commits into from
Jul 24, 2023

Conversation

goooseman
Copy link

@goooseman goooseman commented Jul 3, 2023

Telegram user notifications

Resolves #830

This PR adds telegram subscription in case:

  1. Telegram is configured on BE and NOTIFY_USERS var contains user. In this case config.telegram_notifications should be true which was implemented by @paskal at Switch from telegram_bot_username to telegram_notifications in /config endpoint #1648.
  2. User is logged in with any of the providers

UI for a user which did not subscribe yet:
2023-07-03_19-36-42 (1)

If user is already subscribed, he will see "Subscribed" UI from the very beginning:
2023-07-03_19-37-18 (1)

Other changes

  1. TelegramLink component is created not to repeat UI of Telegram link + QR
  2. Common components/button component is used inside TelegramLink instead of one used before from the components/auth/components/button folder
  3. Global style to make links to be white in dark theme added.

Before
Screenshot 2023-07-03 at 19 39 54

After
Screenshot 2023-07-03 at 19 40 02

i18n

I do not see how the translation process is organized in the repo. Tell me if I need to update any of the .json files with the new.

@goooseman goooseman requested a review from umputun as a code owner July 3, 2023 17:04
@paskal paskal added the frontend label Jul 3, 2023
@paskal paskal requested review from Mavrin and akellbl4 July 3, 2023 17:59
@paskal paskal added this to the v1.12.0 milestone Jul 3, 2023
@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Patch coverage: 94.53% and project coverage change: +2.83 🎉

Comparison is base (497f3ce) 58.71% compared to head (9f93506) 61.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1649      +/-   ##
==========================================
+ Coverage   58.71%   61.55%   +2.83%     
==========================================
  Files         128      133       +5     
  Lines        2885     3004     +119     
  Branches      697      759      +62     
==========================================
+ Hits         1694     1849     +155     
+ Misses       1187     1038     -149     
- Partials        4      117     +113     
Impacted Files Coverage Δ
...pps/remark42/app/components/auth/auth.messsages.ts 100.00% <ø> (ø)
frontend/apps/remark42/app/common/api.ts 70.83% <60.00%> (-1.26%) ⬇️
...y-telegram/comment-form__subscribe-by-telegram.tsx 93.50% <93.50%> (ø)
frontend/apps/remark42/app/common/settings.ts 90.90% <100.00%> (+0.43%) ⬆️
...rontend/apps/remark42/app/components/auth/auth.tsx 81.42% <100.00%> (+0.17%) ⬆️
...end/apps/remark42/app/components/button/button.tsx 100.00% <100.00%> (ø)
...ents/comment-form/__subscribe-by-telegram/index.ts 100.00% <100.00%> (ø)
...ark42/app/components/comment-form/comment-form.tsx 53.30% <100.00%> (+1.59%) ⬆️
...remark42/app/components/telegram/telegram-link.tsx 100.00% <100.00%> (ø)
...ark42/app/components/telegram/telegram.messages.ts 100.00% <100.00%> (ø)
... and 1 more

... and 23 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@paskal
Copy link
Collaborator

paskal commented Jul 3, 2023

Thanks a lot!

  1. Opening the subscribe window calls /api/v1/telegram/subscribe?site=SITE every time, which should be done only once after the page load. There is no need to store the results after the page reload, but within a single page load, it doesn't make sense to call it more than once.
  2. After subscription and closing the subscription window I can't unsubscribe, the interface tries to subscribe me again unsuccessfully:
    image
    Please check how email subscription knows that the user is already subscribed.

@goooseman
Copy link
Author

@paskal

Opening the subscribe window calls /api/v1/telegram/subscribe?site=SITE every time, which should be done only once after the page load. There is no need to store the results after the page reload, but within a single page load, it doesn't make sense to call it more than once.

Fixed. @Mavrin @akellbl4 FYI I've used a new custom useSessionStorage hook to save API call instead of redux which looks like project's standard, because I do not believe we need this state at other components of the application -> I do not believe global state should be used here. As per Uncle Bob global state can be a source of bugs, so I try to avoid using it to avoid application level side-effects. But please tell me if I need to use redux, I can refactor the code

After subscription and closing the subscription window I can't unsubscribe, the interface tries to subscribe me again unsuccessfully

Thanks for the bug. I did test the use-case where user refreshes the page, but did not test when user tries to unsubscribe immediately. A new integration test is created to cover this use-case and bug is fixed.

@paskal Another weird stuff: when I was testing yesterday I was receiving { "code": 17, "error": "already subscribed", ... } from GET /telegram/subscribe in case of user is already subscribed.

Today I receive 409 status code with empty response from the same endpoint for the same use-case.
Screenshot 2023-07-04 at 16 54 44

Which is weird, because I test it on my remote deployment of remark42, which was not changed since yesterday. Anyway, I've covered this additional response in the spec file and implemented it the same as { "code": 17, "error": "already subscribed", ... }

@goooseman
Copy link
Author

@Mavrin @akellbl4 And what about i18n? I see that if I run pnpm run translation-check && pnpm run translation-check I have json files regenerated.

Shall I do it in this PR or it is a separate process? Can't find it in the project readme.

Also, do I need to extract common sentences to some common namespace? Like common.unsubscribe instead of subscribeByTelegram.unsubscribe and subscribeByEmail.unsubscribe?

@goooseman
Copy link
Author

@Mavrin @paskal @akellbl4 @umputun just pinging in case this PR has been fallen into cracks

Copy link
Collaborator

@Mavrin Mavrin left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. It looks good.
Please run to re-generate translation dictionaries

pnpm translation:extract
pnpm translation:generate

https://remark42.com/docs/contributing/translations/

@goooseman
Copy link
Author

@Mavrin thank you for your comments, everything is fixed

@goooseman goooseman requested a review from Mavrin July 20, 2023 15:53
@Mavrin Mavrin self-requested a review July 20, 2023 16:08
Copy link
Collaborator

@Mavrin Mavrin left a comment

Choose a reason for hiding this comment

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

Could you please check size-limit check?

@goooseman
Copy link
Author

@Mavrin It has happened because of size check using npm install eventhough pnpm install used already in the same container. I've created a potential fix to skip npm install, but this fix requires your approval to be ran on CI to check if it helps

Mavrin
Mavrin previously approved these changes Jul 23, 2023
@umputun
Copy link
Owner

umputun commented Jul 23, 2023

@goooseman could you pls rebase the PR to make a reasonable number of logically-consistent commits pls? Squashing it on my side will make a single one and I'm not sure if this is a good idea

@goooseman goooseman force-pushed the feat/telegram-user-notifications branch from 4ba26ee to 9f93506 Compare July 24, 2023 06:59
@goooseman
Copy link
Author

@umputun done, but need re-apporval of CI files to run pipeline

@umputun umputun merged commit 7bc7703 into umputun:master Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Telegram Notify
4 participants