Skip to content

Add sha and time stamp keys to JSON output#24

Open
mienkoja wants to merge 4 commits intomasterfrom
add_sha_and_time_stamp_keys
Open

Add sha and time stamp keys to JSON output#24
mienkoja wants to merge 4 commits intomasterfrom
add_sha_and_time_stamp_keys

Conversation

@mienkoja
Copy link
Copy Markdown
Collaborator

These commits implement several changes to provide 2 new keys to the output of get_metric_list(): calc_date, and calc_sha.

  • calc_date will provide a timestamp (via lubridate::today()) indicating the time that a measurement object was initialized (e.g. via build_all_metrics()).
  • calc_sha will provide any text within /app/R/install-packages.R which matches the following REGEX expression: (?<=oliveR@).*(?=\\"). In principle, this should only be the SHA listed in the install-packages.R script within the oliver-metrics-api image. If this script does not exist in a given environment (e.g. due to running oliveR locally), the key should return 'SHA information not available'.

mienkoja added 4 commits July 12, 2017 06:45
…ed to return a SHA from the metrics-api container image. In most other instances, the function will simply return 'SHA information not available'
…single_value class to add creation time-stamps and SHAs.
@mkornblum
Copy link
Copy Markdown
Contributor

I don't have a lot of context for this, why do we need these pieces of information? Are we planning to display them in the UI somehow?

Comment thread R/get_sha_info.R
sha_file <- tryCatch(paste0(readLines("/app/R/install-packages.R"), collapse = "")
,warning=function(w) 1)
if(sha_file==1) {
sha_value <- 'SHA information not available'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we just return from inside the if/else here we could eliminate the assignment to sha_value as well as line 8

@mkornblum
Copy link
Copy Markdown
Contributor

Confirmed calc_date and calc_sha are present in responses to the get_metric_list endpoint:

% curl -i -k -X POST 'http://localhost:443/ocpu/library/oliveR/R/get_metric_list/json' -H "Content-Type: application/json" -d '{"group_id":"7"}'         :( 1 17-07-12 - 12:13:07
HTTP/1.1 200 OK
Date: Wed, 12 Jul 2017 19:21:16 GMT
Server: libwww-perl-daemon/6.01
Content-Type: text/plain
Content-Length: 1377
Last-Modified: Wed, 12 Jul 2017 19:21:16 GMT

[{"id":"7","acceptance_to_schedule":{"characteristic":"Days Until Visit is Scheduled","characteristic_label":"Expected Days to Schedule","measurement_missing":true,"label":"Expected Days to Schedule","threshold":false,"template":"default","calc_date":"2017-07-12","calc_sha":"bfbc2c690012d934bd12267780cb7e2e3f96b5c7"},"acceptance_to_first_visit":{"characteristic":"Days Until First Visit, as Planned","characteristic_label":"Expected Days to First Planned Visit","measurement_missing":true,"label":"Expected Days to First Planned Visit","threshold":false,"template":"default","calc_date":"2017-07-12","calc_sha":"bfbc2c690012d934bd12267780cb7e2e3f96b5c7"},"child_count_value":{"characteristic":"Children per Referral","characteristic_label":"Children per Referral","characteristic_summary_value":1.7,"measurement_missing":false,"value":1.7,"label":"Children per Referral","threshold":false,"template":"default","calc_date":"2017-07-12","calc_sha":"bfbc2c690012d934bd12267780cb7e2e3f96b5c7"},"attendance_per_scheduled_visit":{"characteristic":"Percentage of Scheduled Visits, which Were Attended","characteristic_label":"Visit Attendance Rate","characteristic_summary_value":"23%","measurement_missing":false,"value":"23%","label":"Visit Attendance Rate","threshold":false,"template":"default","calc_date":"2017-07-12","calc_sha":"bfbc2c690012d934bd12267780cb7e2e3f96b5c7"}}]

So functionality is verified and I am fine to 🚢 but still curious what the intent of the change is

@mkornblum
Copy link
Copy Markdown
Contributor

Another idea is that we could include these at a higher level of the response, like up where id is, since they are metadata that is the same for the whole response, instead of having to include them on every calculation object.

@mienkoja
Copy link
Copy Markdown
Collaborator Author

mienkoja commented Jul 12, 2017 via email

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