Skip to content

Digest for Unmoderated Notes #7987

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

Closed
wants to merge 5 commits into from

Conversation

keshavsethi
Copy link
Member

@keshavsethi keshavsethi commented Jun 4, 2020

Fixes #7986
This is a digest for unmoderated posts that go to admin and moderators and scheduled on a daily and weekly basis. For unmoderated posts status is "4" and it is taken as an argument in content_followed_in_period.
Please refer to following Screenshots and gif
ezgif com-video-to-gif (4)
Screenshot from 2020-06-04 14-20-41
Screenshot from 2020-06-04 14-11-17
For testing, spam digest button is added in /spam for daily digest.
Tests for this are left, I am working on that.
Please review @jywarren @SidharthBansal @pydevsg @ananyaarun @VladimirMikulic @cesswairimu @emilyashley @Uzay-G
Thanks!! ❤️ ✌️

@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #7987 into master will decrease coverage by 2.02%.
The diff coverage is 72.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7987      +/-   ##
==========================================
- Coverage   82.17%   80.15%   -2.03%     
==========================================
  Files          97       98       +1     
  Lines        5643     5698      +55     
==========================================
- Hits         4637     4567      -70     
- Misses       1006     1131     +125     
Impacted Files Coverage Δ
app/jobs/digest_spam_job.rb 33.33% <33.33%> (ø)
app/mailers/admin_mailer.rb 97.40% <75.00%> (-2.60%) ⬇️
app/models/user.rb 90.80% <86.66%> (-0.25%) ⬇️
app/controllers/admin_controller.rb 78.33% <100.00%> (+0.27%) ⬆️
app/controllers/stats_controller.rb 73.62% <100.00%> (ø)
app/channels/application_cable/channel.rb 0.00% <0.00%> (-100.00%) ⬇️
app/channels/application_cable/connection.rb 0.00% <0.00%> (-100.00%) ⬇️
app/channels/user_channel.rb 0.00% <0.00%> (-83.34%) ⬇️
app/channels/user_notification_channel.rb 0.00% <0.00%> (-83.34%) ⬇️
app/channels/room_channel.rb 0.00% <0.00%> (-71.43%) ⬇️
... and 13 more

@Uzay-G
Copy link
Member

Uzay-G commented Jun 4, 2020

@keshavsethi could you please have a look at the codeclimate problems? thanks.

@@ -371,6 +371,11 @@ def queue
end
end

def test_digest_email_spam
DigestSpamJob.perform_async(0)
Copy link
Member

Choose a reason for hiding this comment

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

What does perform_async do here? Does it send the email in the background?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, It is just for testing. It performs Job and creates Digest on daily basis. perform_async gets pushed to the Queue right away. 0 is for daily digest and 1 is for a weekly digest

@@ -0,0 +1,15 @@
class DigestSpamJob
include Sidekiq::Worker
def perform(frequency_digest)
Copy link
Member

Choose a reason for hiding this comment

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

Later on we could maybe add a way for moderators to customize the frequency of their digest so that only those who want it will receive theirs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now it just checks for tags digest:daily, digest:weekly, and if any of them is present then digest is sent. rn it tasks whatever is the settings of the moderator in the notification settings page. I will add more customization for moderators specifically for spam digest like frequency, type, and status (By tagging different tag like spam_digest:daily)
I was thinking of doing this on /spam2 and make the settings page there. But as of now, it is not merged. Should I add it to the current notification settings page for moderators??

@keshavsethi keshavsethi changed the title digest_spam Digest for Unmoderated Notes Jun 4, 2020
Copy link
Member

@pydevsg pydevsg left a comment

Choose a reason for hiding this comment

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

Good work @keshavsethi

@jywarren
Copy link
Member

jywarren commented Jun 5, 2020

Wow, this is really cool! Thanks @keshavsethi !!

A couple questions - how does a user sign up for this, and/or change their settings?

And, would it be possible to write tests for this feature as part of this PR?

Thank you, very exciting!

@keshavsethi
Copy link
Member Author

Wow, this is really cool! Thanks @keshavsethi !!

A couple questions - how does a user sign up for this, and/or change their settings?

And, would it be possible to write tests for this feature as part of this PR?

Thank you, very exciting!

Right now it automatically gets subscribed to all the moderators and admins as per their normal digest settings(like if they are opting of the weekly digest for following posts then they will get unmoderated posts as well in separate digest). I was thinking of adding more customization in spam2. Here It justs checks for digest:daily or digest:weekly tag. I can make a different tag for moderation digest like spam:digest weekly etc. I am working on tests, should I push it in the same PR??
Thanks!! 😄

@keshavsethi
Copy link
Member Author

I made digest UI bit more refined and responsive, Please review.
ezgif com-video-to-gif (5)

@@ -356,6 +373,19 @@ def send_digest_email
end
end

def send_digest_email_spam
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add tests for these model methods?
Everything else looks good...great job 🎉

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will add it soon. Thanks!!

