insightsengineering / teal.reporter

Create and preview reports with Shiny modules
https://insightsengineering.github.io/teal.reporter/
Other
8 stars 9 forks source link

Implement the `Storager` class #6

Closed kpagacz closed 2 years ago

kpagacz commented 2 years ago

Implement a class responsible for saving a Report to a format that can be used to restore the Report object to a previous state.

The motivation is:

Some suggestions:

Here is how it fits into the larger picture: https://app.diagrams.net/#G1MOUaQWakvtz0D_S6VlhHY8j8qzDu1qCW

Polkas commented 2 years ago

We think of Archive name now, not Storage

nikolas-burkoff commented 2 years ago

We should make sure that information about the data (and app - app id may need some thought?) used to create the report are stored in the class so that if you try to load a report into

a) a different app b) the same app with different data (i.e. connector pulls newer data) then the report cannot be loaded.

Although maybe we allow a) to be supported although that would add lots of complexity and for b) if you want to be able to go back and update a report where the data has been updated you are likely to be stuck - without a way of pulling the old data into the specific app.

Is that likely to be a use case @kumamiao ?

kpagacz commented 2 years ago

Is there an assumption there that loading a report would somehow "set" the options of an application to what's stored in the report? How would that work?

nikolas-burkoff commented 2 years ago

I was just thinking that we need to make sure a report can only load if it's valid (i.e. it makes no sense to load a random report A into shiny app Z)

kpagacz commented 2 years ago

I would assume any report can be loaded to a previewer. Is there something we should take into account when loading (and by loading, I mean just passing the contents to the previewer).?

kpagacz commented 2 years ago

Indeed, I would make sure that any kind of report can be previewed inside the general previewer. At least it shouldn't throw. This includes a report that has a TealReportCard in it.

I imagine classes exposing methods to serialize them and save them and then deserialize them to an actual object. Please, take care that a client deserialising the report might not know about teal. If we ever move TealReportCard to teal this can introduce a situation where if teal is not available we deserialize a TealReportCard object to a ReportCard object - and it should work. Liskov substitution, ladies and gents. It doesn't mean that the conversion must be lossless!

Just take extra care to make sure it works right now and will work in the future.

Polkas commented 2 years ago
Format
{
report_id: str - hash + time
report_params: {title: str, author: str, ...}
report_schema_version: str
session_token: str session token
cards: {
  ReportCard: {
    card_name: str
    content: {text_block: {}, ... , table_block: {path: [./img_id.png, ...]}}
    metadata: {filter_state: {}, encodings_state: {}, r_code: str}}, 
  ReportCard: {},
    ..., 
  TealReportCard: {
    card_name: str
    content: {text_block: {}, ... , table_block: {path: [./img_id.png, ...]}}
    metadata: {filter_state: {}, encodings_state: {}, r_code: str}
  }
  }
}
kpagacz commented 2 years ago

Almost there, I think.

The reason for moving the class name to a field is that we want the resource type to be a part of the specification. Whereas the name of the binding (the name of the object) is usually not a part of the object specification. It is how all the provisioning tools work (so Terraform, Cloudformation, etc).

nikolas-burkoff commented 2 years ago

I think we need the hashes of the data as well i.e.

data_hashes : { "ADSL" : hash } , {"ADTTE", : hash}

Or some other structure

The reason for this is imagine you have a report you want to reload into an app, and you reload the app and pull the data from say rice and then import your report - if the data has changed in the underlying system you need to know about it as otherwise as you add more to your report you get some of your report with the old data and some with the new data

kpagacz commented 2 years ago

I think we need the hashes of the data as well i.e.

data_hashes : { "ADSL" : hash } , {"ADTTE", : hash}

Or some other structure

The reason for this is imagine you have a report you want to reload into an app, and you reload the app and pull the data from say rice and then import your report - if the data has changed in the underlying system you need to know about it as otherwise as you add more to your report you get some of your report with the old data and some with the new data

I would assign the hashes to ReportCards and not the whole report to make the distinction on a per-card basis.

nikolas-burkoff commented 2 years ago

I would assign the hashes to ReportCards and not the whole report to make the distinction on a per-card basis.

Yeah that can work

Polkas commented 2 years ago

I think we need the hashes of the data as well i.e.

data_hashes : { "ADSL" : hash } , {"ADTTE", : hash}

Or some other structure The reason for this is imagine you have a report you want to reload into an app, and you reload the app and pull the data from say rice and then import your report - if the data has changed in the underlying system you need to know about it as otherwise as you add more to your report you get some of your report with the old data and some with the new data

I would assign the hashes to ReportCards and not the whole report to make the distinction on a per-card basis.

I just have a talk with @nikolas-burkoff about this topic. I could add them for a validation, possible to give some warning too. Still I think we should enable to mix cards from different sessions with different data setups.