Issue#669 bad page number parameters causing ArgumentError

[hisayohorie]

Fixes #669.

I wrote 2 test for this method because
when the string passed starts with integer, to_i returns the same integer (ex. "2K%&)".to_i = 2)
and other time it returns 0 (ex. "%&43{".to_i = 0)

and I'm not super sure this is the appropriate way to do it? please let me know!
@equivalentideas @henare

[henare]

This is a very interesting approach @hisayohorie and does seem to do the job. However since the error is bubbling up from the will_paginate gem I wonder if it's not something that should be fixed in the Gem itself?

I'm thinking that surely we're not the only ones affected by this error too. Can you please have a look and see if that's right and see how other people are fixing this? I'd check the Gem's issues page first 👍

[hisayohorie]

@henare sure! I will look into it!

[hisayohorie]

@henare this is the discussion around the issue, and seems the best practice isn't solidified?
mislav/will_paginate#271

[henare]

@hisayohorie nice one on tracking down that discussion. That makes me 👍 this solution then.

I think we should add a comment above that method so people know why we've added it. You could even include a link to that discussion. Presumably when it's resolved upstream we'll be able to remove this workaround ✨

Once that's done let's merge and

[henare]

@hisayohorie I noticed you updated this so I thought I'd give it a more thorough check.

Once that's done let's merge and

OK I got a bit ahead of myself 😄 I'll add a couple of comments 💬

[henare]

Do not use unless with else. Rewrite these with the positive case first.

I also notice your indentation is a little out on the params lines - you need an extra space.

[henare]

In my first review I didn't check where these tests were placed. They're under #index rss feed and since this isn't related to the RSS feed these tests should not be here.

But where should they go? I didn't know so I hunted around. I discovered that ApplicationController behaviour that does things like global error checking should be tested by defining an anonymous controller.

So create a new file in this directory called application_controller_spec.rb, with something like this (and removing the tests from this file):

require "spec_helper"

describe ApplicationController do
  controller do
    def index
      render text: nil
    end
  end

  describe "#validate_page_param" do
    it "should try to convert page param to an integer" do
      get :index, page: "2%5B&q"

      expect(controller.params[:page]).to eq(2)
    end

    it "should default to page nil when no page number param is given" do
      get :index, page: "%5B&q"

      expect(controller.params[:page]).to eq(nil)
    end
  end
end

[hisayohorie]

will do!

[henare]

@hisayohorie Once you're ready for me to take another look please assign back to me 🤝

[henare]

I think this is ready to go 👍

[henare]

Remove this stray extra line.

[henare]

Deployed