[WIP] Create Update Feed #117

aajjbb · 2017-07-28T22:36:47Z

This PR has the goal of creating a update feed in LuaRocks, basically showing the users whats new from the modules and users they follow.

…re main objects for the update-feed feature

…its from event model

aajjbb · 2017-07-28T23:38:30Z

spec/applications/modules_spec.moon

    assert.same 0, #followings
+    assert.same 0, #events


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

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

aajjbb · 2017-07-28T23:40:57Z

flows/followings.moon

@@ -41,5 +51,6 @@ class FollowingsFlow extends Flow
        "follow",
        @current_user

-    following\delete!
+    event\delete! if event


When running tests, if this if gets removed, the test fails with a nil. That's because the find of the event object fails. I'm currently investigating it but I have no clue yet.

…information about the events in the index page

… everyone who follows a model

aajjbb · 2017-08-05T21:27:39Z

There are a few details missing here besides a proper UI for the timeline.

create and deliver an event for module upload/update (with proper tests)
think of more test strategies for event/timeline_events to have a better coverage

…of actions will always be users

…ed/starred

leafo

Looks like a good start, here's my feedback from first pass

leafo · 2017-08-08T05:29:53Z

widgets/timeline_events.moon

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


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.

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.

leafo · 2017-08-08T05:30:17Z

widgets/timeline_events.moon

+
+            switch Events\model_for_object_type(row_event.object_object_type)
+              when Modules
+                mod = Modules\find row_event.object_object_id


needs to be preloaded

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

leafo · 2017-08-08T05:30:25Z

widgets/timeline_events.moon

+                  href: @url_for("module", user: user.slug, module: mod.name)
+                }, mod\name_for_display!
+              when Users
+                usr = Users\find row_event.object_object_id


needs to be preloaded

leafo · 2017-08-08T05:31:24Z

models/timeline_events.moon

+        if Events\model_for_object_type(event.object_object_type) == Users
+          @@create(Users\find(event.object_object_id), event)
+
+  @user_timeline: (user) =>


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

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

leafo · 2017-08-08T05:32:56Z

models/timeline_events.moon

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


object_type should be specified when querying by object_id

leafo · 2017-08-08T05:35:49Z

models/timeline_events.moon

+class TimelineEvents extends Model
+  @primary_key: { "user_id", "event_id" }
+
+  @create: (user, event) =>


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

leafo · 2017-08-08T05:36:28Z

models/timeline_events.moon

@@ -0,0 +1,32 @@
+db = require "lapis.db"
+import Model from require "lapis.db.model"
+import Events, Followings, Users from require "models"


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

leafo · 2017-08-08T05:36:43Z

models/events.moon

+    }}
+  }
+
+  @create: (user, object, event_type) =>


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

leafo · 2017-08-08T05:37:14Z

migrations.moon

+      {"event_type", enum}
+      {"source_user_id", enum}
+      {"object_object_id", enum}
+      {"object_object_type", foreign_key}


this should be enum

leafo · 2017-08-08T05:37:33Z

migrations.moon

+    create_table "events", {
+      {"id", serial}
+      {"event_type", enum}
+      {"source_user_id", enum}


these two should be foreign_key

source_user_id really should be a foreign key but event_type too ?

I'm not really sure if I understand what columns should be changed. I've changed the way I thought it made sense, could you check the changes out ?

leafo · 2017-08-09T12:19:51Z

flows/followings.moon

@@ -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) =>


indentation looks messed up

leafo · 2017-08-09T12:23:52Z

flows/followings.moon

@@ -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)


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?

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 ?

leafo · 2017-08-09T12:24:27Z

flows/followings.moon

@@ -35,6 +39,13 @@ class FollowingsFlow extends Flow
      type: Followings.types\for_db type
    }

