IDEMSInternational / open-app-builder

PLH App Frontend
GNU General Public License v3.0
5 stars 24 forks source link

fix: data items are sorted according to original order by default #2312

Closed jfmcquade closed 1 month ago

jfmcquade commented 2 months ago

PR Checklist

Description

As documented in #2311, item rows within a data-items loop are sorted according to their ID value by default. Whilst we do have a sort operation that can be applied to the items via the parameter list of the data-items component, which can be used to sort the list based on some property of the items, there is currently no way to reference the original order of the items in the data list.

This PR makes it the default that data-items render in the order that matches the original data list.

Git Issues

Closes #2311

Screenshots/Videos

Template debug_data_items_order, reading from debug_data_list_order showing items displayed in the expected order.

Screenshot 2024-05-02 at 16 38 00 Screenshot 2024-05-02 at 16 38 16

Passing tests:

Screenshot 2024-05-09 at 12 29 47
jfmcquade commented 1 month ago

Thanks @chrismclarke, I've acted on your suggestions.

  1. We may also want to consider marking the column as a final column type to prevent user writes to the column, although should first test if we're still able to populate and repopulate data on initial load with updated index (possibly just consider later).

This does seem to work and still allow us to update the index if the data changes. See demo below (with some waiting time cut out): I make a user write over some data, then change the order of that data (changing the index for each item) and make additional writes. You can see that the row_index property remains the same until a write is made specifically to that item, at which point it successfully updates to reflect the new order. Does this cover the case you were thinking of? I'm not quite sure how to put this into a spec test, but I've added one that confirms the row_index property cannot be overridden via the DynamicDataService.update() method. Are there any more cases related to setting the column to final that might not be covered?

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/da1cb9f5-dcf1-463d-939c-0d0800a8dec2

chrismclarke commented 1 month ago

This does seem to work and still allow us to update the index if the data changes. See demo below (with some waiting time cut out): I make a user write over some data, then change the order of that data (changing the index for each item) and make additional writes. You can see that the row_index property remains the same until a write is made specifically to that item, at which point it successfully updates to reflect the new order. Does this cover the case you were thinking of?

Great, yeah that works really nicely, because the row_id is a top-level property and only the inner data property is persisted/merged then it means we will always preserve the row_index from parsing the sheet. I also noticed in the rxdb code that the final field is only enforced in dev mode (guessing performance reasons, seems reasonable to me given we're more likely to explore implementation during dev anyways), so production is a little more forgiving anyways.

I'm not quite sure how to put this into a spec test, but I've added one that confirms the row_index property cannot be overridden via the DynamicDataService.update() method. Are there any more cases related to setting the column to final that might not be covered?

I think that's sufficient, our spec tests cover the metadata population and inner data merge so would be a bit artificial adding tests right now for things that don't exist/won't happen. I manually tested migration switching between branches, had looked at adding a spec test but decided would be more effort than its worth as we'd need to totally decouple the schema/migration files from the service code (to be able to replace it more easily), which doesn't seem worthwhile when we're unlikely to change these much again in the future.