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

[WIP] Create Update Feed #117

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
29a28ca
adding migrations and models from events and timeline_events, these a…
aajjbb Jul 28, 2017
5e49de5
adapting following/flows to generate a event when a following occurs
aajjbb Jul 28, 2017
4c90eb3
adapting tests from models to check event creation and testing the un…
aajjbb Jul 28, 2017
7b195b2
fixing typos
aajjbb Jul 28, 2017
535cc1e
adding a primary key to timeline_events
aajjbb Aug 5, 2017
094f399
adding timeline_event to models and tests
aajjbb Aug 5, 2017
a9e44c1
starting to sketch an ui for timeline, currently it only lists small …
aajjbb Aug 5, 2017
212aece
fixing a typo
aajjbb Aug 5, 2017
2c8b3f2
writing a simple logic to present follow/bookmark events
aajjbb Aug 5, 2017
267b290
creating deliver method for TimelineEvents, adding an update event to…
aajjbb Aug 5, 2017
29738fc
renamed the source of events and remove the type field as the source …
aajjbb Aug 5, 2017
2391d72
add an event to the followed/starred user when it's profile is follow…
aajjbb Aug 5, 2017
2a4e099
adapting the events model to use source_user_id
aajjbb Aug 7, 2017
e20ee34
adapting model, flow and tests to consider events to have source_user_id
aajjbb Aug 7, 2017
6040002
merging with upstream/master
aajjbb Aug 8, 2017
962eeda
readding events and timeline_events tables in migrations
aajjbb Aug 9, 2017
ab657c7
adapt classes to use \create method with opts arguments
aajjbb Aug 9, 2017
a9206c5
adding relations of timeline_events and starting to use preload to av…
aajjbb Aug 11, 2017
19c4ef4
using preload correctly with lapis updated version
aajjbb Aug 13, 2017
8517657
removing the useless object_ prefix of the object columns in events, …
aajjbb Aug 13, 2017
d5707fa
fix import scope and weird formatting in tests
aajjbb Aug 13, 2017
11e7091
creates a flow to manage events, it manage event creation and it's de…
aajjbb Aug 17, 2017
1840b30
create a dedicated page for the timeline
aajjbb Aug 17, 2017
fac47c3
fixing timeline_event removal bug
aajjbb Aug 18, 2017
cecbecf
adding time_ago_in_words in timeline_events widget
aajjbb Aug 18, 2017
20f9167
remove useless lines from module tests
aajjbb Aug 21, 2017
6bd7539
add event creation policy to user following
aajjbb Aug 21, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions flows/followings.moon
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

import Flow from require "lapis.flow"

import Followings, Notifications from require "models"
import Events, Followings, Notifications, TimelineEvents, Users from require "models"

import assert_error from require "lapis.application"

Expand All @@ -12,7 +12,8 @@ class FollowingsFlow extends Flow
super ...
assert_error @current_user, "must be logged in"

follow_object: (object, type) =>
follow_object: (object, type) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

indentation looks messed up

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.


f = Followings\create {
source_user_id: @current_user.id
:object
Expand All @@ -25,6 +26,9 @@ class FollowingsFlow extends Flow
Notifications\notify_for target_user, object,
type, @current_user

event = Events\create(@current_user, object, Events.event_types.subscription)
TimelineEvents\deliver(@current_user, event)
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you sure you're delivering the event to the right person? If I follow someone then I would expect an event to show up on their timeline?

Additionally, we already have notifications for follows, do we want to also have timeline events?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It depends, maybe not sending the event to the followed timeline is better (and save some processing).

Yeah, for followings, I guess the notifications that are present are enough.

Do you an idea about what kind of stuff show be added to a timeline ?


f

unfollow_object: (object, type) =>
Expand All @@ -35,6 +39,13 @@ class FollowingsFlow extends Flow
type: Followings.types\for_db type
}

event = Events\find {
Copy link
Collaborator

Choose a reason for hiding this comment

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

so when you unfollow something you'd like all the events cleaned from your timeline (eg. they pushed 10 new updates to their module) Given that, you'll need to use select to handle something like that, since find only returns one row. Also, you don't want to delete the event, but you want to delete the timeline_event

the logic to do this is semi-complicated, so maybe it could be a method on a model, or in a timeline flow

source_user_id: @current_user.id
object_object_id: object.id
object_object_type: Events\object_type_for_object object
event_type: Events.event_types.subscription
}

