Issue365 add jobs #366

leenyburger · 2018-06-01T17:33:21Z

Description of changes

Added job functionality.
Includes the following (copied from issue):
new db table
new GET index endpoint
new resource in ActiveAdmin
test coverage
API docs

Jobs table to include:
title
source_url
source
city
state
country
description
status (i.e. active, inactive)
remote
Also include tagging.

Issue Resolved

Fixes #365

…point

jjhampton · 2018-06-02T20:49:46Z

@leenyburger @hpjaj Any reason that status has to be a string? When using this data in the front-end, it seems a bit fragile to match on strings, especially if they're user-entered via Admin Dashboard, where there's a chance for human typos. Thoughts on using boolean instead? Example: active: true or active: false.

hpjaj · 2018-06-02T22:56:39Z

@jjhampton - We would create a few constants to ensure data integrity. And a dropdown in ActiveAdmin to choose from.

That said, we can definitely switch to a boolean if you don't foresee any use case where more than two statuses would ever be of value?

If we don't see any value there, then I agree, changing the column name to active and type to boolean would be a good call.

jjhampton · 2018-06-02T23:05:12Z

@hpjaj I've already coded up the front-end stuff to filter on for status === "active" for now, should be fine for the time being.
POC here is @ksmacleod99 - Please see above comments - do you envision ever needing to set a Job's status to anything other than just active / inactive?

hpjaj

Looking good!

hpjaj · 2018-06-02T22:59:26Z

app/admin/job.rb

+    column :remote
+
+    actions
+  end


Can you pls add a form for creating/updating. Here is an example:

https://github.com/OperationCode/operationcode_backend/blob/master/app/admin/code_school.rb#L28-L45

When I add the form and try to add a job I get the following error: "

"exception": "#<ActionView::Template::Error: To use the :country input, please install a country_select plugin, like this one: https://github.com/stefanpenner/country_select>",

Should I install that gem, or are we already using something else?

@hpjaj I don't see where you responded here. If @leenyburger issue has been addressed, disregard just getting visibility.

hpjaj · 2018-06-02T23:00:44Z

app/controllers/api/v1/users/jobs_controller.rb

@@ -0,0 +1,11 @@
+module Api


Did this extra controller sneak in?

hpjaj · 2018-06-02T23:01:56Z

db/seeds.rb

@@ -31,6 +31,9 @@
  Service.create!(:name => service)
 end



Can you pls add a delete_all call for Job i.e. https://github.com/OperationCode/operationcode_backend/blob/master/db/seeds.rb#L9-L16

hpjaj · 2018-06-02T23:02:20Z

db/seeds.rb

@@ -31,6 +31,9 @@
  Service.create!(:name => service)
 end

+# Create jobs
+Job.create!(title: "A great job", source_url: "www.applyhere.com", source: "Company A", city: "Virginia Beach", state: "VA", country: "USA", description: "Our job is fun!", status: "active", remote: "false")
+
 # Create team members


As well as a reporting call, i.e. https://github.com/OperationCode/operationcode_backend/blob/master/db/seeds.rb#L57-L64

hpjaj · 2018-06-02T23:07:57Z

app/models/job.rb

@@ -0,0 +1,3 @@
+class Job < ApplicationRecord
+  acts_as_taggable_on


Can you pls add a method similar to this one in here:

https://github.com/OperationCode/operationcode_backend/blob/master/app/models/user.rb#L130-L141

As well as an associated test. This will confirm that a given job can have tags (plural) added to it, and then we can use this method to query for any jobs that match the search criteria.

hpjaj · 2018-06-02T23:23:01Z

test/models/job_test.rb

+require 'test_helper'
+
+class JobTest < ActiveSupport::TestCase
+  # test "the truth" do


Can you pls create a test in here confirm that the associated Factory is valid. All this would need to do is:

use the factory to create a new Job

assert that it is valid?

This keeps us honest with respect to any validations that we add to the Job model class, and its associated Factory staying in step with those requirements.

hpjaj · 2018-07-14T16:17:19Z

re: @jjhampton 's comment here: #366 (comment)

I agree, would be easier to go the boolean route. If that is still on the table (as I know it would require a bit of a refactor on the FE), I would recommend that, as it is for sure the cleanest solution.

If not, then @leenyburger you'll want to follow this pattern in order to implement choices for "active" and "inactive":

#375 (comment)

After this piece is wrapped up, this PR should be ready to ship.

dmarchante · 2018-08-21T14:07:39Z

