-
-
Notifications
You must be signed in to change notification settings - Fork 377
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
Feature: Telegram Notify #830
Comments
Now it's up to @akellbl4 (or someone else familiar with the frontend) to implement the frontend part of the Telegram notifications. |
I want to help with this feature and I want to make my first FE contribution to the project. But can somebody give me an overview of what "FE support" means for this feature? |
There is a button in the web interface which takes you through the flow of subscribing to replies to your messages through email if the feature is enabled on the backend. For Telegram, I've created the endpoints for a similar subscription flow, but there are no buttons in the frontend to allow users to use it. It seems like I didn't describe the steps for a subscription yet. I'll do that once I'm near the computer so that this task will be easier to pick up. |
@paskal I've taken a look on the email subscriptions and looks like I can do the same feature for Telegram I have some question though:
|
Used endpoints are described in remark.rest:
Telegram notifications should be present to all users when enabled except for anonymous users, GitHub or email users can use it as well. No questions to the user, just asking them to click the link to Telegram and then click the link in the chat. The notification subscription flow is following:
The auth flow uses the first three steps as well. Please let me know if you have any questions. |
@paskal I've started to work on implementation, and I need to enable this feature on FE project-wise if:
So I need to know on FE if BE is configured to enable those notifications. I've taken a look at Can I request to add a property here? |
If From #1029:
This is not needed as the backend gives you a proper link to show in the frontend. |
@paskal Looks a little bit implicit, but it's ok for me. On FE we have a single place where this logic is implemented, so I believe it is ok to case string to bool there. |
I've changed the config option to boolean As for the subscription URL, I was wrong: for the |
@paskal I've found a bug Steps to reproduce
ExpectedThe API to fail with 4xx response code and some error message ActualRequest fails with 500 and empty response body. BE logs:
|
Reproduced both on my remote deployment and on demo.remark42.com |
Thanks a lot! As for the confusing error code, please path by yourself in your branch: diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go
--- a/backend/app/rest/api/rest_private_test.go (revision add01455fba91e6b96b6fe13c3be306773afc6bc)
+++ b/backend/app/rest/api/rest_private_test.go (date 1688587010194)
@@ -1156,8 +1156,8 @@
body, err = io.ReadAll(resp.Body)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
- require.Equal(t, http.StatusInternalServerError, resp.StatusCode, string(body))
- require.Equal(t, `{"code":0,"details":"can't set telegram for user","error":"not verified"}`+"\n", string(body))
+ require.Equal(t, http.StatusNotFound, resp.StatusCode, string(body))
+ require.Equal(t, `{"code":0,"details":"request is not verified yet","error":"not verified"}`+"\n", string(body))
mockTlgrm.notVerified = false
diff --git a/backend/app/rest/api/rest_private.go b/backend/app/rest/api/rest_private.go
--- a/backend/app/rest/api/rest_private.go (revision add01455fba91e6b96b6fe13c3be306773afc6bc)
+++ b/backend/app/rest/api/rest_private.go (date 1688586814273)
@@ -430,7 +430,7 @@
var address, siteID string
address, siteID, err := s.telegramService.CheckToken(queryToken, user.ID)
if err != nil {
- rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't set telegram for user", rest.ErrInternal)
+ rest.SendErrorJSON(w, r, http.StatusNotFound, err, "request is not verified yet", rest.ErrInternal)
return
}
|
@paskal I've tried to fix this bug in my branch, but I couldnt find how to run go tests. So I've skipped this change. Anyway it does not affect this functionality. |
I've updated the code above with test change, please apply it in your branch. |
The way the notifications are now made in telegrams is quite convenient for site administrators, but not at all convenient for users.
It would be cool to do, like on vas3k.club.
There you can subscribe for notifications to a separate post, as well as receive notifications to your comments.
The text was updated successfully, but these errors were encountered: