-
Notifications
You must be signed in to change notification settings - Fork 17
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
Ptime mapper script #737
base: master
Are you sure you want to change the base?
Ptime mapper script #737
Conversation
…r the ptime client
…nding skills person
…employee id and update people data scripts
…ite specs to test mapping of people data
…ed for testing requests
@@ -118,9 +119,13 @@ GEM | |||
deep_merge (~> 1.2, >= 1.2.1) | |||
countries (6.0.0) | |||
unaccent (~> 0.3) | |||
crack (1.0.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.
😂
|
||
def fetch_data | ||
puts 'Fetching required data...' | ||
@ptime_employees = Ptime::Client.new.get('employees', { per_page: 1000 })['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.
You surely are aware of the problem if there were suddenly more than 1000 people coming in from the api.
However, let's not do this yet. Just don't use the magic number here and put it in a constant (all uppercase)
ptime_employee_email = ptime_employee['attributes']['email'] | ||
matched_skills_people = Person.where(ptime_employee_id: nil) | ||
.where(name: ptime_employee_name) | ||
.where(email: ptime_employee_email) |
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.
add .first
to the end of this.
.where(name: ptime_employee_name) | ||
.where(email: ptime_employee_email) | ||
|
||
if matched_skills_people.empty? |
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.
after adding .first
above you can just check for .nil?
here
and of course rename the variable
|
||
if matched_skills_people.empty? | ||
unmatched_entries << { name: ptime_employee_name, id: ptime_employee['id'] } | ||
elsif matched_skills_people.one? |
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 then just an else
after adding the .first
unmatched_entries << { name: ptime_employee_name, id: ptime_employee['id'] } | ||
elsif matched_skills_people.one? | ||
if should_map | ||
matched_skills_person = matched_skills_people.first |
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 then not needed
@ptime_employees.each do |ptime_employee| | ||
ptime_employee_firstname = ptime_employee['attributes']['firstname'] | ||
ptime_employee_lastname = ptime_employee['attributes']['lastname'] | ||
ptime_employee_name = "#{ptime_employee_firstname} #{ptime_employee_lastname}" |
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.
Don't assign the variables on line 28 and 29 just do the hash digging here and save those 2 lines
private | ||
|
||
def request(method, path, params = nil) | ||
path = "#{path}?#{URI.encode_www_form(params)}" |
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.
don't use path
as a variable name after passing it
this is a URL if you ask me
|
||
def response_error_message(exception) | ||
JSON.parse(exception.response.body).dig('error', 'message') | ||
rescue JSON::ParserError # rescue only JSON parsing errors |
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.
Why would that happen?
@@ -37,5 +37,7 @@ class Application < Rails::Application | |||
|
|||
config.assets.enabled = true | |||
config.assets.paths << Rails.root.join("uploads") | |||
|
|||
config.active_job.queue_adapter = :delayed_job |
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 don't use delayed_job anymore do we? This broke it for me anways...
Remove everything related to DJ?
No description provided.