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

Add technology parameters to jobs#index, ready for review #96

Merged
merged 4 commits into from
Jun 11, 2016

Conversation

hhoopes
Copy link
Contributor

@hhoopes hhoopes commented Jun 10, 2016

I can't paginate the endpoint for individual technologies because the method sees it as an individual resource. Somehow I need to point it at the jobs, or potentially return the jobs in the first place, and include the parameter/technology name in the JSON (working on the latter, this issue seems relevant.

hhoopes added 2 commits June 9, 2016 22:12
Because technology#show is a single resource, pagination balks and throws and error at pagination. Somehow I need to point the pagination at the jobs.
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.09%) to 89.216% when pulling e69991f on 92-tech-endpoints into e31f313 on master.

@brennanholtzclaw
Copy link
Contributor

I'm not positive I understand the problem, but would writing a simple scope on Job for each Technology solve this? That way you could call, for instance Job.ruby and return (and paginate?) only the Ruby jobs?

Does this make sense or am I off base here?

@hhoopes
Copy link
Contributor Author

hhoopes commented Jun 10, 2016

I think I"ll have to show you in person.(I say as you sit right next to me)

hhoopes added 2 commits June 10, 2016 15:36
Jobs#index now supports querying for a subset of jobs based on technology. The index action now looks to see if there's a technology parameter, then scopes the jobs response appropriately.

This pattern will be a little dryer and more sustainable than what I previously was playing with.

Sample query looks like:
api/v1/jobs/?technology=python
@hhoopes hhoopes changed the title WIP technology#show can't paginate Add technology parameters to jobs#index, ready for review Jun 10, 2016
@hhoopes
Copy link
Contributor Author

hhoopes commented Jun 10, 2016

Ready for review @cheljoh @NickyBobby @brennanholtzclaw

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 92.647% when pulling be9c557 on 92-tech-endpoints into f5308bf on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 92.647% when pulling be9c557 on 92-tech-endpoints into f5308bf on master.

@@ -19,4 +20,8 @@ def assign_tech
tech_matches = Technology.where(name: raw_technologies)
self.technologies = tech_matches
end

def self.by_tech(tech_name)
joins(:technologies).where(technologies: {name: tech_name})
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good! Nice "scope" you made here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I came across this weird note that Rails "officially" prefers class methods if you pass in a param so I shrugged and made a class method instead of a scope. But it does make me feel weird. ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Can you make a scope that accepts an argument? I thought that's why you did it this way/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it looks like scope :created_before, ->(time) { where("created_at < ?", time) } end
(stolen from the aforementioned docs)

@NickyBobby
Copy link
Contributor

Good Job Heidi! I like this dual feature endpoint we got now! I'm gonna go ahead and close this.

we did it

@NickyBobby NickyBobby merged commit c0dd53d into master Jun 11, 2016
@cheljoh
Copy link
Contributor

cheljoh commented Jun 11, 2016

Hey hey it's the Monkees!
On Jun 10, 2016 9:06 PM, "Nicholas" notifications@github.com wrote:

Merged #96 #96.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#96 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ANi6ICBlJ7shwxwvh1rcOmsib_g1b1tsks5qKiZMgaJpZM4Iyodr
.

@hhoopes
Copy link
Contributor Author

hhoopes commented Jun 11, 2016

I approve of this giphy.

@cheljoh
Copy link
Contributor

cheljoh commented Jun 11, 2016

Hey sorry I didn't get a chance to look at this until now. Does the endpoint return all jobs that match tech or does it scope by recent jobs? Might be nice to return only more recent jobs (e.g., last two months).

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.

5 participants