User Stats #77

toyhammered · 2017-03-18T21:36:58Z

https://www.notion.so/User-Stats-78ece66c495e41c1929856524309957d

houndci-bot · 2017-03-19T18:29:53Z

app/models/stat/anime_genre_breakdown.rb

+        self.stats_data[slug] = 0
+      end
+
+      self.stats_data[slug] += 1


Redundant self detected. (https://github.com/bbatsov/ruby-style-guide#no-self-unless-required)

houndci-bot · 2017-03-19T18:29:53Z

app/models/stat/anime_genre_breakdown.rb

+
+    def increment_data(slug)
+      unless stats_data[slug]
+        self.stats_data[slug] = 0


Redundant self detected. (https://github.com/bbatsov/ruby-style-guide#no-self-unless-required)

houndci-bot · 2017-03-19T18:29:53Z

app/models/stat/anime_genre_breakdown.rb

+    end
+
+    def increment_data(slug)
+      unless stats_data[slug]


Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||. (https://github.com/bbatsov/ruby-style-guide#if-as-a-modifier)

houndci-bot · 2017-03-19T18:29:53Z

app/models/stat/anime_genre_breakdown.rb

+
+    # update data with + 1
+    def library_entry_update(library_entry)
+


Extra empty line detected at method body beginning.

houndci-bot · 2017-03-19T18:29:53Z

app/models/stat/anime_genre_breakdown.rb

+    end
+
+    # update data with + 1
+    def library_entry_update(library_entry)


Put empty method definitions on a single line. (https://github.com/bbatsov/ruby-style-guide#no-single-line-methods)

toyhammered · 2017-03-20T06:10:29Z

app/models/library_entry.rb

@@ -228,4 +228,15 @@ def myanimelist_linked_account
      type: 'LinkedAccount::MyAnimeList'
    )
  end
+
+  after_commit ->() { create_stat }, on: :create


I already know this name sucks

That's... actually not a bad name 🤔 That said, it might be better to just toss them in a block like this, depending on how much logic you'll have in there (and how many of these you need):

after_commit(on: :create) do Stat::AnimeGenreBreakdown.increment_genres(user, media.genres) end

toyhammered · 2017-03-20T06:10:38Z

app/models/library_entry.rb

@@ -228,4 +228,15 @@ def myanimelist_linked_account
      type: 'LinkedAccount::MyAnimeList'
    )
  end
+
+  after_commit ->() { create_stat }, on: :create
+  after_destroy ->() { delete_stat }


and this one

toyhammered · 2017-03-20T06:11:48Z

app/models/stat/anime_genre_breakdown.rb


-    def library_entries
-      user.library_entries.where(media_type: 'Anime')
+      p record


will remove

toyhammered · 2017-03-26T23:05:02Z

app/models/stat/anime_genre_breakdown.rb

+      end
+    end
+
+    def self.increment_genres(user, genres)


I kind of feel like that these methods can just be set to increment and decrement and then just used across all the other Stats (so they are all consistent, because we already know it is genres due to type)

Good thinking!

NuckChorris · 2017-03-27T03:37:40Z

app/models/library_entry.rb

@@ -228,4 +228,15 @@ def myanimelist_linked_account
      type: 'LinkedAccount::MyAnimeList'
    )
  end
+
+  after_commit ->() { create_stat }, on: :create


That's... actually not a bad name 🤔 That said, it might be better to just toss them in a block like this, depending on how much logic you'll have in there (and how many of these you need):

after_commit(on: :create) do Stat::AnimeGenreBreakdown.increment_genres(user, media.genres) end

NuckChorris · 2017-03-27T03:38:51Z

app/models/stat.rb

+class Stat < ApplicationRecord
+  belongs_to :user, required: true
+  # expose for jsonapi
+  alias_attribute :kind, :type


This is gonna need adjustment once #86 is merged

sure, looked at what you did and it seems good.

You don't need this with the STIResource

NuckChorris · 2017-03-27T03:39:18Z

app/models/stat.rb

+  # expose for jsonapi
+  alias_attribute :kind, :type
+
+  validates_presence_of :type, :stats_data


Generally I prefer validates :type, :stats_data, presence: true

NuckChorris · 2017-03-27T03:42:00Z

app/models/stat/anime_genre_breakdown.rb

+        save
+      else
+        update_attribute(:stats_data, stats_data)
+      end


Wouldn't save work for both of these cases?

Also we might want save! which'll throw an error if the save fails (otherwise this might break silently)

hmm, now that I think about it I don't know why I actually split it.

NuckChorris · 2017-03-27T03:44:29Z

app/models/stat/anime_genre_breakdown.rb

+
+        record.stats_data[genre.slug] -= 1
+        record.stats_data['total'] -= 1
+      end


You might be able to combine increment and decrement into an update_genres(genres, -1) method (where the second parameter is either -1 or +1 depending on whether you wanna decrement or increment. Just something to think about

I originally had it with an update_genres and was thinking about going with the (+1 -1, we talked about it) but decided that splitting them up would be best for consistency across all the Stats.

houndci-bot · 2017-03-27T04:34:05Z

app/policies/stat/anime_genre_breakdown_policy.rb

@@ -0,0 +1 @@
+class Stat::AnimeGenreBreakdownPolicy < StatPolicy; end


Use nested module/class definitions instead of compact style.

toyhammered · 2017-03-28T03:14:47Z

app/models/stat/anime_amount_watched.rb

+    end
+
+    def progress_to_time(le, progress)
+      return 0 if le.anime.episode_length.nil?


I just assumed we are skipping if there is no episode_length?

Seems reasonable tbh

toyhammered · 2017-03-28T03:17:29Z

app/resources/stat_resource.rb

@@ -0,0 +1,13 @@
+class StatResource < BaseResource


should this be immutable, now that I think about it recalculate! is run with a rake task and both increment + decrement are just inside the LibraryEntry.

Actually, yes, that's a good point!

toyhammered · 2017-03-28T03:18:15Z

app/services/regenerate_stat_service.rb

+class RegenerateStatService
+  class << self
+    def anime_genre_breakdown
+      User.where(id: LibraryEntry.select(:user_id).where(media_type: 'Anime'))


I added the .where(media-type: 'Anime') because no point running it if they only have manga library entries.

Makes sense

toyhammered · 2017-03-28T03:19:24Z

db/migrate/20170326222205_add_time_spent_to_library_entry.rb

+
+    say_with_time 'Filling time_spent column' do
+      LibraryEntry.where(media_type: 'Anime').joins(:anime)
+        .update_in_batches('time_spent = anime.episode_length * progress')


what happens if there is no anime.episode_length because there is no default (or null constraint) so wouldn't it be nil * progress which will cause an error?

I'm pretty sure it'll just replace it with 0 (since this is Postgres math, not Ruby math), that's what it did last night when I was pulling numbers for josh 🤔 But it's possible that order matters, I did progress * episode_length

If you wanna be explicit you could use coalesce(anime.episode_length, 0)

toyhammered · 2017-03-28T03:21:44Z

spec/factories/stats.rb

+  factory :stat do
+    association :user, factory: :user, strategy: :build
+    type 'Stat::AnimeGenreBreakdown'
+    stats_data { { total: 0 } }


I am getting this error if stats_data is just a {}:

Failure/Error: FactoryGirl::Linter.new([factory], :factory).lint! FactoryGirl::InvalidFactoryError: The following factories are invalid: * stat - Validation failed: Stats data can't be blank (ActiveRecord::RecordInvalid)

Is that if you do stats_data {} or is it with stats_data { {} }?

first one is uhh... wrong? I have tried with the second one, and there is a default set already to {} at a database level.

First one is really more "ambiguous" than wrong, it could be either a block or a hash (and both are valid for FactoryGirl).

Also the problem is simple:

{}.blank? #=> true

You can probably drop the validation since the database provides a default

toyhammered · 2017-03-28T03:23:20Z

spec/models/stat_spec.rb

+  subject { described_class.new }
+
+  it { should belong_to(:user) }
+  it { should validate_presence_of(:type) }


I'll add validation for stats_data also

NuckChorris · 2017-03-28T18:44:16Z

app/models/library_entry.rb

+  after_commit(on: :create) do
+    sync_entry(:create) # mal exporter
+    # Stat STI
+    Stat::AnimeGenreBreakdown.increment(user, media.genres)


These probably shouldn't be in after_commit, that'll make them run in a separate transaction which could result in them being out-of-sync with this model. Should be in after_create and after_destroy hooks

well the after_create is creating some janky timing errors with my tests now....

Really? That suggests your tests are wrong, since after_commit is just never run during tests (the commit never actually happens, instead it just runs a rollback after each test)

heh, well I know what is wrong but my easy solution of just doing a before do subject.recalculate! fails it seems...

NuckChorris · 2017-03-28T18:44:25Z

app/models/library_entry.rb

+  after_destroy do
+    sync_entry(:delete) # mal exporter
+    # Stat STI
+    Stat::AnimeGenreBreakdown.decrement(user, media.genres)


NuckChorris · 2017-03-28T18:45:10Z

app/models/stat.rb

+class Stat < ApplicationRecord
+  belongs_to :user, required: true
+  # expose for jsonapi
+  alias_attribute :kind, :type


You don't need this with the STIResource

NuckChorris · 2017-03-28T18:46:43Z

app/models/stat/anime_amount_watched.rb

+
+    def self.decrement(user, library_entry)
+      record = user.stats.find_by(type: 'Stat::AnimeAmountWatched')
+      # TODO: do we want to raise or return?


Just do nothing, raising an exception might break library updates

NuckChorris · 2017-03-28T18:47:02Z

app/models/stat/anime_amount_watched.rb

+    end
+
+    def progress_to_time(le, progress)
+      return 0 if le.anime.episode_length.nil?


Seems reasonable tbh

NuckChorris · 2017-03-28T18:48:45Z

app/resources/stat_resource.rb

@@ -0,0 +1,13 @@
+class StatResource < BaseResource


Actually, yes, that's a good point!

NuckChorris · 2017-03-28T18:49:17Z

app/services/regenerate_stat_service.rb

+class RegenerateStatService
+  class << self
+    def anime_genre_breakdown
+      User.where(id: LibraryEntry.select(:user_id).where(media_type: 'Anime'))


Makes sense

NuckChorris · 2017-03-28T18:50:30Z

db/migrate/20170326222205_add_time_spent_to_library_entry.rb

+
+    say_with_time 'Filling time_spent column' do
+      LibraryEntry.where(media_type: 'Anime').joins(:anime)
+        .update_in_batches('time_spent = anime.episode_length * progress')


I'm pretty sure it'll just replace it with 0 (since this is Postgres math, not Ruby math), that's what it did last night when I was pulling numbers for josh 🤔 But it's possible that order matters, I did progress * episode_length

If you wanna be explicit you could use coalesce(anime.episode_length, 0)

NuckChorris · 2017-03-28T18:51:56Z

spec/factories/stats.rb

+  factory :stat do
+    association :user, factory: :user, strategy: :build
+    type 'Stat::AnimeGenreBreakdown'
+    stats_data { { total: 0 } }


Is that if you do stats_data {} or is it with stats_data { {} }?

NuckChorris · 2017-03-28T18:52:24Z

spec/models/stat_spec.rb

+  subject { described_class.new }
+
+  it { should belong_to(:user) }
+  it { should validate_presence_of(:type) }


NuckChorris · 2017-03-30T02:31:04Z

app/models/library_entry.rb

+
+  after_create do
+    # Stat STI
+    case media_type


I'd prefer if you used kind for this (returns a symbol :anime, :manga, etc.), since media_type is deprecated and slated for eventual removal

Sounds good, there are quite a few other places that I use media_type so should I change them to kind?

Yeah, anywhere you're using media_type from LibraryEntry, change to kind, so we can gradually wean off the polymorphic column

NuckChorris · 2017-03-30T02:36:43Z

app/models/stat/anime_amount_watched.rb

+
+    def default_stats
+      # needs to be a string and NOT a symbol
+      # due to how it is being saved.


I'm pretty sure Rails will handle all the conversions itself?

Also it's probably better for default_stats to just be a DEFAULT_STATS constant that you self.stats_data = DEFAULT_STATS if stats_data.blank?, instead of a method which modifies the instance and returns the self.

When a method has a name like default_stats, you expect that it'll return the default stats. The fact that this method doesn't is a bit of a betrayal of expectations.

Okay! I actually wasn't entirely sure how to approach the default_stats because I felt like a method wasn't right. Thinking about it now, having a constant (frozen) is prob the best method.

NuckChorris · 2017-03-30T02:40:34Z

app/models/stat/anime_genre_breakdown.rb

+      save!
+    end
+
+    def default_stats


See above, re: the other default_stats method

This, again.

oops... I only fixed 1 of them...

NuckChorris · 2017-03-30T02:42:19Z

db/migrate/20170326222205_add_time_spent_to_library_entry.rb

+    add_column :library_entries, :time_spent, :integer
+
+    say_with_time 'Filling time_spent column' do
+      LibraryEntry.where(media_type: 'Anime').joins(:anime)


I would prefer if you used by_kind(:anime) for this

so instead of joins(:anime), use by_kind(:anime)? Can I have a tl;dr for what the difference is?

Also for the media_type: 'Anime' should I change it to .where(kind: :anime)?

Nonono, replace where(media_type: 'Anime') with by_kind(:anime)

As the name implies, by_kind gets Library Entries by kind 😉

NuckChorris · 2017-03-30T02:43:16Z

lib/tasks/user_stats.rake

@@ -0,0 +1,10 @@
+task user_stats: 'kitsu:user_stats_setup'


Is there a task named user_stats_setup, cause I ain't seeing one 🤔

oh I failed at copying the stats.rake :D. it should be user_stats_setup (like 2 lines down)

uhhhh why are you copying stats.rake? That's literally just a thing to configure how we count lines of code with rake stats (try running it, it'll tell you the lines of code in Kitsu!)

It shouldn't be named user_stats_setup unless it's doing some from of configuration setup (which it isn't)

NuckChorris · 2017-03-30T02:43:52Z

lib/tasks/user_stats.rake

+task user_stats: 'kitsu:user_stats_setup'
+
+namespace :kitsu do
+  task :user_stats do


You probably want to use the environment dependency so that all of Rails will load for this (otherwise it probably won't be able to access any Rails models)

Can I get an English translation of this please?

TL;DR go look at kitsu.rake

houndci-bot · 2017-04-01T05:16:34Z

lib/tasks/user_stats.rake

@@ -0,0 +1,8 @@
+namespace :kitsu do
+  task :user_stats => :environment do


Use the new Ruby 1.9 hash syntax. (https://github.com/bbatsov/ruby-style-guide#hash-literals)

assuming it just wants user_stats: :environment?

houndci-bot · 2017-04-01T05:16:34Z

app/models/stat/anime_amount_watched.rb

+class Stat
+  class AnimeAmountWatched < Stat
+    DEFAULT_STATS = {
+        'total_anime' => 0,


Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

houndci-bot · 2017-04-01T05:16:34Z

app/models/stat/anime_amount_watched.rb

@@ -0,0 +1,66 @@
+class Stat
+  class AnimeAmountWatched < Stat
+    DEFAULT_STATS = {


Freeze mutable objects assigned to constants.

tis ain't possible houndy :(.

Why isn't that possible? It looks possible to me :v

houndci-bot · 2017-04-01T05:16:34Z

app/models/library_entry.rb

+    when :anime
+      Stat::AnimeGenreBreakdown.decrement(user, media.genres)
+      Stat::AnimeAmountWatched.decrement(user, self)
+    when :manga


Avoid when branches without a body.

houndci-bot · 2017-04-01T05:16:34Z

app/models/library_entry.rb

+    when :anime
+      Stat::AnimeGenreBreakdown.increment(user, media.genres)
+      Stat::AnimeAmountWatched.increment(user, self)
+    when :manga


Avoid when branches without a body.

houndci-bot · 2017-04-04T00:35:15Z

app/resources/user_resource.rb

@@ -2,7 +2,7 @@ class UserResource < BaseResource
  PRIVATE_FIELDS = %i[email password confirmed previous_email language time_zone
                      country share_to_global title_language_preference
                      sfw_filter rating_system].freeze
-
+  


Trailing whitespace detected. (https://github.com/bbatsov/ruby-style-guide#no-trailing-whitespace)

houndci-bot · 2017-04-04T00:35:15Z

app/resources/user_resource.rb

@@ -2,7 +2,7 @@ class UserResource < BaseResource
  PRIVATE_FIELDS = %i[email password confirmed previous_email language time_zone
                      country share_to_global title_language_preference
                      sfw_filter rating_system].freeze
-
+  


Trailing whitespace detected. (https://github.com/bbatsov/ruby-style-guide#no-trailing-whitespace)

NuckChorris

Great progress, I like where this is going!

NuckChorris · 2017-04-04T20:09:27Z

app/models/stat/anime_amount_watched.rb

@@ -0,0 +1,66 @@
+class Stat
+  class AnimeAmountWatched < Stat
+    DEFAULT_STATS = {


Why isn't that possible? It looks possible to me :v

NuckChorris · 2017-04-04T21:34:08Z

app/models/stat/anime_genre_breakdown.rb

+      save!
+    end
+
+    def default_stats


This, again.

NuckChorris · 2017-04-04T21:35:29Z

db/schema.rb


+ActiveRecord::Schema.define(version: 20170326222205) do


I think this might've gotten messed up in your merge, but I think the blank line is supposed to be after the ActiveRecord::Schema.define line 🤔

(not a big deal tho, it'll get fixed next time rails dumps the schema)

houndci-bot · 2017-04-04T22:07:42Z

app/models/stat/anime_amount_watched.rb

+      record = user.stats.find_or_initialize_by(
+        type: 'Stat::AnimeAmountWatched'
+      )
+      record.stats_data['all_time'] =


Favor a normal if-statement over a modifier clause in a multiline statement. (https://github.com/bbatsov/ruby-style-guide#no-multiline-if-modifiers)

houndci-bot reviewed Mar 19, 2017

View reviewed changes

toyhammered commented Mar 20, 2017

View reviewed changes

toyhammered commented Mar 26, 2017

View reviewed changes

NuckChorris reviewed Mar 27, 2017

View reviewed changes

houndci-bot reviewed Mar 27, 2017

View reviewed changes

toyhammered force-pushed the user-stats branch from 258f06c to d3453b6 Compare March 28, 2017 03:07

toyhammered commented Mar 28, 2017

View reviewed changes

NuckChorris reviewed Mar 28, 2017

View reviewed changes

NuckChorris reviewed Mar 30, 2017

View reviewed changes

toyhammered force-pushed the user-stats branch from 2ad4549 to cbb3498 Compare April 1, 2017 05:16

houndci-bot reviewed Apr 1, 2017

View reviewed changes

toyhammered force-pushed the user-stats branch from cbb3498 to 3d34c42 Compare April 1, 2017 05:44

houndci-bot reviewed Apr 4, 2017

View reviewed changes

toyhammered added 9 commits April 3, 2017 18:05

add base structure for incorperating stats into user

47bfa41

add recalculate, still need to write tests

40e771e

refactor AnimeGenreBreakdown to be more efficient

f65f7bf

add regenerate stat service + anime_genre_breakdown

7efbbf3

add tests + guard clauses

9eafdde

add rake task for user stats

0b79076

add presence validation for stats_data

cf3713c

add stats_data with value to factory

c54b6a3

add time_spent to library_entries

403257b

toyhammered added 11 commits April 3, 2017 18:05

add save_record method to Stat class

4fca75a

update AnimeGenreBreakdown to use increment and decrement methods

c891d87

add after_commit blocks and merge code together

a8044c5

add anime amount watched stats

8d5ae61

change type for anime_amount_watched to be correct

5e2abed

change query for finding record in AnimeAmountWatched

b25bd48

fix AnimeGenreBreakdown

ac8d866

use STIResource for kind attribute

700a3da

add time_spent to factory + fix tests

5d8ddbb

merge nuck's review commits together

2e6b118

fix rubocop errors

f79de39

toyhammered force-pushed the user-stats branch from 1440f85 to f79de39 Compare April 4, 2017 01:06

remove duplicate rating from LibraryEntries factory

7a3875d

NuckChorris reviewed Apr 4, 2017

View reviewed changes

add policy and refactor code

3ad8615

houndci-bot reviewed Apr 4, 2017

View reviewed changes

toyhammered added 2 commits April 5, 2017 00:10

Merge branch 'the-future' into user-stats

c360001

fix rubocop error

4026ca8

NuckChorris approved these changes Apr 6, 2017

View reviewed changes

NuckChorris merged commit 8117608 into the-future Apr 6, 2017

wopian mentioned this pull request Apr 18, 2017

User Stats wopian/hibari#93

Closed

20 tasks

NuckChorris deleted the user-stats branch May 2, 2017 05:03


		# update data with + 1
		def library_entry_update(library_entry)

		@@ -0,0 +1 @@
		class Stat::AnimeGenreBreakdownPolicy < StatPolicy; end

		@@ -0,0 +1,8 @@
		namespace :kitsu do
		task :user_stats => :environment do

User Stats #77

User Stats #77

Conversation

toyhammered commented Mar 18, 2017

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

toyhammered Mar 28, 2017 • edited

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

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

toyhammered Mar 28, 2017 •

edited