@keshavsethi keshavsethi requested a review from Uzay-G June 8, 2020 02:37
@@ -10,6 +10,11 @@
<li class="nav-item"><a class="nav-link <% if params[:action] == "spam_revisions" %> active <% end %>" href="/spam/revisions"><i class="fa fa-list"></i><span class="d-xs-inline d-md-none d-lg-inline"> Revisions</span></a></li>
<li class="nav-item"><a class="nav-link <% if params[:action] == "spam_comments" %> active <% end %>" href="/spam/comments"><i class="fa fa-comment"></i><span class="d-xs-inline d-md-none d-lg-inline"> Comments</span></a></li>
<li class="nav-item"><a class="nav-link" href="/people"><i class="fa fa-user"></i><span class="d-xs-inline d-md-none d-lg-inline"> Active Users</span></a></li>
<li class="nav-item">
<%= form_tag "/admin/test_digest_email_spam", method: :post do %>
<%= submit_tag "Test Spam Digest Email", class: "btn btn-sm btn-info", style: "width: 100%;" %>
Copy link
Member

Choose a reason for hiding this comment

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

Why is the name Test Spam Digest Email?
Also, please add corresponding tests in the PR.

Copy link
Member Author

@keshavsethi keshavsethi Jun 8, 2020

Choose a reason for hiding this comment

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

This is a testing button just to test digest for 1-day unmoderated nodes. I have changed its position on the profile page where it is visible only to moderators. Name is Test Spam Digest Email because it is for tests and was in /spam and it creates email digest. I have added some tests. Please review. Thanks!!

@keshavsethi keshavsethi requested a review from a team as a code owner June 8, 2020 23:38
Copy link
Collaborator

@cesswairimu cesswairimu left a comment

Choose a reason for hiding this comment

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

Looks good 🎉

@keshavsethi
Copy link
Member Author

@jywarren @SidharthBansal @pydevsg @ananyaarun @VladimirMikulic @cesswairimu @emilyashley @Uzay-G If this fine, Can we please merge this?? 🙏

@@ -305,4 +305,7 @@ class UserTest < ActiveSupport::TestCase
end
end

test 'Send digest email for unmoderated posts' do
assert users(:bob).send_digest_email_spam
Copy link
Member

Choose a reason for hiding this comment

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

Here, or in our functional tests, i wonder if we can check the content of the email itself to confirm? There are some other examples of this in our tests. It would help secure this feature against future issues!

@jywarren
Copy link
Member

OK, so currently those tags are set in https://publiclab.org/settings - and I think we're OK to rely on that for this setting as well. But let's anticipate that people might in the future want to separate out moderation digest settings from regular content digests. As long as that isn't too much extra work, i think we could modify the text shown here (just for moderators/admins) to say:

Do you want to receive a weekly digest of content you follow [, as well as one for moderation emails]?

image

What do you think? Or would it be easier to just make a unique user tag like digest:moderation:weekly digest:moderation:daily?

Thank you, this is so great! Fantastic work!!! 🎉 🎉 🎉

@keshavsethi
Copy link
Member Author

keshavsethi commented Jun 11, 2020

OK, so currently those tags are set in https://publiclab.org/settings - and I think we're OK to rely on that for this setting as well. But let's anticipate that people might in the future want to separate out moderation digest settings from regular content digests. As long as that isn't too much extra work, i think we could modify the text shown here (just for moderators/admins) to say:

Do you want to receive a weekly digest of content you follow [, as well as one for moderation emails]?

image

What do you think? Or would it be easier to just make a unique user tag like digest:moderation:weekly digest:moderation:daily?

Thank you, this is so great! Fantastic work!!!

@jywarren As mentioned in comment #7987 (comment)
Right now it automatically gets subscribed to all the moderators and admins as per their normal digest settings(like if they are opting of the weekly digest for following posts then they will get unmoderated posts as well in separate digest). I was thinking of adding more customization in spam2. Here It justs checks for digest:daily or digest:weekly tag. I can make a different tag for moderation digest like spam:digest weekly etc

@jywarren
Copy link
Member

OK, yes, let's move to digest:moderation:weekly and digest:moderation:daily tags and corresponding UI settings -- would you be able to develop a unified PR with those distinct user tags and some UI on /settings to be able to switch those on and off too? Thank you @keshavsethi this is really awesome!

@keshavsethi
Copy link
Member Author

OK, yes, let's move to digest:moderation:weekly and digest:moderation:daily tags and corresponding UI settings -- would you be able to develop a unified PR with those distinct user tags and some UI on /settings to be able to switch those on and off too? Thank you @keshavsethi this is really awesome!

Sure, I will work on its settings and tags, Thanks!

@keshavsethi
Copy link
Member Author

Please refer to #8058. I have made some changes to this and updated it in #8058
Thanks!!

@jywarren jywarren changed the base branch from master to main June 30, 2020 18:17
@jywarren
Copy link
Member

jywarren commented Jul 7, 2020

Great, closing this one!

@jywarren jywarren closed this Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Digest for Unmoderated Notes
6 participants