/plots2

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

Planning issue for Email Notifications GSoC project #1421

Closed
jywarren opened this Issue May 19, 2017 · 38 comments

Comments

Assignees

@StlMaris123 StlMaris123

Labels
summer-of-code
Projects
None yet
Milestone
  Email notificatio...
3 participants
@jywarren @StlMaris123 @ujithaperera
@jywarren
Copy link
Contributor

jywarren commented May 19, 2017 •
edited

This issue is to help plan out the stages of @StlMaris123's summer project on Email Notifications. Let's try to break this into phases so that Stella can tackle this project in self-contained sections that can come online one by one, starting with basic issues and moving on to more complex ones that build on earlier work.

Ideally we'll be able to see earlier phases come online at PublicLab.org, and see how they work in the real world before moving on to later phases.

Links

Here we'll organize the overall issue, but we can break out individual steps into their own issue as we go, so the comments section doesn't get overwhelming.

Phase 1: Email notifications without scheduling

#396 outlines a way to get emails sending to tag subscribers even if tags are added up to an hour after the post is published (currently a shortcoming of the tag notifications system). To break this up:

  • Create a new method called followers_who_dont_follow_tags in /app/models/tag.rb:
# returns a list of users who follow this tag, but don't follow the list of given tags;
# used for notifying users of new tagging on notes they don't already follow
def followers_who_dont_follow_tags(tags)
...
end