return unless following

if object.get_user
Expand Down
40 changes: 40 additions & 0 deletions models/events.moon
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
db = require "lapis.db"

import Model, enum from require "lapis.db.model"
import safe_insert from require "helpers.models"

class Events extends Model
@timestamp: true

@event_types: enum {
subscription: 1
bookmark: 2
update: 3
}

@relations: {
{"source_user", belongs_to: "Users"}
{"object", polymorphic_belongs_to: {
[1]: {"module", "Modules"}
[2]: {"user", "Users"}
}}
}

@create: (user, object, event_type) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to use an opts argument for all model creation, with assertions on required fields. I know there are some models in the codebase that use this style, but I've been updating them as I see them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

assert user, "missing event's user"
assert object, "missing event's object"
assert event_type, "missing event_type, events must have a type"

opts = {
:event_type
source_user_id: user.id
object_object_id: object.id
object_object_type: @@object_type_for_object object
}

event = safe_insert @, opts

return event

delete: () =>
db.delete @@table_name!, { id: @id }
32 changes: 32 additions & 0 deletions models/timeline_events.moon
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
db = require "lapis.db"
import Model from require "lapis.db.model"
import Events, Followings, Users from require "models"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to avoid putting module requires on the top level, and instead in the methods, to prevent any issues with circular dependencies in the future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed


class TimelineEvents extends Model
@primary_key: { "user_id", "event_id" }

@create: (user, event) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to use an opts argument for all model creation, with assertions on required fields. I know there are some models in the codebase that use this style, but I've been updating them as I see them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

super {
user_id: user.id
event_id: event.id
}

@delete: (user, event) =>
db.delete @table_name!, { user_id: user.id, event_id: event.id }

@deliver: (user, event) =>
switch event.event_type
when Events.event_types.update
followers = Followings\select "where object_id = ?", event.object_object_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

object_type should be specified when querying by object_id

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed


for users in *followers
follower_user = Users\find users.source_user_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

preload the user, use the relation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@create(follower_user, event)
else
@@create(user, event)

if Events\model_for_object_type(event.object_object_type) == Users
@@create(Users\find(event.object_object_id), event)

@user_timeline: (user) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

you probably want to either paginate this, or limit to something reasonable like 50 for the time being

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm limiting by 50 but I plan to add pagination soon.

@@select "where user_id = ?", user.id
4 changes: 4 additions & 0 deletions models/users.moon
Original file line number Diff line number Diff line change
Expand Up @@ -234,3 +234,7 @@ class Users extends Model

uuid = generate_uuid()
"#{username}-#{uuid\gsub("-", "")\sub 1, 10}"

timeline: () =>
import TimelineEvents from require "models"
TimelineEvents\user_timeline @
14 changes: 12 additions & 2 deletions spec/applications/modules_spec.moon
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ import request_as from require "spec.helpers"
factory = require "spec.factory"


import Modules, Versions, Followings, Users, Notifications, NotificationObjects from require "models"
import Modules, Versions, Events, Followings, Users, Notifications, NotificationObjects from require "spec.models"
Copy link
Collaborator

Choose a reason for hiding this comment

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

spec.models adds a before_each to truncate, so you should move this entire line into the describe, and then you can remove the redundant trucate_tables method call in the existing before_each

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed


describe "applications.modules", ->
use_test_server!

before_each ->
truncate_tables Modules, Versions, Followings, Users, Notifications, NotificationObjects
truncate_tables Events, Modules, Versions, Followings, Users, Notifications, NotificationObjects

it "follows module", ->
current_user = factory.Users!
Expand All @@ -21,8 +21,13 @@ describe "applications.modules", ->
assert.same 302, status

followings = Followings\select!
events = Events\select!
user_timeline = current_user\timeline!

assert.same 1, #followings
assert.same 1, #events
assert.same 1, #user_timeline

following = unpack followings

assert.same current_user.id, following.source_user_id
Expand All @@ -44,7 +49,12 @@ describe "applications.modules", ->
assert.same 302, status

followings = Followings\select!
events = Events\select!
user_timeline = current_user\timeline!