Can you review the conflicts and address the final comments from @hpjaj @leenyburger?

leenyburger · 2018-08-22T11:54:50Z

@dmarchante Yes. I'm out of town until next week but can take a look then.

apex-omontgomery · 2018-09-14T19:19:57Z

Hi @leenyburger how is this going? Is there anything blocking you that I can assist with?

leenyburger · 2018-09-16T13:35:57Z

@wimo7083 No blockers, just haven't been able to find the time. Making up for hurricane delay next week, can take a look the week of the 24th.

leenyburger · 2018-10-01T17:40:13Z

@wimo7083 I'm not going to be able to get to this in a timely manner. Sorry to leave you hanging!

apex-omontgomery · 2018-10-03T15:54:58Z

Okay so I'm going to make the decision we want to use a boolean instead of a string. I think this would align better with what the frontend needs.

For this I'd recommend:

changing status to is_active_status
changing all the mocks to respect a boolean instead of string
changing the schema to respect the new type.

If nobody objects I'd like to finish this up.

robbkidd · 2018-10-03T20:37:42Z

I have a maybe philosophical question: what does it mean for a job to have an "active status"?

Merge job changes into head branch

kylemh · 2018-10-05T20:40:59Z

@robbkidd Agreed. I feel like that field is unnecessary. I'd prefer a self-cleaning database.

We could end up having tons of inactive jobs

apex-omontgomery · 2018-10-05T20:45:11Z

Since we don't know when the job will expire. it's probably best to make it adjustable. I can add something to the route so we eliminate innactive jobs.

I went with is_open because apparently using open in Factory girl gave me a security warning.

robbkidd · 2018-10-05T20:50:38Z

🤔 One option for flagging jobs as either open or closed is to record the date on which it closed. This is a bit like a technique named "soft delete." It would involve adding a datetime column closed_at (if you wanted to match the Rails timestamps style). A nil closed_at means the job is still open; a non-nil closed_at means the job is closed and you also know the date it closed. Knowing the closed date means old-and-closed jobs could be removed from the system.

class Job

  scope :open, -> { where(closed_at: nil) }
  scope :closed, -> { where.not(closed_at: nil) }

  def open?
    self.closed_at.nil?
  end

  def closed?
    !open?
  end
end

There is an open method on Kernel that does IO object instantiation. That might have been the cause of the error from FactoriGirl. It is safe to override that method here with the query scope. The Job class is probably not going to use that Kernel method directly to open a file or some other IO stream.

Makefile

apex-omontgomery · 2018-10-05T21:23:44Z

Makefile

 .PHONY: test
 test: bg
-	docker-compose run operationcode-psql bash -c "while ! psql --host=operationcode-psql --username=postgres -c 'SELECT 1'; do sleep 5; done;"
-	docker-compose run ${RAILS_CONTAINER} bash -c 'export RAILS_ENV=test && rake db:test:prepare && rake test && rubocop'
+	$(DOCKER_COMPOSE) run operationcode-psql bash -c "while ! psql --host=operationcode-psql --username=postgres -c 'SELECT 1'; do sleep 5; done;"


I'm starting to feel odd about this.

As this gets bigger it should really be things in their own target, or in a bash script. Large makefiles with lots of distractions are hard to understand. Plus hiding tests behind a very long bash script means I can't pass in parameters to run test groups.

robbkidd · 2018-10-06T01:06:58Z

test/factories/jobs.rb

+    city Faker::Address.city
+    state Faker::Address.state
+    country Faker::Address.country
+    description Faker::Lorem.paragraph


All the attributes being set above this comment should get assigned with block to evaluate the Faker data lazily each time the factory is used instead of at test load time. Otherwise all jobs created with this factory will have the same values.

e.g. title { Faker::Lorem.characters(7) }

apex-omontgomery · 2018-10-06T20:13:01Z

app/models/job.rb

@@ -0,0 +1,10 @@
+class Job < ApplicationRecord


Does having this in the models section of the code base make it clear enough that this is separate from Sidekiq Jobs?

I think so, yes. Though, it might be helpful to rename the background jobs from Job(s) to Worker(s).

Are you sure you don't want JobJobs?

Now that you mention it, maybe WorkJobber.

leenyburger added 5 commits June 1, 2018 13:24

Apiary doc no sematic issues, added jobs controller, model, test, end…

ada2703

…point

Added acts_as_taggable to job model

a1ffefe

Fixed database user name issue

