From 0fa24ac4b73f0ec5ffe18cf9308fea31509ba950 Mon Sep 17 00:00:00 2001 From: davegson <3080765+davegson@users.noreply.github.com> Date: Fri, 3 May 2019 13:40:45 +0200 Subject: [PATCH 1/6] Rename incorrect spec file --- spec/models/{activities_spec.rb => activity_spec.rb} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename spec/models/{activities_spec.rb => activity_spec.rb} (100%) diff --git a/spec/models/activities_spec.rb b/spec/models/activity_spec.rb similarity index 100% rename from spec/models/activities_spec.rb rename to spec/models/activity_spec.rb From 01fc2ad1923833f75ba19385feb34a1dfbcacddd Mon Sep 17 00:00:00 2001 From: davegson <3080765+davegson@users.noreply.github.com> Date: Fri, 3 May 2019 16:25:29 +0200 Subject: [PATCH 2/6] Fix rubocops --- app/channels/application_cable/connection.rb | 2 +- app/models/notification.rb | 2 +- cable/config.ru | 2 +- spec/factories/comments.rb | 2 +- spec/factories/stamps.rb | 2 +- spec/models/comment_spec.rb | 2 ++ spec/models/notification_spec.rb | 4 ++-- spec/views/semantic/header.html.haml_spec.rb | 1 - spec/workers/notification_broadcast_worker_spec.rb | 2 +- 9 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/channels/application_cable/connection.rb b/app/channels/application_cable/connection.rb index 251cbd8..8e8a8b7 100644 --- a/app/channels/application_cable/connection.rb +++ b/app/channels/application_cable/connection.rb @@ -11,7 +11,7 @@ def connect protected def find_verified_user - if verified_user = env['warden'].user + if (verified_user = env['warden'].user) verified_user else reject_unauthorized_connection diff --git a/app/models/notification.rb b/app/models/notification.rb index 64297e0..6e08b45 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -4,7 +4,7 @@ class Notification < ApplicationRecord belongs_to :recipient, class_name: 'User' belongs_to :reference, polymorphic: true - after_create { NotificationBroadcastWorker.perform_async(self.id) } + after_create { NotificationBroadcastWorker.perform_async(id) } validates_presence_of %i[activity actor recipient reference] diff --git a/cable/config.ru b/cable/config.ru index 341e724..e049ff1 100644 --- a/cable/config.ru +++ b/cable/config.ru @@ -1,4 +1,4 @@ -require ::File.expand_path('../../config/environment', __FILE__) +require ::File.expand_path('../../config/environment', __FILE__) Rails.application.eager_load! run ActionCable.server diff --git a/spec/factories/comments.rb b/spec/factories/comments.rb index 7fbf122..a2d1395 100644 --- a/spec/factories/comments.rb +++ b/spec/factories/comments.rb @@ -6,7 +6,7 @@ association :commentable, factory: :label_stamp end - # meaning: the comment & commentable were created by the same user + # meaning: the comment & commentable were created by the same user factory :initial_comment, parent: :comment do commentable { create(:label_stamp, creator: user) } end diff --git a/spec/factories/stamps.rb b/spec/factories/stamps.rb index 2ee4b95..51b8298 100644 --- a/spec/factories/stamps.rb +++ b/spec/factories/stamps.rb @@ -26,7 +26,7 @@ end trait :with_comments do - after(:create) do |stamp, evaluator| + after(:create) do |stamp, _| stamp.comments << FactoryBot.build(:comment, commentable: stamp, user_id: stamp.user_id) stamp.comments << FactoryBot.build_list(:comment, 2, commentable: stamp) stamp.save diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index e9e4e4e..39336f7 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -60,7 +60,9 @@ expect { subject }.to change { Notification.count }.by(2) end + # rubocop:disable LineLength it 'creates notifications {activity: self, actor: actor, recipient: other commenters, reference: stamp}' do + # rubocop:enable LineLength subject activity = PublicActivity::Activity.last diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 1744f39..752d289 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -21,7 +21,7 @@ it { is_expected.to have_db_index(:activity_id) } it { is_expected.to have_db_index(:actor_id) } it { is_expected.to have_db_index(:recipient_id) } - it { is_expected.to have_db_index([:reference_type, :reference_id]) } + it { is_expected.to have_db_index(%i[reference_type reference_id]) } end describe '#create' do @@ -37,7 +37,7 @@ subject { notification.actor } context 'actor_id is -1' do - let(:notification) { FactoryBot.create(:notification, :system_actor) } + let(:notification) { FactoryBot.create(:notification, :system_actor) } it 'returns the System object' do expect(subject).to eq(System.new) diff --git a/spec/views/semantic/header.html.haml_spec.rb b/spec/views/semantic/header.html.haml_spec.rb index b3458f9..af7c6f1 100644 --- a/spec/views/semantic/header.html.haml_spec.rb +++ b/spec/views/semantic/header.html.haml_spec.rb @@ -13,7 +13,6 @@ def rendered context 'user has notifications' do let(:user) { FactoryBot.create(:user, :with_notifications) } - it 'shows the notification inbox as a purple icon' do expect(rendered).to have_css('.inbox.purple.large.icon') end diff --git a/spec/workers/notification_broadcast_worker_spec.rb b/spec/workers/notification_broadcast_worker_spec.rb index 4e2a908..ca2bd97 100644 --- a/spec/workers/notification_broadcast_worker_spec.rb +++ b/spec/workers/notification_broadcast_worker_spec.rb @@ -2,7 +2,7 @@ describe '#perform' do subject { worker.perform(notification.id) } - let(:worker) { NotificationBroadcastWorker.new() } + let(:worker) { NotificationBroadcastWorker.new } let(:notification) { FactoryBot.create(:notification) } it 'calls NotificationsChannel.broadcast_to' do From 3507dabe09a8d99670360a6c7d7c31a1c607568c Mon Sep 17 00:00:00 2001 From: davegson <3080765+davegson@users.noreply.github.com> Date: Fri, 3 May 2019 16:28:15 +0200 Subject: [PATCH 3/6] Implement #stampable_name for stamps the thought behind this is to be able to call the same method in the UI for different stamps. Another usecase is activities caching, which does not care about the stamp type, but does want to save the descriptor/stampable_name --- app/models/stamp.rb | 7 ++++++- app/models/stamp/flag.rb | 4 ++++ app/models/stamp/label.rb | 4 ++++ app/views/activities/_comment.create.html.haml | 2 +- app/views/activities/_vote.create.html.haml | 2 +- app/views/notifications/_notification.html.haml | 2 +- 6 files changed, 17 insertions(+), 4 deletions(-) diff --git a/app/models/stamp.rb b/app/models/stamp.rb index e3675ec..5ca84c4 100644 --- a/app/models/stamp.rb +++ b/app/models/stamp.rb @@ -27,7 +27,12 @@ def peers? end # must be implemented @ each subclass - # can describe a stronger connection than peers + # -> used to describe the stampable object in the UI + def stampable_name + raise NotImplementedError + end + + # -> can describe a stronger connection than peers def siblings raise NotImplementedError end diff --git a/app/models/stamp/flag.rb b/app/models/stamp/flag.rb index f63862c..82e3daa 100644 --- a/app/models/stamp/flag.rb +++ b/app/models/stamp/flag.rb @@ -12,6 +12,10 @@ def app stampable end + def stampable_name + app.name + end + def siblings peers end diff --git a/app/models/stamp/label.rb b/app/models/stamp/label.rb index de44d08..f76fc94 100644 --- a/app/models/stamp/label.rb +++ b/app/models/stamp/label.rb @@ -14,6 +14,10 @@ def domain stampable end + def stampable_name + domain.name + end + def initial_stamp_cannot_be_0 return true if percentage != 0 return true if siblings.accepted.present? diff --git a/app/views/activities/_comment.create.html.haml b/app/views/activities/_comment.create.html.haml index 2855de0..2e35841 100644 --- a/app/views/activities/_comment.create.html.haml +++ b/app/views/activities/_comment.create.html.haml @@ -1,4 +1,4 @@ commented on a = link_to activity.recipient_type, activity.recipient of -= link_to activity.recipient.stampable.name, activity.recipient.stampable += link_to activity.recipient.stampable_name, activity.recipient.stampable diff --git a/app/views/activities/_vote.create.html.haml b/app/views/activities/_vote.create.html.haml index 5782d3c..89b68ba 100644 --- a/app/views/activities/_vote.create.html.haml +++ b/app/views/activities/_vote.create.html.haml @@ -1,6 +1,6 @@ voted for a = link_to activity.recipient_type, activity.recipient of -= link_to activity.recipient.stampable.name, activity.recipient.stampable += link_to activity.recipient.stampable_name, activity.recipient.stampable its = activity.recipient.state diff --git a/app/views/notifications/_notification.html.haml b/app/views/notifications/_notification.html.haml index 7973b7a..1fd623c 100644 --- a/app/views/notifications/_notification.html.haml +++ b/app/views/notifications/_notification.html.haml @@ -6,4 +6,4 @@ = render partial: "notifications/#{notification.activity.key}", locals: { notification: notification } = link_to notification.reference_type, notification.reference of - = link_to notification.reference.stampable.name, notification.reference.stampable + = link_to notification.reference.stampable_name, notification.reference.stampable From cca4d4aa352991947f53f85c16017774cabe4af4 Mon Sep 17 00:00:00 2001 From: davegson <3080765+davegson@users.noreply.github.com> Date: Fri, 3 May 2019 16:41:21 +0200 Subject: [PATCH 4/6] Create page for System user --- app/controllers/users_controller.rb | 2 +- app/models/system.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 3b9390c..3431203 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,6 +1,6 @@ class UsersController < ApplicationController def show - @user = User.find(params[:id]) + @user = params[:id].to_i == -1 ? System.new : User.find(params[:id]) end def index diff --git a/app/models/system.rb b/app/models/system.rb index 3e2337b..4c907ce 100644 --- a/app/models/system.rb +++ b/app/models/system.rb @@ -9,4 +9,12 @@ class System < User def self.polymorphic_name 'System' end + + def created_at + '01-01-2019'.to_date + end + + def updated_at + created_at + end end From 17b93fb6c6bb7249897260ec30b619e1b2b6e037 Mon Sep 17 00:00:00 2001 From: davegson <3080765+davegson@users.noreply.github.com> Date: Wed, 15 May 2019 13:41:11 +0200 Subject: [PATCH 5/6] Add caching to activities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1) the different caches are set up in Activity::CACHE_CONFIG 2) you can directly go ahead and use a cache, if a object has not saved the cache it will load and save it the first time it is called. The second time it already only calls the cache 🎉 So this can be deployed to production or caches can later be added without having to worry --- app/models/activity.rb | 48 ++++++++++++ app/models/comment.rb | 3 + config/initializers/public_activity.rb | 37 +++++++++ spec/factories/activity.rb | 25 ++++-- spec/models/activity_spec.rb | 104 +++++++++++++++++++++++++ 5 files changed, 210 insertions(+), 7 deletions(-) diff --git a/app/models/activity.rb b/app/models/activity.rb index 55147d6..5bcce28 100644 --- a/app/models/activity.rb +++ b/app/models/activity.rb @@ -13,6 +13,54 @@ class Activity vote.create ].freeze + # outer key: defines the key of an activity ('.' replaced with '_') + # => this way each key can cache different data + # inner key: caching_key + # inner value: method(s) to call on an activity (via send) to get the cache value + CACHE_CONFIG = { + app_create: { + app_name: 'trackable.name', + user_username: 'owner.username' + }, + comment_create: { + comment_content: 'trackable.content', + user_username: 'owner.username', + stampable_name: 'recipient.stampable_name' + }, + domain_create: { + domain_name: 'trackable.name', + user_username: 'owner.username' + }, + stamp_accept: { + stampable_name: 'trackable.stampable_name', + stampable_type: 'trackable.type' + }, + stamp_archive: { + stampable_name: 'trackable.stampable_name', + stampable_type: 'trackable.type' + }, + stamp_create: { + stampable_name: 'trackable.stampable_name', + stampable_type: 'trackable.type' + }, + stamp_deny: { + stampable_name: 'trackable.stampable_name', + stampable_type: 'trackable.type' + }, + stamp_dispute: { + stampable_name: 'trackable.stampable_name', + stampable_type: 'trackable.type' + }, + user_signup: { + user_username: 'trackable.username' + }, + vote_create: { + user_username: 'owner.username', + stampable_name: 'recipient.stampable_name', + stampable_type: 'recipient.type' + } + }.freeze + def persisted? false end diff --git a/app/models/comment.rb b/app/models/comment.rb index ce6283c..23ee2b4 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -1,6 +1,9 @@ class Comment < ApplicationRecord include PublicActivity::Common + # :commentable can currently only be used with stamps + # comment.create activities caches data of stamps, so this would fail if used on another object + # in cache config => comment_create => stampable_name is cached belongs_to :commentable, polymorphic: true belongs_to :user diff --git a/config/initializers/public_activity.rb b/config/initializers/public_activity.rb index ce00be5..0ea5abc 100644 --- a/config/initializers/public_activity.rb +++ b/config/initializers/public_activity.rb @@ -3,4 +3,41 @@ has_many :boosts, foreign_key: :trigger_id # a trigger can cause multiple boosts, iE stamp.accept validates :key, presence: true, inclusion: { in: Activity::KEYS } + + # key can only have one dot (.) as defined in Activity::KEYS + def config_key + key.sub('.', '_').to_sym + end + + # if it is not set yet, it will fetch and set the data + # second call already retrieves the cache + # => adding caches in Activity::CACHE_CONFIG is no big deal & performant + def cache(cache_key) + value = parameters[cache_key.to_s] + + return value if value.present? || !cache_key_set?(cache_key) + + set_cache! + cache(cache_key) + end + + def cache_key_set?(cache_key) + Activity::CACHE_CONFIG[config_key][cache_key.to_sym].present? + end + + # if you want to reload all of the caches, use PublicActivity::Activity.all.each(&:set_cache!) + # though it is still performant to just get the caches whenever needed + def set_cache! + Activity::CACHE_CONFIG[config_key]&.each do |cache_key, retrievers| + parameters[cache_key.to_s] = fetch_value(retrievers) + end + + save if changed? + end + + def fetch_value(retrievers) + retrievers.split('.').inject(self) do |cache_value, retriever| + cache_value.send(retriever) + end + end end diff --git a/spec/factories/activity.rb b/spec/factories/activity.rb index 2035f7b..b2cfb4c 100644 --- a/spec/factories/activity.rb +++ b/spec/factories/activity.rb @@ -2,19 +2,23 @@ factory :activity, class: PublicActivity::Activity do association :owner, factory: :user end + + factory :app_activity, parent: :activity do + association :trackable, factory: :app + key { 'app.create' } + end + + factory :comment_activity, parent: :activity do + association :trackable, factory: :comment + recipient { trackable.commentable } + key { 'comment.create' } + end factory :domain_activity, parent: :activity do association :trackable, factory: :domain - key { 'domain.create' } end - factory :app_activity, parent: :activity do - association :trackable, factory: :app - - key { 'app.create' } - end - factory :signup_activity, parent: :activity do association :trackable, factory: :user key { 'user.signup' } @@ -34,6 +38,13 @@ key { 'stamp.accept' } end + factory :user_activity, parent: :activity do + association :trackable, factory: :user + owner { trackable } + recipient { nil } + key { 'user.create' } + end + factory :vote_activity, parent: :activity do transient do vote { create(:vote) } diff --git a/spec/models/activity_spec.rb b/spec/models/activity_spec.rb index 65a323e..d605b06 100644 --- a/spec/models/activity_spec.rb +++ b/spec/models/activity_spec.rb @@ -15,6 +15,110 @@ it { is_expected.to validate_inclusion_of(:key).in_array(Activity::KEYS) } end + describe '#config_key' do + subject { activity.config_key } + let(:activity) { FactoryBot.create(:signup_activity) } + + it 'replaces the key´s . with _' do + expect(subject).to eq(:user_signup) + end + + it 'returns a symbol' do + expect(subject.class).to eq(Symbol) + end + end + + describe '#cache_key_set?(cache_key)' do + subject { activity.cache_key_set?(cache_key) } + let(:activity) { FactoryBot.create(:app_activity) } + + context 'cache_key is configured' do + let(:cache_key) { :app_name } + + it 'returns true' do + expect(subject).to be true + end + end + + context 'cache_key is not configured' do + let(:cache_key) { :unset_key } + + it 'returns false' do + expect(subject).to be false + end + end + end + + describe '#cache(cache_key)' do + subject { activity.cache(cache_key) } + + let(:activity) { FactoryBot.create(:signup_activity, parameters: parameters, trackable: user) } + let(:user) { FactoryBot.create(:user) } + let(:parameters) { {} } + + context 'cache_key is set' do + let(:cache_key) { :user_username } + + context 'cache is already set' do + let(:parameters) { {user_username: user.username} } + + it 'returns the cached value' do + expect(subject).to eq(user.username) + end + end + + context 'cache is not set yet' do + it 'sets the cache' do + expect { subject }.to change { activity.parameters }.from({}).to({'user_username' => user.username}) + end + + it 'recalls #cache(cache_key) to return the value' do + expect(activity).to receive(:cache).twice.with(cache_key).and_call_original + subject + end + end + end + + context 'cache_key is not set' do + let(:cache_key) { :unset_key } + + it 'returns nil' do + expect(subject).to eq nil + end + end + end + + describe '#fetch_value(retrievers)' do + subject { activity.fetch_value(retrievers) } + + describe 'CACHE_CONFIG' do + Activity::CACHE_CONFIG.each do |config_key, config_hash| + model_type = config_key.to_s.split('_').first + factory_key = "#{model_type}_activity".to_sym + activity_key = config_key.to_s.sub('_', '.') + + context "for '#{activity_key}' activity" do + config_hash.each do |cache_key, retrievers| + let(:activity) { FactoryBot.create(factory_key, key: activity_key) } + let(:retrievers) { retrievers } + + describe "cache_key: #{cache_key}" do + describe "#fetch_value('#{retrievers}')" do + it 'does not raise an error' do + expect { subject }.not_to raise_error + end + + it 'does not return nil' do + expect(subject).not_to eq(nil) + end + end + end + end + end + end + end + end + describe 'view partials' do Activity::KEYS.each do |key| it "#{key}.html.haml partial exists" do From bb6ebabc0d648edfab1e17a2a34486a9d0265c00 Mon Sep 17 00:00:00 2001 From: davegson <3080765+davegson@users.noreply.github.com> Date: Wed, 15 May 2019 14:07:37 +0200 Subject: [PATCH 6/6] Fix rubocops --- app/models/concerns/votable/state.rb | 2 ++ spec/factories/activity.rb | 2 +- spec/models/activity_spec.rb | 6 ++++-- spec/models/comment_spec.rb | 2 +- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/models/concerns/votable/state.rb b/app/models/concerns/votable/state.rb index 8c50e90..fe831c2 100644 --- a/app/models/concerns/votable/state.rb +++ b/app/models/concerns/votable/state.rb @@ -2,6 +2,7 @@ module Votable module State extend ActiveSupport::Concern + # rubocop:disable Metrics/BlockLength included do # used so boosts can reference the transition_activity attr_accessor :transition_activity @@ -54,6 +55,7 @@ module State end end end + # rubocop:enable Metrics/BlockLength def scheduled_at created_at + ENVProxy.required_integer('STAMP_CONCLUDE_IN_HOURS').hours diff --git a/spec/factories/activity.rb b/spec/factories/activity.rb index b2cfb4c..91cc922 100644 --- a/spec/factories/activity.rb +++ b/spec/factories/activity.rb @@ -2,7 +2,7 @@ factory :activity, class: PublicActivity::Activity do association :owner, factory: :user end - + factory :app_activity, parent: :activity do association :trackable, factory: :app key { 'app.create' } diff --git a/spec/models/activity_spec.rb b/spec/models/activity_spec.rb index d605b06..b03072f 100644 --- a/spec/models/activity_spec.rb +++ b/spec/models/activity_spec.rb @@ -60,7 +60,7 @@ let(:cache_key) { :user_username } context 'cache is already set' do - let(:parameters) { {user_username: user.username} } + let(:parameters) { { user_username: user.username } } it 'returns the cached value' do expect(subject).to eq(user.username) @@ -69,7 +69,9 @@ context 'cache is not set yet' do it 'sets the cache' do - expect { subject }.to change { activity.parameters }.from({}).to({'user_username' => user.username}) + expect { subject }.to change { activity.parameters }.from({}).to( + 'user_username' => user.username + ) end it 'recalls #cache(cache_key) to return the value' do diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index 39336f7..5246f85 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -62,7 +62,7 @@ # rubocop:disable LineLength it 'creates notifications {activity: self, actor: actor, recipient: other commenters, reference: stamp}' do - # rubocop:enable LineLength + # rubocop:enable LineLength subject activity = PublicActivity::Activity.last