ohmage / server

The ohmage server application.
37 stars 25 forks source link

dates in response/read should be inclusive #869

Closed jeroen closed 8 years ago

jeroen commented 8 years ago

Currently the date parameters for /app/survey_response/read are interpreted as a non-inclusive interval which means that setting end_date=2015-10-21 will not return responses from the date 2015-10-21 itself. People find this confusing and would prefer the inclusive interval.

Example:

https://lausd.mobilizingcs.org/app/survey_response/read?campaign_urn=urn:campaign:lausd:2015:fall:polytechnicsh:stevenson:math:p1:height&privacy_state=shared&client=manager&user_list=urn:ohmage:special:all&prompt_id_list=urn:ohmage:special:all&output_format=csv&sort_oder=timestamp&column_list=urn:ohmage:user:id,urn:ohmage:context:timestamp,urn:ohmage:prompt:response,urn:ohmage:context:location:latitude,urn:ohmage:context:location:longitude&suppress_metadata=true&start_date=2015-01-01&end_date=2015-10-21

stevenolen commented 8 years ago

@jeroenooms actually, I think the server is handling this as intended..but the documentation is certainly lacking. Given these two survey_response/read calls:

test1 = oh.survey_response_read(campaign_urn: 'urn:campaign:cathy:test:mwfcathy:12112014newcampaign', start_date: '2011-01-01', end_date: '2015-10-23')
test2 = oh.survey_response_read(campaign_urn: 'urn:campaign:cathy:test:mwfcathy:12112014newcampaign', start_date: '2011-01-01', end_date: '2015-10-23 23:59:59')
test1.count
=> 5
test2.count
=> 6

It's a bit more clear what's going on. The dates provided by the client are cast to a long as a unix timestamp and then used in the sql query (seen here: https://github.com/ohmage/server/blob/master/src/org/ohmage/query/impl/SurveyResponseQueries.java#L943-L946). To me it feels very unnatural to try to discern the lack of specificity in this date in the server code (ie. when should the end_date by taken literally and when should it be set as the futuremost timestamp in that date).

I can update the readme to make it more clear that a client can provide hh:mm:ss as well as the date..

jeroen commented 8 years ago

Mmm yes end_date is a bit of a misnomer if it expects a timestamp. I can easily add 23:59:59 to the string on the client. Yet I think that it would be neat to do this on the server for other applications that are also passing a date.

stevenolen commented 8 years ago

@jeroenooms and I had a bit of a side conversation about this. It makes sense to leave these params as-is on the server: when a client provides a non-specific date, the server has to make an assumption about the entire timestamp (eg. client passes 2011-01-01 the server will parse the date as 2011-01-01 00:00:00). Since that date can't be represented as two different unique dates (00:00:00 and 23:59:59) it needs to be up to the client to be as specific as it needs to be for user expectation based on the client's environment (eg, if the client provides a "from" and "to" date range, it's expecting "to" to be inclusive of the entire day, rather than just the date).

This is a good one to note for others interacting with the survey_response/read api call!