777af57

Fixed sytax errors

8543559

Removed extraneous comments

6f022f0

hpjaj self-requested a review June 1, 2018 19:52

jjhampton mentioned this pull request Jun 2, 2018

Display featured jobs on Jobs page, above search results OperationCode/operationcode_frontend#985

Merged

hpjaj reviewed Jun 2, 2018

View reviewed changes

leenyburger added 2 commits June 4, 2018 10:54

Added test for job acts as taggable, updated admin

e2c5b19

Fixed formatting errors:

a1bcd87

jjhampton mentioned this pull request Aug 21, 2018

Add jobs table, endpoint, and functionality #365

Open

wimo7083 and others added 5 commits October 5, 2018 13:48

slight modifications

b9e40d6

fixed test issues with variable change

263375b

fix dangling is_open

75eed14

Merge pull request #418 from wimo7083/issue365_add_jobs

2c0596f

Merge job changes into head branch

Merge branch 'master' into issue365_add_jobs

7656a81

wimo7083 added 2 commits October 5, 2018 14:57

start adding expiration

8960e38

section makefile- its getting large, and update apiary

c85f56d

robbkidd reviewed Oct 5, 2018

View reviewed changes

Makefile Outdated Show resolved Hide resolved

wimo7083 added 2 commits October 5, 2018 15:19

changed to scoped call with default being active

f6de002

re-add phony for target

ef27d87

apex-omontgomery reviewed Oct 5, 2018

View reviewed changes

wrong location

d8c8336

robbkidd reviewed Oct 6, 2018

View reviewed changes

completed scope and added block scope for mocking

9ea4d55

apex-omontgomery reviewed Oct 6, 2018

View reviewed changes

apex-omontgomery added Needs DB Migration Status: Blocked labels Nov 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue365 add jobs #366

Issue365 add jobs #366

leenyburger commented Jun 1, 2018 •

edited

Loading

jjhampton commented Jun 2, 2018

hpjaj commented Jun 2, 2018

jjhampton commented Jun 2, 2018

hpjaj left a comment

hpjaj Jun 2, 2018

leenyburger Jun 4, 2018 •

edited

Loading

dmarchante Jul 14, 2018

hpjaj Jun 2, 2018

hpjaj Jun 2, 2018

hpjaj Jun 2, 2018

hpjaj Jun 2, 2018

hpjaj Jun 2, 2018

hpjaj commented Jul 14, 2018 •

edited

Loading

dmarchante commented Aug 21, 2018

leenyburger commented Aug 22, 2018

apex-omontgomery commented Sep 14, 2018

leenyburger commented Sep 16, 2018

leenyburger commented Oct 1, 2018

apex-omontgomery commented Oct 3, 2018

robbkidd commented Oct 3, 2018

kylemh commented Oct 5, 2018

apex-omontgomery commented Oct 5, 2018

robbkidd commented Oct 5, 2018 •

edited

Loading

apex-omontgomery Oct 5, 2018

robbkidd Oct 6, 2018

apex-omontgomery Oct 6, 2018

robbkidd Oct 6, 2018

BRIMIL01 Oct 6, 2018

robbkidd Oct 6, 2018

		@@ -0,0 +1,3 @@
		class Job < ApplicationRecord
		acts_as_taggable_on

Issue365 add jobs #366

Are you sure you want to change the base?

Issue365 add jobs #366

Conversation

leenyburger commented Jun 1, 2018 • edited Loading

Description of changes

Issue Resolved

jjhampton commented Jun 2, 2018

hpjaj commented Jun 2, 2018

jjhampton commented Jun 2, 2018

hpjaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leenyburger Jun 4, 2018 • edited Loading

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

hpjaj commented Jul 14, 2018 • edited Loading

dmarchante commented Aug 21, 2018

leenyburger commented Aug 22, 2018

apex-omontgomery commented Sep 14, 2018

leenyburger commented Sep 16, 2018

leenyburger commented Oct 1, 2018

apex-omontgomery commented Oct 3, 2018

robbkidd commented Oct 3, 2018

kylemh commented Oct 5, 2018

apex-omontgomery commented Oct 5, 2018

robbkidd commented Oct 5, 2018 • edited Loading

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

leenyburger commented Jun 1, 2018 •

edited

Loading

leenyburger Jun 4, 2018 •

edited

Loading

hpjaj commented Jul 14, 2018 •

edited

Loading

robbkidd commented Oct 5, 2018 •

edited

Loading