Address expiry of studies from clinicaltrials.gov #157
Conversation
Added a task to un-publish trials that are no longer in the list of recruiting trials for the location that is being used to pull from clinicaltrials.gov. - Refactored ctgov connector nct_ids_for_location method to use current ct.gov API. Also uses recursion without passing arrays of IDs into a new method invocation for memory benefits. - In same method, include filter for only recruiting studies at location. - Allow cleanup_stray_trials and stray_trials to accept an argument of an array of IDs that should be considered "still recruiting"; this can be leveraged for testing bc there isn't a great way to mock ct.gov API responses given the current state of coupling in the connector. - Tests for these updates.
spec/connectors/ctgov_spec.rb
Outdated
| ctgov.cleanup_stray_trials(remaining_ids) | ||
| will_hide.reload | ||
|
|
||
| expect(will_hide.visible).to be_falsey | ||
| expect(wont_hide_2.visible).to be_truthy |
There was a problem hiding this comment.
If you need to reload will_hide to ensure .visible is falsey, shouldn't you also need to reload wont_hide_2?
There was a problem hiding this comment.
I don't believe so, because trials in wont_hide_2 aren't being changed. Only those in will_hide.
| trials = connector.cleanup_stray_trials | ||
| puts "Have un-published (system_ids): " | ||
| puts trials.map{ |e| " #{e.system_id}\n" } |
There was a problem hiding this comment.
connector.cleanup_stray_trials returns the result of calling update(visible: true on stray trials. Since update will return the record(s) regardless of whether the update actually succeeded, would you need to do any final checking in this rake task to ensure what you are printing actually includes successfully updated records?
There's only a single validation I saw for Trial, so the likelihood of the update failing is probably minimal. And you aren't actually manipulating the returned data, so maybe this doesn't matter much.
There was a problem hiding this comment.
Valid point. Updated to use update! so we get an error alert if there's an issue.
lib/connectors/ctgov.rb
Outdated
| response_ids = Array(JSON.parse(response.body || "{}").dig("StudyFieldsResponse").dig("StudyFields")).map do |result| | ||
| Array(result.dig("NCTId")).first | ||
| response_ids = Array(payload.dig("studies")).map do |result| | ||
| result.dig("protocolSection").dig("identificationModule").dig("nctId") |
|
|
||
| def extract_zip | ||
| dirname = "#{Rails.root}/tmp/" | ||
| unless File.directory?(dirname) |
There was a problem hiding this comment.
FileUtils.mkdir_p doesn't fail if a directory already exists. These checks are probably not needed.
| end | ||
|
|
||
| unless File.directory?("#{dirname}trials/") | ||
| FileUtils.mkdir_p("#{dirname}trials/") |
There was a problem hiding this comment.
There's Dir.mktmpdir that might be appropriate here. It looks kind of like Tempfile but without the built-in cleanup hook I think.
| private | ||
|
|
||
| def extract_zip | ||
| dirname = "#{Rails.root}/tmp/" |
There was a problem hiding this comment.
Not using File.join and friends here is screaming to my eyeballs, but I don't think it's strictly necessary anymore (i.e. forward slashes work on both *nix and Windows now).
This was introduced solely for testing -- replace with stubbing :site_nct_ids in the spec.
Added a task to un-publish trials that are no longer in the list of recruiting trials for the location that is being used to pull from clinicaltrials.gov.