-
Notifications
You must be signed in to change notification settings - Fork 2
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
WorkableFormFiller and seedfile updates #224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall
@@ -18,7 +18,7 @@ def job_url_api(url_api, ats_identifier, _job_id) | |||
"#{url_api}#{ats_identifier}?includeCompensation=true" | |||
end | |||
|
|||
def fetch_url(job_data) | |||
def fetch_url(job_data, _company_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is _company_id doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing useful! It was a lazy solution to the problem that only BambooHR needs info extrinsic to the all_jobs json in order to build the posting_url, but I've found a workaround and removed it.
@@ -18,6 +18,10 @@ def fetch_id(job_data) | |||
job_data['id'] | |||
end | |||
|
|||
def fetch_url(job_data, company_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose/context of the fetch_url method again?
storage/csv/unparseable_urls.csv
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the process that keeps outputting unparseable_urls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think it's the DevITJobs updater
data&.each do |job| | ||
title, location = @ats.fetch_title_and_location(job) | ||
relevant_jobs << @ats.fetch_url(job) if relevant?(title, location) | ||
data&.each do |job_data| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if there is a better way to filter relevant jobs whilst we look at this. @daniel-sussman / @patanj101 - any thoughts on how else we might only draw in relevant jobs from each company's API endpoint?
One thought is that we could just pull all the jobs at this point and then add the filter to the query later. I'd be interested in what effect this had on performance - you don't have to check whether each job is relevant, but you do have to import more jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2nd review
@@ -18,6 +18,10 @@ def fetch_id(job_data) | |||
job_data['id'] | |||
end | |||
|
|||
def fetch_posting_url(job_data, company_id) | |||
url_base.sub('XXX', company_id) + job_data['id'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should convert in due course to proper variable names e.g. ${company_id}
@@ -1,9 +1,13 @@ | |||
module Importer | |||
# This is a helper class that's called from the seedfile. Its purpose is to receive a list of ats_identifiers and return a list of posting urls for relevant jobs to seed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very helpful - cheers
end | ||
relevant_jobs | ||
end | ||
|
||
def additional_args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the story here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I made the name more explanatory and added a comment.
…g companies called 'j'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3rd Review - great stuff Dan
@@ -100,6 +100,7 @@ gem 'rails-html-sanitizer' | |||
# HTTP Clients | |||
# gem 'httparty' # Installed as a dependency via Avo | |||
# gem 'faraday' # Installed as a dependency via Cloudinary | |||
gem 'faraday-follow_redirects', '~> 0.3.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably does what it says on the tin - easiest way of achieving this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, think this is easiest if we want to handle all requests with Faraday rather than HTTParty.
@@ -12,6 +12,12 @@ def fetch_id(job_data) | |||
job_data['shortcode'] | |||
end | |||
|
|||
def fetch_posting_url(job_data, company_id) | |||
# build long-form posting url that includes company_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you put the comment above the method then it will generate docs correctly. I like having these in and would be keen to include these on more methods. FAO also @patanj101
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
@@ -3,8 +3,8 @@ module Workable | |||
module ParseUrl | |||
def parse_url(url, _saved_ids = nil) | |||
regex_formats = [ | |||
%r{https://apply\.workable\.com/([^/]+)(?:/j/([^/]+))?}, | |||
%r{https://apply\.workable\.com/api/v1/accounts/([^/]+)(?:/jobs/([^/?]+)(?:\?.*)?)?} | |||
%r{^https://apply\.workable\.com/(?:([^/]+)/)?j/([^/]+)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impact of changes here? Does this also handle HTTP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should prevent the parser bug that made CompanyCreator try to create Workable companies named 'j'. It only handles https, not http (none of the parsing methods do).
@@ -43,158 +38,5 @@ def hidden_element | |||
def response_field | |||
find(:css, "label[for='#{@locator}']").ancestor('fieldset', match: :first) | |||
end | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer required presumably as we have the actual payload? Is it all working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just moved this from the children to the parent class, actually. Will implement a functional #click_submit_button method once we're finished with development testing.
@@ -45,15 +45,16 @@ def attach_file_to_application | |||
attach_file(@locator, @filepath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add comments to this file please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
# apply_url = 'https://adobe.wd5.myworkdayjobs.com/en-US/external_experienced/job/Sr-Machine-Learning-Engineer--Adobe-Firefly_R147574/apply' | ||
apply_url = 'https://maxar.wd1.myworkdayjobs.com/en-US/MAXAR/job/Senior-Automation-Engineer_R21029/apply' | ||
Importer::GetWorkdayFields.call(apply_url) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add one of these for each ATS integration Dan?
Also can we add some validation condition(s) so that we know it's worked? e.g. Capybara checks all input fields are filled. Or checks a particular input field contains the value in AQS. Or we could deliberately not fill one of the required fields and click the submit button - if the form returns >1 error then there is an issue we haven't noticed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also add similar functionality in the rspec folder in due course?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the formfiller ones are all in the ff.rake file. I've been using the admin.rake file just for convenience of testing during development (restoring the file before pushing to main).
For classes that rely on Capybara, I'm not sure you want me to write rspec tests because they'll be slow. The rakefile works well for informal testing in development, where I give Capybara sleep instructions so I can verify visually that the form's correct.
group[response] | ||
end | ||
|
||
ASHBY_PAYLOADS = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have the payloads sit in a separate file so that this is a bit cleaner?
end | ||
end | ||
|
||
def prompt_user(group) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally this logic would sit in a service class rather than in the rake file for cleanness - can you move?
@@ -0,0 +1,1076 @@ | |||
namespace :ff do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly with this testing @patanj101
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice solution btw Dan
@@ -3,7 +3,7 @@ | |||
RSpec.describe CompanyJobsFetcher do | |||
context "with an existing company" do | |||
before do | |||
allow($stdout).to receive(:write) # suppresses terminal clutter | |||
# allow($stdout).to receive(:write) # suppresses terminal clutter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What changed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a lazy test that relied on capturing what's printed to the console ($stdout). One problem is that diverting $stdout means it doesn't display anymore, so you have no info on why the test failed. I changed this to evaluate method result rather than its printed output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved - cheers both
WorkableFormFiller is pretty solid now.
Added a rakefile for testing purposes. To test FormFiller, type 'rake ff:[ats_name]', e.g. rake ff:greenhouse.
The seedfile now includes Ashby, Bamboo, Greenhouse and Workable jobs.