e-picsa / picsa-apps

Monorepo for building tools to support E-PICSA Apps
https://picsa.app
GNU General Public License v3.0
6 stars 2 forks source link

feat(dashboard): add monitoring pages and forms db #231

Closed khalifan-kfan closed 7 months ago

khalifan-kfan commented 7 months ago

Screenshots (updated)

localhost_4200_monitoring(720p) localhost_4200_monitoring(720p) (1) localhost_4200_monitoring(720p) (2)


Description

Add monitoring forms display pages and database schemas

Discussion

218

Extra information and queries

  1. @chrismclarke could you help confirm whether am on the right track and if the schema so far is fine. To be more specific, 1.1 Should I link the cover item to storage by foreign key? 1.2 For data such as appCountries and appSummary, should I make them relations instead of embedded data in the data base? 1.3 Have I represented the enketoDefinition field as required?

    2.I have attempted to use the picsa-data-table component but as you can see in the screenshot, some fields are presented in a specific type of way, any pointers on what I should put into consideration to effect it?

Screenshots / Videos

Screenshot 2024-02-13 at 14 36 10
chrismclarke commented 7 months ago

Thanks @khalifan-kfan , looks like a really good start, let me do my best to address the questions you raised:

Extra information and queries @chrismclarke could you help confirm whether am on the right track and if the schema so far is fine. To be more specific, 1.1 Should I link the cover item to storage by foreign key? 1.2 For data such as appCountries and appSummary, should I make them relations instead of embedded data in the data base? 1.3 Have I represented the enketoDefinition field as required?

Overall schema looks good, although one minor consideration is to change column names to use snake_case instead of camelCase (e.g. enketoDefinition -> enketo_definition). This is purely a matter of convention and isn't super significant, just more common for databases to use snake_case and javascript camelCase. It does then mean some incompatibility if writing the same definitions in both languages, but usually not a big issue as we have automated type generation from the database to use in the code instead of manually typing.

1.1 - I think given that it's an svg actually might be easier to just inline as text and avoid having to integrate storage buckets at this point

1.2 - I think I need to have a more considered think generally about how we're going to manage what data appears in what version of the app. I think for now what you have done is fine and makes sense to just keep as hardcoded per form

1.3 - Yes you have, the field is JSON so using JSONB data type is a good choice. Whilst it would be possible to store as a string, that would add an extra step to parse/stringify when interacting with, and by using JSONB it also makes the data inside the JSON searchable via SQL if we ever wanted to do that

2.I have attempted to use the picsa-data-table component but as you can see in the screenshot, some fields are presented in a specific type of way, any pointers on what I should put into consideration to effect it?

Good point, as mentioned on the call I reached the same limitation myself so I've updated the picsa-data-table component to provide functionality to override display for a specific cell. I've provided an example by duplicating the table and pushed as commit ee24b72. Let me know if the changes make sense (really I should add some documentation about the component more generally... will put on a todo list). A good challenge would be to see if you can extend the example to format the rest of the columns you have custom displays for, e.g. created_at date pipe and appCountries array join.

chrismclarke commented 7 months ago

I'll revert the PR back to draft for now, but let me know if you have any other questions or clarifications, and when you think next ready for a code review

khalifan-kfan commented 7 months ago

Thanks for the response @chrismclarke, should I enable all crud operations to the monitoring forms table in the UI or I should leave it as read only

chrismclarke commented 7 months ago

Thanks for the response @chrismclarke, should I enable all crud operations to the monitoring forms table in the UI or I should leave it as read only

I think for now probably just leaving readonly - I expect admin would probably like a way to make changes to form submissions in the future, although this will need to be considered a bit more carefully to ensure that any changes admin make don't just get replaced if the user updates the submission from their side (incoming data will always take priority) - so we need a way to ensure any admin changes also get synced back to the user device, and will need to establish a conflict resolution strategy if both an admin and a user try to push differences to the same form submission

chrismclarke commented 7 months ago

I've removed myself as a reviewer just so I don't get notified as you push commits (annoying github feature), but feel free to tag me when ready for review or if you want me to look at anything else

khalifan-kfan commented 7 months ago

Hey @chrismclarke, you could now take a look and let me know on what I can change

khalifan-kfan commented 7 months ago

Thanks for the review @chrismclarke, let me look at another issue to take on