+    event = Events\find {


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

…oid (n + 1) queries

leafo · 2017-08-13T19:19:38Z

spec/applications/modules_spec.moon

@@ -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"


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

leafo · 2017-08-13T19:20:19Z

spec/models/events_spec.moon

+  Modules
+  Users from require "spec.models"
+
+import Events, TimelineEvents from require "models"


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

leafo · 2017-08-13T19:20:52Z

spec/models/events_spec.moon

+      object: module
+      event_type: Events.event_types.subscription
+    }
+      )


weird formatting

…this fixed the broken preloads in timeline_events widget too

…liver to timelines and removal of events from timeline (in case of unsubscription)

aajjbb added 4 commits July 28, 2017 19:33

adding migrations and models from events and timeline_events, these a…

29a28ca

…re main objects for the update-feed feature

adapting following/flows to generate a event when a following occurs

5e49de5

adapting tests from models to check event creation and testing the un…

4c90eb3

…its from event model

fixing typos

7b195b2

aajjbb commented Jul 28, 2017

View reviewed changes

aajjbb added 6 commits August 5, 2017 01:04

adding a primary key to timeline_events

535cc1e

adding timeline_event to models and tests

094f399

starting to sketch an ui for timeline, currently it only lists small …

a9e44c1

…information about the events in the index page

fixing a typo

212aece

writing a simple logic to present follow/bookmark events

2c8b3f2

creating deliver method for TimelineEvents, adding an update event to…

267b290

… everyone who follows a model

aajjbb added 3 commits August 5, 2017 20:33

renamed the source of events and remove the type field as the source …

29738fc

…of actions will always be users

add an event to the followed/starred user when it's profile is follow…

2391d72

…ed/starred

adapting the events model to use source_user_id

2a4e099

aajjbb force-pushed the create-update-feed branch from c89cdda to 2a4e099 Compare August 7, 2017 18:55

adapting model, flow and tests to consider events to have source_user_id

e20ee34

aajjbb requested a review from leafo August 7, 2017 20:38

leafo requested changes Aug 8, 2017

View reviewed changes

merging with upstream/master

6040002

leafo reviewed Aug 9, 2017

View reviewed changes

aajjbb added 2 commits August 11, 2017 10:35

readding events and timeline_events tables in migrations

962eeda

adapt classes to use \create method with opts arguments

ab657c7

aajjbb force-pushed the create-update-feed branch from d4d0fa3 to ab657c7 Compare August 11, 2017 13:37

aajjbb added 2 commits August 11, 2017 13:08

adding relations of timeline_events and starting to use preload to av…

a9206c5

…oid (n + 1) queries

using preload correctly with lapis updated version

19c4ef4

aajjbb force-pushed the create-update-feed branch from d990483 to 19c4ef4 Compare August 13, 2017 18:51

leafo approved these changes Aug 13, 2017

View reviewed changes

removing the useless object_ prefix of the object columns in events, …

8517657

…this fixed the broken preloads in timeline_events widget too

aajjbb force-pushed the create-update-feed branch from b55129b to 8517657 Compare August 13, 2017 19:27

aajjbb added 6 commits August 13, 2017 16:30

fix import scope and weird formatting in tests

d5707fa

creates a flow to manage events, it manage event creation and it's de…

11e7091

…liver to timelines and removal of events from timeline (in case of unsubscription)

create a dedicated page for the timeline

1840b30

fixing timeline_event removal bug

fac47c3

adding time_ago_in_words in timeline_events widget

cecbecf

remove useless lines from module tests

20f9167

aajjbb force-pushed the create-update-feed branch from d5daf4b to 714cbcf Compare August 21, 2017 21:16

add event creation policy to user following

6bd7539

aajjbb force-pushed the create-update-feed branch from 714cbcf to 6bd7539 Compare August 21, 2017 21:21

aajjbb mentioned this pull request Aug 22, 2017

[WIP] Send an weekly email digest to users #121

Open

[WIP] Create Update Feed #117

Are you sure you want to change the base?

[WIP] Create Update Feed #117

Conversation

aajjbb commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aajjbb commented Aug 5, 2017

leafo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment