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

Planning issue for Email Notifications GSoC project #1421

Closed
12 of 13 tasks
jywarren opened this issue May 19, 2017 · 38 comments
Closed
12 of 13 tasks

Planning issue for Email Notifications GSoC project #1421

jywarren opened this issue May 19, 2017 · 38 comments
Assignees
@StlMaris123
Labels
summer-of-code
Milestone
Email notificatio...

Comments

@jywarren
Copy link
Member

jywarren commented May 19, 2017 •
edited
Loading

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 Active Scheduling of emails (pending Rails 4.2) #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 Display "activity from past day" in a list at /subscriptions/digest #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 mentioned this issue May 19, 2017
Open
@StlMaris123
Copy link
Collaborator

StlMaris123 commented May 21, 2017 •
edited
Loading

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
Copy link
Member Author

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
Copy link
Collaborator

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

@ujithaperera
Copy link
Contributor

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
Copy link
Collaborator

StlMaris123 commented May 30, 2017 via email

@jywarren
Copy link
Member Author

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
Copy link
Collaborator

StlMaris123 commented May 31, 2017 via email

@ujithaperera
Copy link
Contributor

ujithaperera commented May 31, 2017 via email •
edited by jywarren
Loading

@jywarren
Copy link
Member Author

jywarren commented May 31, 2017 via email

@StlMaris123
Copy link
Collaborator

StlMaris123 commented Jun 1, 2017 via email

@jywarren
Copy link
Member Author

jywarren commented Jun 1, 2017

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

@jywarren
Copy link
Member 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
Copy link
Collaborator

StlMaris123 commented Jun 2, 2017 via email

@ujithaperera
Copy link
Contributor

ujithaperera commented Jun 2, 2017 via email •
edited by jywarren
Loading

@jywarren
Copy link
Member Author

jywarren commented Jun 2, 2017

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

@jywarren
Copy link
Member Author

jywarren commented Jun 2, 2017

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

@jywarren
Copy link
Member 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
Copy link
Contributor

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

@jywarren
Copy link
Member Author

jywarren commented Jun 2, 2017

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

@ujithaperera
Copy link
Contributor

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

@jywarren
Copy link
Member 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
Copy link
Contributor

yes :) . I'm available on Monday.

@StlMaris123
Copy link
Collaborator

StlMaris123 commented Jun 2, 2017 via email

@jywarren
Copy link
Member Author

jywarren commented Jun 2, 2017 via email

@StlMaris123
Copy link
Collaborator

StlMaris123 commented Jun 2, 2017 via email

@jywarren
Copy link
Member Author

jywarren commented Jun 2, 2017 via email

@StlMaris123
Copy link
Collaborator

StlMaris123 commented Jun 5, 2017 via email

@ujithaperera
Copy link
Contributor

hi @StlMaris123 ,

I'm available.

@jywarren
Copy link
Member Author

jywarren commented Jun 5, 2017

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

@StlMaris123
Copy link
Collaborator

StlMaris123 commented Jun 5, 2017 via email

@StlMaris123 StlMaris123 mentioned this issue Jun 9, 2017
Closed
3 tasks
@jywarren jywarren mentioned this issue Jun 12, 2017
Closed
3 tasks
@jywarren
Copy link
Member Author

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 mentioned this issue Jun 14, 2017
Closed
3 tasks
@jywarren jywarren mentioned this issue Jun 19, 2017
Merged
3 tasks
@jywarren
Copy link
Member 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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Collaborator

StlMaris123 commented Jul 20, 2017 via email

@ujithaperera
Copy link
Contributor

ujithaperera commented Jul 21, 2017 •
edited
Loading

@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 mentioned this issue Aug 22, 2017
Closed
7 tasks
@jywarren
Copy link
Member Author

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
Copy link
Member Author

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

@jywarren jywarren mentioned this issue Feb 8, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@StlMaris123 StlMaris123

Labels
summer-of-code
Projects
None yet
Milestone
Email notifications overhaul
Development

No branches or pull requests
3 participants
@jywarren @ujithaperera @StlMaris123