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

15/05/24 update existing company jobs #162

Merged
merged 15 commits into from
May 24, 2024

Conversation

daniel-sussman
Copy link
Collaborator

Cleaned up UpdateExistingCompanyJobs so it now uses #create_all_relevant_jobs and updates both ats_identifiers.csv and invalid_ids.csv when finished. Some bugfixes along the way and a few tweaks to CheckUrlIsValid to make json queries more tolerant of redirects, timeouts and other weird server responses.

@Ches-ctrl
Copy link
Owner

Can you fix the run errors Dan? Looks like VCR may have got turned off somehow

options = { headers: { 'Accept' => 'application/json' } }

begin
response = HTTParty.get(api_url, options)

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

The URL of this request depends on a
user-provided value
.
Copy link
Owner

Choose a reason for hiding this comment

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

Added sanitize url to patch this

Copy link
Owner

@Ches-ctrl Ches-ctrl left a comment

Choose a reason for hiding this comment

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

No changes required but cannot merge until Ilya's PR goes through as there will likely be a merge conflict

Copy link
Owner

Choose a reason for hiding this comment

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

Need to consider how we're going to get the invalid IDs csv out of Heroku

Copy link
Owner

Choose a reason for hiding this comment

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

Are we still having standard application_field hashes or is this just for testing?

Copy link
Owner

Choose a reason for hiding this comment

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

Super nice we can get this from the API!

Copy link
Owner

Choose a reason for hiding this comment

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

How come the change to phone_number? Does that need to be reflected elsewhere?

Copy link
Owner

Choose a reason for hiding this comment

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

Need to approve Ilya's updates in his prior PR before this can go through - make sure we're not losing any functionality in the cross-over

Copy link
Owner

Choose a reason for hiding this comment

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

Apologies if this was where the job_description error was

options = { headers: { 'Accept' => 'application/json' } }

begin
response = HTTParty.get(api_url, options)
Copy link
Owner

Choose a reason for hiding this comment

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

Added sanitize url to patch this

Copy link
Owner

Choose a reason for hiding this comment

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

Added sanitize_url to fix this, although appreciate the risk is minimal

Copy link
Owner

Choose a reason for hiding this comment

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

Wrapped the API calls in cassettes - previously were building but not calling them

Copy link
Owner

Choose a reason for hiding this comment

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

How come the change in API endpoint?

Copy link
Owner

@Ches-ctrl Ches-ctrl left a comment

Choose a reason for hiding this comment

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

All good, no conflicts, merging

@Ches-ctrl Ches-ctrl merged commit 5083059 into master May 24, 2024
7 checks passed
@Ches-ctrl Ches-ctrl deleted the 15/05/24_UpdateExistingCompanyJobs branch May 28, 2024 21:13
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.

2 participants