Followable #1727

taitus · 2017-07-05T17:47:41Z

Where

Related PR's: A6. Proposals/BudgetInvestments follow button rockandror/consuldemocracy#6

What

Allow logged users to follow/unfollow proposals or budgets investments
Show in the user profile their interests according to the proposals or budgets investments they are following.
User can mark his interests as public.
~~Show current followers at proposal and budget investment pages.~~
Followers of a proposal or budget investment will receive related notifications.

How

Create polymorphic association on model concern “Followable” and adding to Proposal and Budget investments.
Adding check “public_interest” to user account.
Adding interests to user profile.
~~Adding new tab to proposals and budget investment for display Followers and notifications~~
Create new reusable partials to display followable buttons, followable followers

Screenshots

1. Follow button - Mobile

2. Follow button - Tablet

3. Follow button - Desktop

4. Public interest check on user account

5. Interest list on user profile

Test

Adding Followable shared feature specs, and apply to Proposals and Budget Investment
Increased the user spec feature to display public interest
Increased the notification and proposal notification spec feature to check notification sending to new followers

Deployment

Run migrations

Warnings

None

bertocq · 2017-07-05T20:44:58Z

db/schema.rb

@@ -95,9 +95,8 @@
  create_table "budget_ballots", force: :cascade do |t|
    t.integer  "user_id"
    t.integer  "budget_id"
-    t.datetime "created_at",                     null: false
-    t.datetime "updated_at",                     null: false
-    t.integer  "ballot_lines_count", default: 0


This is 👍 , I messed up somehow on a PR and didn't realize it. I added this on a feature branch that is still on PR stage https://github.com/consul/consul/pull/1712/files#diff-ddcb0cf5e4dfc6ac0c06b85bac771760 My apologies >_< 🙇

Senen · 2017-07-06T13:41:15Z

app/controllers/account_controller.rb

@@ -25,8 +25,8 @@ def account_params
      if @account.organization?
        params.require(:account).permit(:phone_number, :email_on_comment, :email_on_comment_reply, :newsletter, organization_attributes: [:name, :responsible_name])
      else
-        params.require(:account).permit(:username, :public_activity, :email_on_comment, :email_on_comment_reply, :email_on_direct_message, :email_digest, :newsletter, :official_position_badge)
+        params.require(:account).permit(:username, :public_activity, :public_interest, :email_on_comment, :email_on_comment_reply, :email_on_direct_message, :email_digest, :newsletter, :official_position_badge)


@bertocq maybe public_interest should be public_interests?

+1 Yes it sounds more descriptive, right? Feature is "User can mark his interests as public." so plural makes sense.

Senen · 2017-07-06T13:44:36Z

app/models/follow.rb

@@ -0,0 +1,31 @@
+class Follow < ActiveRecord::Base
+  belongs_to :user
+  #TODO Rock&RoR: Check touch usage on cache system


@taitus We have to do the check, fix if necessary and remove the comment.

bertocq · 2017-07-06T14:14:57Z

app/models/follow.rb

@@ -0,0 +1,31 @@
+class Follow < ActiveRecord::Base
+  belongs_to :user


I think a cool improvement here (even if there is no actual use on the code right now) would have been to use a counter_cache

Something like belongs_to :user, counter_cache: :following_count

Without forgetting the migration:

add_column :users, :following_count, :integer, default: 0 add_index :users, :following_count

Don't worry about adding this, seems like there is no actual usage not even reason to. On the mockups there is nowhere to use it (only individual Followable counts) so when that time comes we'll add it if @decabeza thinks it makes sense to put a global following count 👍 .

Because of that we do not have implemented it. Yes, maybe is a YAGNI thing for now.

bertocq · 2017-07-06T14:22:23Z

app/models/follow.rb

+class Follow < ActiveRecord::Base
+  belongs_to :user
+  #TODO Rock&RoR: Check touch usage on cache system
+  belongs_to :followable, polymorphic: true


I think here the counter_cache usage I suggested for the belongs_to :user would actually be used, so its more like a performance improvement for any @investment.follows.count or @proposal.follows.count around. What do you think about naming it "followers_count" twitter-style? 😄

bertocq · 2017-07-06T14:53:14Z

app/models/follow.rb

+    !! by_user_and_followable(user, followable).try(:first)
+  end
+
+  def self.interests_by(user)


What do you think about using a symbol proc and { |item| .. } syntax to make function body smaller? It would be something like user.follows.map { |follow| follow.followable.tags.map(&:name) }.flatten.compact.uniq (the entire function body)

As we commented, this method could make more sense in the User model, to be used as user.interests

bertocq · 2017-07-06T15:11:17Z

app/views/follows/_followers.html.erb

+      <div class="callout primary text-center">
+        <%= t('follows.no_followers') %>
+      </div>
+    <% end %>


Adding here an else and including inside it the following .each loop could maybe be safer?

bertocq · 2017-07-06T15:13:56Z

app/models/proposal.rb

@@ -170,6 +171,10 @@ def notifications
    proposal_notifications
  end

+  def users_to_notify


Maybe increasing proposal model spec to check this new method, just reusing existing spec for voters could be a nice test improvement https://github.com/rockandror/consul/blob/followable/spec/models/proposal_spec.rb#L382-L425

bertocq · 2017-07-06T15:24:07Z

spec/factories.rb

@@ -166,8 +166,8 @@
    end

    trait :flagged do
-      after :create do |debate|


Nice catch! 👍

