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

AutoSubscribe to Issue when create or comment to it #9535

Conversation

6543
Copy link
Member

@6543 6543 commented Dec 29, 2019

Fix #8243 auto subscribe issue on interact
try to fix issue #9233 (this is a independend bug)
close #8852

  • add IssueWatchMode type

    • update functions
  • correct Tests

  • add Tests

  • remove race when create/update IW

  • migration

@6543 6543 force-pushed the fix_8243-auto-subscribe-issue-on-interact branch from 4e5df52 to e8e1d17 Compare December 30, 2019 18:00
@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jan 1, 2020
@6543 6543 force-pushed the fix_8243-auto-subscribe-issue-on-interact branch from a511262 to c8bcaf0 Compare January 2, 2020 09:53
@6543 6543 mentioned this pull request Jan 7, 2020
@6543 6543 force-pushed the fix_8243-auto-subscribe-issue-on-interact branch from c8bcaf0 to b318aa6 Compare January 7, 2020 13:57
routers/repo/issue.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 7, 2020
@6543
Copy link
Member Author

6543 commented Jan 21, 2020

also look at #9118

@6543 6543 mentioned this pull request Jan 23, 2020
5 tasks
@6543 6543 force-pushed the fix_8243-auto-subscribe-issue-on-interact branch from 39dc312 to 7ff8521 Compare January 30, 2020 07:29
@6543 6543 force-pushed the fix_8243-auto-subscribe-issue-on-interact branch 5 times, most recently from 0d2ca3f to 2d7e067 Compare February 13, 2020 09:43
@codecov-io
Copy link

codecov-io commented Feb 13, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@9f7148c). Click here to learn what that means.
The diff coverage is 67.4%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #9535   +/-   ##
========================================
  Coverage          ?   43.7%           
========================================
  Files             ?     587           
  Lines             ?   81204           
  Branches          ?       0           
========================================
  Hits              ?   35493           
  Misses            ?   41313           
  Partials          ?    4398
Impacted Files Coverage Δ
modules/indexer/code/indexer.go 42.62% <ø> (ø)
models/issue.go 53.82% <0%> (ø)
routers/home.go 46.8% <0%> (ø)
models/user.go 49.54% <0%> (ø)
models/user_mail.go 64.18% <100%> (ø)
modules/setting/indexer.go 91.89% <100%> (ø)
routers/repo/search.go 65.62% <100%> (ø)
modules/git/repo_language_stats.go 66.17% <100%> (ø)
modules/indexer/code/wrapped.go 40.74% <66.66%> (ø)
modules/indexer/code/bleve.go 64.7% <68.83%> (ø)
... and 2 more

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 9f7148c...f29de15. Read the comment docs.

@6543 6543 changed the title [WIP] Refactor issue subscription Refactor issue subscription Feb 13, 2020
@6543 6543 force-pushed the fix_8243-auto-subscribe-issue-on-interact branch from 7f25bb4 to d667d6f Compare February 13, 2020 20:59
@6543 6543 changed the title Refactor issue subscription AutoSubscribe to Issue when create or comment to it Feb 13, 2020
@6543
Copy link
Member Author

6543 commented Feb 13, 2020

I think now it is more a feature or enhancement than a refactor can we change lables :)

@6543 6543 force-pushed the fix_8243-auto-subscribe-issue-on-interact branch from c7c504f to d9ce9f9 Compare February 13, 2020 21:22
@zeripath zeripath added type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Feb 13, 2020
@zeripath zeripath added this to the 1.12.0 milestone Feb 13, 2020
models/issue_watch.go Outdated Show resolved Hide resolved
models/issue_watch.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 19, 2020
@6543
Copy link
Member Author

6543 commented Feb 19, 2020

@guillep2k would be nice if you had time to look at it :)

Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

My question is still standing: why are auto-watch subscriptions needed on issues when participants should be automatically notified anyway? Am I missing something? 🤔

if err := mailer.MailParticipantsComment(comment, act, issue); err != nil {
log.Error("MailParticipantsComment: %v", err)
}

if err := mailer.MailParticipants(issue, doer, actionType); err != nil {
log.Error("MailParticipants: %v", err)
}

etc.

I know that this doesn't cover the dashboard feed (action table), but subscribing the participants will make the code linked above redundant. Maybe we should remove that code and let subscriptions handle the notifications? If so, we'd need to auto-subscribe all participants in the migration step.

integrations/README.md Outdated Show resolved Hide resolved
@6543
Copy link
Member Author

6543 commented Feb 19, 2020

I know that this doesn't cover the dashboard feed (action table), but subscribing the participants will make the code linked above redundant. Maybe we should remove that code and let subscriptions handle the notifications? If so, we'd need to auto-subscribe all participants in the migration step.

I would prefere if a action witch triger a email notification also create a notification for ui - this inconsistency is anoing for me

@guillep2k
Copy link
Member

I would prefere if a action witch triger a email notification also create a notification for ui - this inconsistency is anoing for me

I understand that, but now there are two paths in which participants will be receiving email notifications: via issue_watch and via mailer.MailParticipants. It's redundant! I think we should either carefully ⑴ remove the "mail participants" logic from modules/notification/mailer or duplicate that logic in modules/notification/action for the dashboard feed, leaving the issue_watch table rest as it is.

I say "carefully" because there are several subtleties in how the emails are tailored for each notification type.

@6543
Copy link
Member Author

6543 commented Feb 21, 2020

@guillep2k I'll split up changes of this pull to make independent desicions about how UI & Mail Notifications are unifiedy and how to make this configurable

@6543
Copy link
Member Author

6543 commented Mar 2, 2020

since #10473 was merged ... should we still migrate db to use integer instead of bool ( move from is_watching to mode colum) so we can add different modes in future - if so I'll remove IssueWatchModeAuto for now and rebase?

EDIT: modesl like: "notify only on state-change" & "ignore mentions too" ...

@6543
Copy link
Member Author

6543 commented Mar 2, 2020

👍 for rebase and migrate
👎 for close this

@zeripath @guillep2k

@guillep2k
Copy link
Member

I don't think this is needed after your other PR was merged. Unless there's something else I'm missing. 😁

@6543 6543 closed this Mar 3, 2020
@6543 6543 deleted the fix_8243-auto-subscribe-issue-on-interact branch March 3, 2020 02:39
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues Subscription different Types inform participants on UI too / autosubscribe to issue
6 participants