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.

Sign up for GitHub

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

Jump to bottom

Add UserDetailTelegram support #1038

Merged
merged 1 commit into from
Jun 13, 2021
Merged

Add UserDetailTelegram support #1038

merged 1 commit into from
Jun 13, 2021

Conversation

paskal
Copy link
Sponsor Collaborator

@paskal paskal commented Jun 7, 2021

For telegram notifications we'll need to get a telegram user ID from the user and store and retrieve it later, this PR adds support for that user details and improves a bunch of tests in affected modules.

All newly added code is tested.

@paskal paskal added the backend label Jun 7, 2021
@paskal paskal requested a review from umputun June 7, 2021 20:41
@paskal paskal changed the title add UserDetailTelegram support Add UserDetailTelegram support Jun 7, 2021
@paskal paskal force-pushed the paskal/user_detail_telegram branch from 41a931c to 86040cd Compare June 12, 2021 22:10
@codecov
Copy link

codecov bot commented Jun 12, 2021 •
edited
Loading

Codecov Report

Merging #1038 (86040cd) into master (91deae5) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

❗ Current head 86040cd differs from pull request most recent head 411b8e4. Consider uploading reports for the commit 411b8e4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1038      +/-   ##
==========================================
- Coverage   44.28%   44.18%   -0.11%     
==========================================
  Files         126      126              
  Lines        2897     2904       +7     
  Branches      653      653              
==========================================
  Hits         1283     1283              
- Misses       1602     1609       +7     
  Partials       12       12
Impacted Files Coverage Δ
frontend/app/components/root/root.tsx 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea15b28...411b8e4. Read the comment docs.

@paskal
Copy link
Sponsor Collaborator Author

paskal commented Jun 12, 2021

@akellbl4 do you know why codecov is running on that MR which doesn't change anything in the frontend and is reporting frontend/app/components/root/root.tsx problems?

umputun
umputun requested changes Jun 12, 2021
View reviewed changes
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

I have not checked much of the logic as it seems to me mostly an extension of the existing user email part of the managment. Just a couple of minor things

backend/app/notify/notify.go Show resolved Hide resolved
backend/app/notify/notify.go Outdated Show resolved Hide resolved
@paskal paskal force-pushed the paskal/user_detail_telegram branch 2 times, most recently from 02e3c3b to 429f9e4 Compare June 13, 2021 06:53
paskal
paskal commented Jun 13, 2021
View reviewed changes
backend/app/notify/notify.go
// getNotificationEmails returns list of emails for notifications for provided comment.
// Emails is not added to the returned list in case original message is from the same user as the notification receiver.
func (s *Service) getNotificationEmails(req Request, notifyComment store.Comment) (result []string) {
// getNotificationTargets returns list of notification targets (like email or telegram username) for users
Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

Updated the comment to reflect the new name, made results deduplicated to prevent the caller from doing it.

@coveralls
Copy link

coveralls commented Jun 13, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 933652465

  • 37 of 45 (82.22%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 83.601%
Changes Missing Coverage Covered Lines Changed/Added Lines %
backend/app/rest/api/admin.go 1 2 50.0%
backend/app/store/service/service.go 26 27 96.3%
backend/app/store/engine/bolt.go 1 7 14.29%
Totals Coverage Status
Change from base Build 932689304: -0.02%
Covered Lines: 5511
Relevant Lines: 6592
💛 - Coveralls

@paskal paskal requested a review from umputun June 13, 2021 15:04
umputun
umputun requested changes Jun 13, 2021
View reviewed changes
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

just a comment to fix and good to go

backend/app/store/engine/engine.go Outdated Show resolved Hide resolved
@paskal paskal force-pushed the paskal/user_detail_telegram branch from 429f9e4 to 411b8e4 Compare June 13, 2021 18:17
@paskal paskal requested a review from umputun June 13, 2021 18:17
@umputun umputun merged commit fd4c6ce into master Jun 13, 2021
@umputun umputun deleted the paskal/user_detail_telegram branch June 13, 2021 18:18
@paskal paskal added this to the v1.9 milestone Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umputun umputun Awaiting requested review from umputun umputun is a code owner

Assignees
No one assigned
Labels
backend
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@paskal @coveralls @umputun