Phase 2: what's happened in the past X time period?

  • precursor to ActiveJob
  • add a function to User, something like user.content_followed_in_past_period(timePeriod) or user.followed_nodes_updated_since_time_ago(timePeriod) that returns nodes which are: a) followed/liked or followed by tag, b) updated, commented upon, or posted within the past timePeriod
    • unit test the method above
  • let's display the nodes resulting in a list on /profile/USERNAME/following (maybe .rss?) so we can visually confirm that it's working in production, and people can subscribe to an RSS feed of this.
  • once this is working, we can move on to actually email notifying periodically via an ActiveJob (see #1584)

Phase 2.5: email templating

  • format content from above user.content_followed_in_past_period(timePeriod) in an email with subject line Updates from PublicLab.org in the last TIME_PERIOD (we could default to week, for now) and a note that this is based on content and topic tags you follow; manage your subscriptions here -- complete in

    plots2/app/models/user.rb

    Lines 313 to 315 in c6b2d34

    def content_followed_in_past_period(time_period)
    self.node.where("created >= #{time_period.to_i} AND changed >= #{time_period.to_i}")
    end
  • make an email triggerable with a controller method and route like /subscription/digest, so we can test this without needing to commit to ActiveJob yet (moved to #2027)
    • make a controller for this, so there's an action the button would activate

Phase 3: ActiveJob scheduling

This has been moved to #1584

This is just preliminary - but should be a good example of how these projects can be broken up, and although I know it is a different order from your proposal, I think it's a good place to start.

After this, we could work on Subscriptions as a phase, using the ActiveJob work to send a daily email as an alternative option to the current "as it happens" email notifications. Then we might consider the Reply by email which will be pretty amazing. What do you think? Do you have more steps to add above to break this into even smaller pieces, or want to try adding phases with individual steps for the Subscriptions or Replies phases? Thanks, Stella!!!

@jywarren jywarren added this to the Email notifications overhaul milestone May 19, 2017

@jywarren jywarren assigned StlMaris123 May 19, 2017

@jywarren jywarren referenced this issue May 19, 2017

Open

wiki page edit email notifications (max 1 per hour) #396

@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented May 21, 2017 •
edited

Thank you so much @jywarren This is a great place to start. What is the validity of the phases? (Period you should take to complete the phases)
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented May 22, 2017

I think each one could plausibly be done in a week, maybe less -- what do you think? Maybe the second one in 3 days? But you tell me, we want to leave room for you to figure things out if you run into trouble.
@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented May 26, 2017

@jywarren Is it possible to have a call? There are some things I would like to get clear before embarking on the issue
@ujithaperera

This comment has been minimized.

Sign in to view
Copy link
Contributor

ujithaperera commented May 26, 2017

Yeah I also think that it is a good idea to have a small voice call and clear all the doubts. then it'll be more productive to boost up the first task.
@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented May 30, 2017

@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented May 31, 2017

Yes, I'd be happy to. I'll be available on Thursday, but it may have to wait until Monday for us to find a time as I'm currently on a trip. What times are you free on Thursday? Thanks!
@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented May 31, 2017

@ujithaperera

This comment has been minimized.

Sign in to view
Copy link
Contributor

ujithaperera commented May 31, 2017 •
edited by jywarren

@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented May 31, 2017

@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented Jun 1, 2017

@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 1, 2017

10am would be really great, i'll look for you then, @ujithaperera is that OK?
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 1, 2017

I also made some additional suggestions and edits to break up into additional phases. @StlMaris123 - what do you think of these changes? See how I'm making it into as small components as possible so we can publish the code and try it out incrementally and step by step, rather than having to have all the parts running all at once?
@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented Jun 2, 2017

@ujithaperera

This comment has been minimized.

Sign in to view
Copy link
Contributor

ujithaperera commented Jun 2, 2017 •
edited by jywarren

@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 2, 2017

hi, all, i'm in the chatroom now!
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 2, 2017

https://riot.im/app/#/room/#publiclab:matrix.org or chat.publiclab.org
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 2, 2017

Hmm, it looks like we didn't manage to sync timezones. I could try again on Monday though. This would be the same as the time I linked to above -- please click the link to see: http://everytimezone.com/#2017-6-5,120,cn3

The chart says it's 7:30PM in +5.5 (@ujithaperera), and 5:00PM in +3 (@StlMaris123). Will that work?

Can you read over teh changes I made in the plan above, before then, and write back with some questions here? Thank you!
@ujithaperera

This comment has been minimized.

Sign in to view
Copy link
Contributor

ujithaperera commented Jun 2, 2017

Actually I am available now. I joined to the chat room. but still finding a way to connect to you
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 2, 2017

ah ok, but is StellaMaris available? It's evening there now.
@ujithaperera

This comment has been minimized.

Sign in to view
Copy link
Contributor

ujithaperera commented Jun 2, 2017

she said that she is available. But it's looks like she is not in the chat room right now.
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 2, 2017

It looks like she may have joined at 10am at her local time, and is probably offline now (as should be, on a friday night -- you too!) But will Monday work for you at the time I linked to above?
@ujithaperera

This comment has been minimized.

Sign in to view
Copy link
Contributor

ujithaperera commented Jun 2, 2017

yes :) . I'm available on Monday.
@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented Jun 2, 2017

@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 2, 2017

@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented Jun 2, 2017

@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 2, 2017

@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented Jun 5, 2017

@ujithaperera

This comment has been minimized.

Sign in to view
Copy link
Contributor

ujithaperera commented Jun 5, 2017

hi @StlMaris123 ,

I'm available.
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 5, 2017

We are coordinating in the chat room: https://riot.im/app/#/room/#publiclab:matrix.org
@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented Jun 5, 2017

@StlMaris123 StlMaris123 referenced this issue Jun 9, 2017

Closed

Dont follow tags #1455

2 of 3 tasks complete

@jywarren jywarren added the summer-of-code label Jun 12, 2017

@jywarren jywarren referenced this issue Jun 12, 2017

Closed

New tag email fixes#1421 #1452

2 of 3 tasks complete
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jun 12, 2017

I adjusted the line:

ensure the above method has the conditions (using a unit test):

adding the unit test idea. Model methods like the one you're working on should be unit testable. Thanks!

@StlMaris123 StlMaris123 referenced this issue Jun 14, 2017

Closed

List Users Following tags #1459

3 of 3 tasks complete

@jywarren jywarren referenced this issue Jun 19, 2017

Merged

Followers not following given tags fixes#1421 #1472

3 of 3 tasks complete
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jul 6, 2017

I made some updates to, and added detail to, Phase 2, and reviewed this with @ujithaperera -- as soon as #1481 and #1507 are complete, we can start on those!
@ujithaperera

This comment has been minimized.

Sign in to view
Copy link
Contributor

ujithaperera commented Jul 18, 2017

hi @StlMaris123 , unfortunately we were unable to windup the session properly.
FYI, I can not find a method called subscriptions in node model. This is the reference I have used.
https://github.com/publiclab/plots2/blob/master/app/models/node.rb

may be this update still in your local. Anyway I was talking about, subscriptions method that you using in your followers_who_dont_follow_tags method is not referred to the node model. it's referring to the Tag model itself. So its calling the subscriptions method in Tag model not in the Node model. And same happens in your subscription_mailer also. it's not referring to the Node model. Perhaps my reading may be wrong. you may have any other implementation. If not you can consider this issue.

@jywarren I have explained the requirement of a RSS feed and describe the way that RSS feed works for the mail notification for the subscriptions. But we have a doubt regarding the method user.content_followed_in_past_period(timePeriod) in phase 2 and 2.5 . what is the exact value that we passed to this method as a parameter. in other words what would be the value for this timePeriod . And would like to hear suggestions also.

Hope @StlMaris123 will catch the things up after she established her connection back.
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jul 18, 2017

In Rails you can pass time as values such as 1.weeks or 3.months -- weird but true. And then you can make queries like in these methods:

https://github.com/publiclab/plots2/blob/master/app/controllers/stats_controller.rb#L14

However watch out because some models (from previous Drupal system) do not have native Rails-style timestamps. And the naming is a bit inconsistent. Check out the link above to see how you can use ranges of time for various models in the app. Start with smaller pieces and work out from there, and open a PR sooner rather than later so everyone can see your work!
@StlMaris123

This comment has been minimized.

Sign in to view
Copy link
Collaborator

StlMaris123 commented Jul 20, 2017

@ujithaperera

This comment has been minimized.

Sign in to view
Copy link
Contributor

ujithaperera commented Jul 21, 2017 •
edited

@StlMaris123 since your are getting test proven correct results then most of the time your implementation process doesn't matter actually. I just wanted to let you know about following attribute.
Okay then. Hope you all done with phase 1. If you have any additional updates, you can push them also. then @jywarren and me can review them also.
Let's create a new PR for the phase 2. Then we can start our conversation in new PR.

@StlMaris123 StlMaris123 referenced this issue Aug 22, 2017

Closed

Active Scheduling of emails (pending Rails 4.2) #1584

1 of 7 tasks complete
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jan 16, 2018

Hi, all! I'm closing this up as it's now reflected in #1584 by @StlMaris123 and #2027 which I just created. Thanks everybody!!!
@jywarren

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jywarren commented Jan 16, 2018

And congrats to @StlMaris123 for completing a great deal of work on this -- just a few more parts to go!

@jywarren jywarren closed this Jan 16, 2018

@jywarren jywarren referenced this issue Feb 8, 2018

Open

Record peoples' timezones #2278

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment