Closed goooseman closed 1 year ago
Patch coverage: 94.53
% and project coverage change: +2.83
:tada:
Comparison is base (
497f3ce
) 58.71% compared to head (9f93506
) 61.55%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Thanks a lot!
/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.Please check how email subscription knows that the user is already subscribed.
@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.
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", ... }
@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
?
@Mavrin @paskal @akellbl4 @umputun just pinging in case this PR has been fallen into cracks
@Mavrin thank you for your comments, everything is fixed
@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
@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
@umputun done, but need re-apporval of CI files to run pipeline
Telegram user notifications
Resolves #830
This PR adds telegram subscription in case:
NOTIFY_USERS
var containsuser
. In this caseconfig.telegram_notifications
should be true which was implemented by @paskal at #1648.UI for a user which did not subscribe yet:
If user is already subscribed, he will see "Subscribed" UI from the very beginning:
Other changes
TelegramLink
component is created not to repeat UI of Telegram link + QRcomponents/button
component is used insideTelegramLink
instead of one used before from thecomponents/auth/components/button
folderBefore
After
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.