opensafely-core / airlock

Other
1 stars 0 forks source link

Improve re-adding updated file flow #552

Closed bloodearnest closed 3 weeks ago

bloodearnest commented 1 month ago

Currently, we can re-add the file, but you could potentially choose a different filetype or group, which is likely to cause issues.

We should specialise the add file flow in this case to pre-choose the group/type,

madwort commented 1 month ago

nb. due to the implementation of updating a file (i.e. delete & re-add), the current behaviour is that a file will be removed from its current group & lose its current filetype, and only have the new info.

This could cause issues with loosely coupled data, though (such a group comments that mention a filename), so we should do this.

madwort commented 1 month ago

Actually, I think this could get complicated when the add_files dialogue is triggered from a multiselect.

Update file from a file view

In this case, we can rework the add_files dialogue so that the updated file is forced to use the existing details (output/supporting file, group membership).

Update file(s) from a directory view / multiselect

Adding files

The current intended workflow is that you can select multiple files, then select whether each file is output/supporting, and then select the one group that all files will be added to. This is to support researchers adding files in batches to a group.

Updating files

At the moment, you can select a mix of files to add/update, and updated files will be removed from their current group & added to the new group selected in the add_file dialogue. If we want to keep the updated files in their existing groups, I'm not sure how we should rework the dialogue to do that.

madwort commented 1 month ago

In the interim, it's tempting to rework the BLL code to ensure that an updated file keeps the same details (filetype & group) as the previous version - I think if a researcher wanted to change those they should do that more explicitly via withdraw & re-add?

rebkwok commented 1 month ago

I think it's reasonable for updating to require that the file keeps its existing details. You're updating the file because its content has changed, if you want to change its group/type, you'd need to do it the same way as you would for a file that hasn't been updated (i.e. withdraw/re-add).

How about we make the multiselect modal only do one thing? So you can select multiple file to add, or you can select multiple files to update, but you can't update files with the "add files" button. Possibly the action needs to become a drop down so you choose the thing you want to do with the files you've multiselected, and we have one button that does the action (similar to actions on a django admin list view).

bloodearnest commented 1 month ago

I think it's reasonable for updating to require that the file keeps its existing details.

I would state it even more strongly that that - its a hard requirement. The reason is that the previous rounts votes and comments, and the mental context for the output checkers, is linked to that file in that group. Allowing user's to change it feels wrong.

You're updating the file because its content has changed, if you want to change its group/type, you'd need to do it the same way as you would for a file that hasn't been updated (i.e. withdraw/re-add).

I agree.

How about we make the multiselect modal only do one thing? So you can select multiple file to add, or you can select multiple files to update, but you can't update files with the "add files" button. Possibly the action needs to become a drop down so you choose the thing you want to do with the files you've multiselected, and we have one button that does the action (similar to actions on a django admin list view).

This was the way the multiselect was intended to be used, with "add files" and "update files" being different conceptual actions with different buttons. I see updating existing files as a fundamentally different thing from a UX pov to adding a new files.

Another option is to not support update files in multiselect at all, and just in single file view. Adding multiselect update file could be future ticket.

madwort commented 1 month ago

I think we're in agreement - I'll start by removing the group & type params from the update method here: https://github.com/opensafely-core/airlock/pull/586/files (still need to fix tests) & then think about UI

madwort commented 1 month ago

Another option is to not support update files in multiselect at all, and just in single file view. Adding multiselect update file could be future ticket.

I think let's do this - https://github.com/opensafely-core/airlock/pull/595

EDIT: follow-up ticket is https://github.com/opensafely-core/airlock/pull/596 on the triage pile

madwort commented 1 month ago

the group/filetype dialogue now has no effect when used from a file, although we should also hide it from the user.