bertocq · 2017-07-06T15:27:06Z

spec/shared/features/followable.rb

@@ -0,0 +1,182 @@
+shared_examples "followable" do |followable_class_name, followable_path, followable_path_arguments|


Really like the usage of shared_examples 👏 😊

bertocq · 2017-07-06T15:30:03Z

spec/spec_helper.rb

@@ -5,6 +5,7 @@
 require 'knapsack'
 Dir["./spec/models/concerns/*.rb"].each { |f| require f }
 Dir["./spec/support/**/*.rb"].sort.each { |f| require f }
+Dir["./spec/shared/**/*.rb"].sort.each { |f| require f }


Seems like we have 4 shared_examples for concerns, but they are across 3 different subfolder 🙈 . I'll correct this afterwards don't worry :). IMHO /spec/models/concerns/ should be the path for a concern shared_example

…odel.

…wable entity.

…artial.

…d proposals pages when user is not logged.

…reading.

…ements renderization. Add whitespace to proposal page.

…sts. Add specification to check the discard of duplicate interests.

bertocq

👏 !!

…sign UI design

bertocq reviewed Jul 5, 2017

View reviewed changes

Senen force-pushed the followable branch from e817a1d to ffc2f14 Compare July 6, 2017 13:36

Senen reviewed Jul 6, 2017

View reviewed changes

bertocq reviewed Jul 6, 2017

View reviewed changes

spec/factories.rb

@@ -166,8 +166,8 @@

end

trait :flagged do

after :create do |debate|

Copy link

Collaborator

bertocq Jul 6, 2017

Choose a reason for hiding this comment

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

Nice catch! 👍

bertocq reviewed Jul 6, 2017

View reviewed changes

Senen and others added 16 commits July 7, 2017 12:02

Create followable concern, follow model. Add followable to proposal m…

84dbef1

…odel.

Add followable to budget investment model.

bce28af

Add public interests validation and show interests on user profile

19820e4

Create rspec shared examples to test followable features on any follo…

1f22286

…wable entity.

Add followable JS module. Refactor follow_helper and follow buttons p…

313b789

…artial.

Add styles and translations for users interests.

14ec4f5

Add dependent destroy to followable.

35f7f11

Send proposals notifications to followers.

b29bf62

Fix scss-lint

0ffa213

Do not show flaggable and followable buttons on budgets_investment an…

97ee107

…d proposals pages when user is not logged.

Remove unneeded return from Follow model. Add whitespaces to enhance …

b60748c

…reading.

Refactor js followable response.

0c2be6a

New proposal method to get users_to_notify.

5fe894a

Use display in followable shared specs names when referring to DOM el…

e54e33f

…ements renderization. Add whitespace to proposal page.

New texts and titles for followable buttons.

60e812c

Initialize App js modules after followable update.

a7aef0a

Senen force-pushed the followable branch 2 times, most recently from c21cb0a to f5e8789 Compare July 7, 2017 12:40

Move and refactor method from follow to user model to get user intere…

4b2310c

…sts. Add specification to check the discard of duplicate interests.

Add missing proposal model specs to check new users_to_notify method.

112e064

Senen force-pushed the followable branch from f5e8789 to 112e064 Compare July 7, 2017 13:18

bertocq approved these changes Jul 7, 2017

View reviewed changes

Senen and others added 5 commits July 10, 2017 12:46

Add comment to temporary bugfix.

95630d6

Set button style to followable button

58b1ee5

Move followable button to sidebar after social share buttons

503e079

Remove comment

b0ee525

Merge branch 'master' into followable

204ea13

voodoorai2000 approved these changes Jul 11, 2017

View reviewed changes

bertocq merged commit 9c4eb4c into consuldemocracy:master Jul 11, 2017

bertocq mentioned this pull request Jul 11, 2017

Consul changes pull AyuntamientoMadrid/consul#848

Merged

Senen mentioned this pull request Jul 12, 2017

A6. Proposals/BudgetInvestments follow button rockandror/consuldemocracy#6

Closed

15 tasks

javierm pushed a commit to javierm/consul that referenced this pull request Dec 7, 2018

Merge pull request consuldemocracy#1727 from AyuntamientoMadrid/ui-de…

9303afd

…sign UI design

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Followable #1727

Followable #1727

taitus commented Jul 5, 2017 •

edited

bertocq Jul 5, 2017

Senen Jul 6, 2017

bertocq Jul 6, 2017

Senen Jul 6, 2017

bertocq Jul 6, 2017

bertocq Jul 6, 2017

Senen Jul 6, 2017

bertocq Jul 6, 2017

bertocq Jul 6, 2017 •

edited

bertocq Jul 6, 2017

bertocq Jul 6, 2017

bertocq Jul 6, 2017

bertocq Jul 6, 2017

bertocq Jul 6, 2017

bertocq Jul 6, 2017

bertocq left a comment

		@@ -0,0 +1,31 @@
		class Follow < ActiveRecord::Base
		belongs_to :user

		@@ -0,0 +1,182 @@
		shared_examples "followable" do \|followable_class_name, followable_path, followable_path_arguments\|

Followable #1727

Followable #1727

Conversation

taitus commented Jul 5, 2017 • edited

Where

What

How

Screenshots

Test

Deployment

Warnings

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

bertocq Jul 6, 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

bertocq left a comment

Choose a reason for hiding this comment

taitus commented Jul 5, 2017 •

edited

bertocq Jul 6, 2017 •

edited