assert.same 0, #followings
assert.same 0, #events
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fails because of the comment mentioned above in flows/followings.moon

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently "only" in tests the event created in follow_object can't be found and isn't deleted.

assert.same 0, #user_timeline

current_user\refresh!
mod\refresh!
Expand Down
63 changes: 63 additions & 0 deletions spec/models/events_spec.moon
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import use_test_env from require "lapis.spec"

factory = require "spec.factory"

import
Modules
Users from require "spec.models"

import Events, TimelineEvents from require "models"
Copy link
Collaborator

Choose a reason for hiding this comment

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

convert this to require from spec.models and put it inside the describe block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.


describe "models.events", ->
use_test_env!

it "creates an event of user following user", ->
user = factory.Users!
followed_user = factory.Users!

event = Events\create(user, followed_user, Events.event_types.subscription)
user_timeline = user\timeline!

assert.same user.id, event.source_user_id
assert.same followed_user.id, event.object_object_id
assert.same event.event_type, Events.event_types.subscription

assert.same, #user_timeline, 1

it "creates an event of user following a module", ->
user = factory.Users!
module = factory.Modules!

event = Events\create(user, module, Events.event_types.subscription)
user_timeline = user\timeline!

assert.same user.id, event.source_user_id
assert.same module.id, event.object_object_id
assert.same event.event_type, Events.event_types.subscription

assert.same, #user_timeline, 1

it "creates an event of user starring a module", ->
user = factory.Users!
module = factory.Modules!

event = Events\create(user, module, Events.event_types.bookmark)
user_timeline = user\timeline!

assert.same user.id, event.source_user_id
assert.same module.id, event.object_object_id
assert.same event.event_type, Events.event_types.bookmark

assert.same, #user_timeline, 1

it "deletes an event", ->
user = factory.Users!
module = factory.Modules!

event = Events\create(user, module, Events.event_types.bookmark)
event_id = event.id

event\delete!

assert.same nil, Events\find user.id, event_id
assert.same 0, #user\timeline!
7 changes: 6 additions & 1 deletion views/index.moon
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import to_json from require "lapis.util"

TimelineEvents = require "widgets.timeline_events"

class Index extends require "widgets.page"
content: =>
div class: @@css_classes!, ->
Expand All @@ -14,6 +16,10 @@ class Index extends require "widgets.page"
@inner_content!

inner_content: =>
if @current_user
h2 ->
text "Timeline"
widget TimelineEvents!

div class: "home_columns", ->
div class: "column", ->
Expand Down Expand Up @@ -63,4 +69,3 @@ class Index extends require "widgets.page"

script type: "text/javascript", ->
raw "new M.Index(#{@widget_selector!}, #{to_json @downloads_daily});"

43 changes: 43 additions & 0 deletions widgets/timeline_events.moon
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import Events, Modules, Users from require "models"

class TimelineEvents extends require "widgets.base"
@needs: {
"modules"
}

inner_content: =>
ul ->
for event in *@current_user\timeline!
row_event = Events\find(event.event_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

both of these will cause the n+1 queries issue: http://leafo.net/guides/postgresql-preloading.html#avoiding-n-and-1-queries

the solution in this case is to use the preload function ahead of time, then use the relation getter to get each object. You'll need to add the missing relation on the timeline events.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm currently preloading it in timeline method instead of in the widget as I guess it's cleaner to only have presentation in a widget.

user = Users\find(row_event.source_user_id)


message = switch row_event.event_type
when Events.event_types.subscription
" followed "
when Events.event_type.bookmark
" starred "
when Events.event_type.update
" delivered a new version of "
else
""
li ->
span class: "author", ->
a href: @url_for("user_profile", user: user.slug), user\name_for_display!
text message

switch Events\model_for_object_type(row_event.object_object_type)
when Modules
mod = Modules\find row_event.object_object_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs to be preloaded

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those are broken because the nested preload of object in timeline.event isn't working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

a {
class: "title",
href: @url_for("module", user: user.slug, module: mod.name)
}, mod\name_for_display!
when Users
usr = Users\find row_event.object_object_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs to be preloaded

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

a {
class: "title",
href: @url_for("user_profile", user: usr.slug)
}, usr\name_for_display!
else
""