Flesh out handling of Updates

[ghost]

• Provide explicit attribute accessors for the Update type
• Handle "more updates" in a manner consistent with the existing "more projects" functionality
• Add tests to the client spec and a new update spec

[ghost]

These two commits address different but related issues, so I can squash them and open a new pull request if that is preferable.

[jamlen]

Thanks for the contribution, can you also add some tests for these? I'd prefer to see less in-code comments and more tests which describe the desired behaviour and handling of edge cases.

Any comments from @markolson or @benrugg ?

[ghost]

Sure thing, I'll work on getting some tests together.

[markolson]

My one comment isn't particularly relevant to this PR, but the pattern

def THING
    reload! unless @raw['THING']
    @raw['THING']
end

is repeated a lot, and we can probably DRYing that up easily :)

[benrugg]

Hey, @ajkerrigan, thanks for adding this functionality. It's great to have more contributions to kickscraper!

[ghost]

I've added some tests, and had to tweak a couple things along the way to get them all to pass. I guess that's how it's supposed to work! I added small comments in one or two places where the necessary changes were not obvious (at least to me).

I'm pretty green when it comes to Ruby, RSpec and open source contributions in general - so don't be shy if these changes don't jive with the conventions you've established.

[benrugg]

Sorry nobody has checked this out yet. I'll try to get to it sometime soon. Or if anyone else has a chance, go for it!

[benrugg]

Hey, just looking at this right now. I'm wondering if this test is actually passing for the wrong reason. If TEST_PROJECT_ID is a valid project, why is subject nil on the next line?

Maybe the subject should be in a context block, and it should search for an invalid project id?

[benrugg]

Hey @ajkerrigan, thanks again for adding this functionality. It looks great to me. I made one note on a specific test if you want to check that out...

[ghost]

Ah yes I see what you're saying there. It may be that I'm misunderstanding something about how the subject block works. I actually modeled that test after an existing project test, because I was trying to do essentially the same thing:

context "loads more projects after a successful search" do
subject do 
  client.recently_launched_projects
  client.more_projects_available?.should be_true
  client.load_more_projects 
end

it_returns "a collection", Kickscraper::Project
end

It looked to me like the it_returns piece was referring to the last value in the subject block, which is the array returned by load_more_projects. In my case, I figured the effective subject would be the array returned by load_more_updates. Even though it seems to pass, it definitely feels dirty and looks a bit awkward. Does this seem like a reasonable replacement?

  context "loads more updates when available" do
    before { client.find_project(TEST_PROJECT_ID) }

    describe "more updates are available" do
      subject { client }
      its(:more_updates_available?) { should be_true }
    end
    describe "more updates load properly" do
      subject { client.load_more_updates }
      it_returns "a collection", Kickscraper::Update
    end
  end

[benrugg]

Ah yeah, gotcha. You know what... I was talking about the test "throws an error when accessing updates without a valid project loaded". That one seemed weird to me, because it seemed like the way the code is written, it is pulling in a valid project for the subject, rather than an invalid one.

But, I think you're right about the new way of writing the other two tests. That looks more semantic. Either way I think is probably fine. (But I'm not a guru at writing tests!)

[jamlen]

CAVEAT: I've not looked at this since June and I'm currently working in NodeJS so my memory is a bit fuzzy :)

Just a thought but why are these update methods on client.rb and not on project? After all the updates are only related to a given project. It seems from your commit that load_more_updates method is only used from the tests.

This would then lead to:

  context "loads more updates when available" do
    let(:project) { client.find_project(TEST_PROJECT_ID) }

    describe "more updates are available" do
      subject { project }
      its(:more_updates_available?) { should be_true }
    end
    describe "more updates load properly" do
      subject { project.load_more_updates }
      it_returns "a collection", Kickscraper::Update
    end
  end

[ghost]

That's a valid point @jamlen - I'll see about airlifting the update logic from client to project. I remember thinking about where to put the code initially, and now that you ask I can't recall a good reason for putting it on the client.

[benrugg]

Hey @ajkerrigan, a bunch of the code base has changed since you originally submitted this update... Would you like to check it out and see if your new changes still work?

I think we can pull this in, especially if you move the new methods to project instead of client...

[ghost]

I won't be able to devote any time to this until at least next week, but I'd like to have a look and see what if anything can be salvaged. It won't meet my original need (crawling public and private updates for a user's backed projects), but I do think there's still some value in the pure public approach.

I agree that the methods make more sense on project. I'm struggling to remember why the heck I put them on client to begin with!

[benrugg]

Cool, thanks for checking it out again. I haven't done any work with pulling in project updates, but I'd love to see what's possible. (There are still both public and private api calls, so maybe you can do both for the updates).