msupply-foundation / open-msupply

Open mSupply represents our most recent advancement in the Logistics Management Information System (LMIS), expanding on more than two decades of development inherited from the well-established legacy of the original mSupply.
https://msupply.foundation/open-msupply/
Other
23 stars 15 forks source link

Report graphql queries and backwards compatibility #2000

Closed andreievg closed 1 month ago

andreievg commented 1 year ago

A placeholder to record this conversation: https://github.com/openmsupply/open-msupply/pull/1979#discussion_r1261858646

An outcome of this issue would be a strategy to deal with potential changes to graphql api that may affect report. (for now we should not make any breaking changes to query API).

Back of an envelop solutions could include:

Suggested solution

Manifest file to include min omSupply version, allow multiple reports with the same name (can't put in just report json since report arguments can change), add om_ field for version in mSupply. See epic for easy of deployment and testing

andreievg commented 9 months ago

There seems to be a break in standard report, as mentioned in telegram chat.

Looks like it happened because default query was changed, if query is not specified (and the template considers data in the previous query shape). We did emphasises the importance of not changing API for reports, but missed the point about changing default queries.

I see this as a very high impacts area (a lot of effort to fix and a lot of disruption to users workflow) with medium likelihood, so very high risk.

Would propose we add tests for standard reports and add for consideration in triage

andreievg commented 4 months ago

I think this needs to be looked at by Ruru team during report work

mark-prins commented 4 months ago

Note that the breaking change was to the default graphQL queries, if we bundle gql with the report then we don't repeat that particular issue. there is potential to introduce a breaking change to the graphQL schema - that could well break other parts of the app too though.

andreievg commented 2 months ago

that could well break other parts of the app too though.

Our codegen would tell us, but yes there is a potential to break other parts of the system as a whole (integrations)

roxy-dao commented 1 month ago

Already done in epic #4800