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

Interest Timelines #174

Merged
merged 37 commits into from
Jul 18, 2017
Merged

Interest Timelines #174

merged 37 commits into from
Jul 18, 2017

Conversation

NuckChorris
Copy link
Member

@NuckChorris NuckChorris commented Jul 14, 2017

What It Is

Discussion spaces, customized to match the stuff you're interested in! Interest feeds are a combination of interest-general discussion, discussion of media in your library (excluding Dropped), and discussion of episodes you've seen or chapters you've read.

How it Works

We create a timeline for each user for each interest, have it automatically follow the interest-global feed. Then, when they update their library, we generate a MediaFollow (so they can unfollow discussion of specific series, but get it all by default). When a post is tagged with a unit, we send it to the corresponding unit feed. The interest timeline automatically follows all episodes you've seen in past.

Potential Improvements

  • It might get messy to follow all episode discussion. If this becomes a problem we could probably limit to the last N episodes you've seen ✅ Done!
  • The automatic MediaFollow might be better if we invert it, tracking unfollowed media instead. That way, we wouldn't try to recreate a follow you deleted ✅ Done!
  • Perhaps MediaFollow should control episode discussion too? ✅ Done!

What's Done

Everything!

What Needs Doing

  • Build-breaking issue
  • Change interest-global discussion into a flat feed
  • Backfill MediaFollows
  • Dump interest-global follows

@JoshFabian
Copy link
Contributor

Damn, this is a nice PR. Well-played!

@@ -213,6 +213,59 @@ def split_auto_follows(scope = User)
flatten(results)
end

def library_progress_follows(scope = User)

Choose a reason for hiding this comment

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

Assignment Branch Condition size for library_progress_follows is too high. [26.93/25] (http://c2.com/cgi/wiki?AbcMetric)

Copy link
Member

@toyhammered toyhammered left a comment

Choose a reason for hiding this comment

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

I understood about 50% of the code (I have not really dug deeply into feeds), but as for any code errors didn't really see many (left some comments on questionable stuff).

@@ -149,12 +149,9 @@ def activity
end

def kind
Copy link
Member

Choose a reason for hiding this comment

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

what would happen if kind is nil? I don't think that is actually possible but gonna throw it out there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Explosions, probably. But it's something that should never happen, so I'm not terribly worried about that

@@ -218,6 +211,18 @@ def kind
end
end

after_save if: :progress_changed? do
Copy link
Member

Choose a reason for hiding this comment

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

should this really be an after_save? Might be better to do after_commit on: :update and then wrap the statement in an if progress_changed? Only saying this because after_save deals with both create and update and you already have a create below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you're right

@NuckChorris NuckChorris force-pushed the feature/media-timelines branch from ed0de4f to a23f833 Compare July 18, 2017 06:50
@NuckChorris NuckChorris merged commit 801213a into the-future Jul 18, 2017
@NuckChorris NuckChorris deleted the feature/media-timelines branch August 14, 2017 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants