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

Update _index.html.haml #4245

Closed
wants to merge 4 commits into from

Conversation

fabianfiorotto
Copy link
Contributor

This params broken the pagination of the followers list in tags page
To reproduce try to paginate in
https://joindiaspora.com/tags/diaspora

This params broken the pagination of the followers list in tags page
To reproduce try to paginate in 
https://joindiaspora.com/tags/diaspora
@jhass
Copy link
Member

jhass commented Jun 20, 2013

We need a test that fails without this change :)

@fabianfiorotto
Copy link
Contributor Author

I don't know how to use jasmine to tests url requests. Can you help me to write this?
This url is returning a 406 error:

url = $('.pagination a').first().attr('href');

and after my change will return status 200

@Raven24
Copy link
Member

Raven24 commented Jun 22, 2013

This should be a sufficient test, IMHO:

it could go into spec/integration/tag_people_spec.rb (or something else)

require 'spec_helper'

describe TagsController, type: :controller do
  describe 'will_paginate people on the tag page' do
    let(:people) { (1..5).map { FactoryGirl.create(:person) } }
    let(:tag)    { "diaspora" }

    before do
      Stream::Tag.any_instance.stub(people_per_page: 4)
      Person.should_receive(:profile_tagged_with).with(/#{tag}/).twice.and_return(people)
    end

    it 'paginates the people set' do
      get "/tags/#{tag}"

      expect(response.status).to eq(200)
      response.body.should match(/div class="pagination"/)
      response.body.should match(/href="\/tags\/#{tag}\?page=2"/)
    end

    it 'fetches the second page' do
      get "/tags/#{tag}", page: 2

      expect(response.status).to eq(200)
      response.body.should match(/<em class="current">2<\/em>/)
    end
  end
end

and you'd need to include this, to make Stream::Tag work with that test scenario

diff --git a/lib/stream/tag.rb b/lib/stream/tag.rb
index a1a096b..75f40f9 100644
--- a/lib/stream/tag.rb
+++ b/lib/stream/tag.rb
@@ -3,11 +3,12 @@
 #   the COPYRIGHT file.

 class Stream::Tag < Stream::Base
-  attr_accessor :tag_name, :people_page
+  attr_accessor :tag_name, :people_page, :people_per_page

   def initialize(user, tag_name, opts={})
     self.tag_name = tag_name
     self.people_page = opts[:page] || 1
+    self.people_per_page = 15
     super(user, opts)
   end

@@ -24,7 +25,7 @@ class Stream::Tag < Stream::Base
   end

   def tagged_people
-    @people ||= ::Person.profile_tagged_with(tag_name).paginate(:page => people_page, :per_page => 15)
+    @people ||= ::Person.profile_tagged_with(tag_name).paginate(:page => people_page, :per_page => people_per_page)
   end

   def tagged_people_count

@fabianfiorotto
Copy link
Contributor Author

Thanks!! I think is not a good idea modify the controller. More changes would mean generating new bugs. I will keep it simple. You can add new functionality later.


describe TagsController, type: :controller do
describe 'will_paginate people on the tag page' do
let(:people) { (1..16).map { FactoryGirl.create(:person) } }
Copy link
Member

Choose a reason for hiding this comment

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

I'm not completely convinced... this is actually creating 16 records in the db, so this might take unnecessarily longer than it could.
third opinion anybody?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, just reduce the per_page limit. also range#each if you don't care about the return value and n.times if you don't even care about the iterated value.

@jhass
Copy link
Member

jhass commented Jul 6, 2013

Thank you! I squashed the commits and added a changelog entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants