USACE / cwms-data-api

Corps Water Management System RESTful Data Service
MIT License
13 stars 14 forks source link

Update offices and offices/{office} end points to include an operational data attribute #321

Closed rma-psmorris closed 1 year ago

rma-psmorris commented 1 year ago

This is a request to update the CDA offices and offices/{office} end points to include an attribute per office as to whether the office has operational data. Suggested is that the office owning locations be the indicator that an office has operational data.

The reason for the request is that the CWMS client AAA authentication UI displays a selection of offices (sourced from the offices end point) for the user to choose from that is later used in calls against CDA. The addition of this attribute will allow us to limit the set of offices displayed to those offices that have operational data.

rma-rripken commented 1 year ago

Instead of adding an attribute to the results could the end-point take an optional filter? require-locations=true or something like that. As far as actually finding the offices that own locations - is that just a matter of " select distinct DB_OFFICE_ID from cwms_20.AV_LOC" or is there a better way?

MikeNeilson commented 1 year ago

I agree with Ryan. It would be cleaner to have a parameter on the request.

that query is pretty much how I would do it. Though I think it needs to have "where location_code != 0" to avoid the "deleted ts" code.

FYI, we're going to have Charles work on this particular one because it is simple and gives him some experience but I am going to mark it as approved in case he needs help and Daniel and I are unavailable.

rma-psmorris commented 1 year ago

@adamkorynta -- you would be the one coding against this new end feature. Can you please add your thoughts per Ryan's suggestion?

adamkorynta commented 1 year ago

I like the parameter has-data=true as it hides the location query as an implementation detail for how it is determined. I don't think we'd want this endpoint to balloon into has-locations=true&has-timeseries=falseetc.

adamkorynta commented 1 year ago

Though I guess I'll say the opposite of that if you were to add in attributes instead of a parameter. It may be useful for the returned office data to have a list of data types for that office:

{
   office-id = '',
   office-name = '',
   has-locations = true,
   has-timeseries = true,
   has-location-levels = true,
}

But I don't have a use-case for that kinda of implementation.

MikeNeilson commented 1 year ago

let's go with "lack of use-case = no reason to add code."

adamkorynta commented 1 year ago

100%

krowvin commented 1 year ago

Running SELECT DISTINCT DB_OFFICE_ID FROM CWMS_20.AV_LOC WHERE LOCATION_CODE != 0 ORDER BY DB_OFFICE_ID against the database returns

CWMS
LRB
LRD
LRE
LRH
LRL
LRN
LRP
MVK
MVM
MVN
MVP
MVR
MVS
NAB
NAE
NAN
NAO
NAP
NWD
NWDM
NWDP
NWO
SAJ
SAM
SAS
SAW
SPA
SPK
SPL
SWF
SWG
SWL
SWT

Is this what we would expect? Do we see anyone missing?

MikeNeilson commented 1 year ago

That looks right to me. Though for that initial analysis sorting alphabetically might make it make more sense.

SPN is all under SPK, so that makes sense not to to be there. the NWD is suspicious to me but that whole region is complicated so it probably does need to be there.

krowvin commented 1 year ago

Good point, updated alphabetical with new SQL

rma-psmorris commented 1 year ago

I forget why CWMS owns a location code.

MikeNeilson commented 1 year ago

oh, that's probably the "deletes ts/location" marker. the query should also check for location_code <> 0 (not equals)

krowvin commented 1 year ago

I thought Oracle supported != since 10g? I was under the impression doing != would be the same as <> Running the same query and replacing != with <> returns the same 34 results above, including CWMS. Unless i'm misunderstanding. Here's a snippet of some of the base locations that show up with the cwms DB_OFFICE_ID image

Would we prefer the result of this query to ignore cwms and not return it?

MikeNeilson commented 1 year ago

Huh, that’s surprising. But yeah, have the query manually ignore the CWMS office. Looks like we have some cleanup to do.