Fix Dataset Release Page #840
Conversation
nafiz1001
force-pushed
the
fix-readset-release
branch
from
81bae1a
to
beda6ee
Compare
|
|
||
| return Response(self.get_serializer(Dataset.objects.get(pk=pk)).data) | ||
| return Response(status=204) |
There was a problem hiding this comment.
you removed the return of the data that was updated? what is the reasoning?
There was a problem hiding this comment.
The error messages are not caught in the front end (the operation fails silently in the console).
There was a problem hiding this comment.
I just felt it was pointless to respond with a dataset if the frontend is never using it anyway.
I should look into catching the error.
There was a problem hiding this comment.
Well you should at least tell that there was an error ... if you remove the line to set released_by it does fail; it just does not show anything on the frontend side.
| [ReleaseStatus.AVAILABLE]: ReleaseStatus.RELEASED, | ||
| [ReleaseStatus.RELEASED]: ReleaseStatus.BLOCKED, | ||
| [ReleaseStatus.BLOCKED]: ReleaseStatus.RELEASED, | ||
| } as const |
There was a problem hiding this comment.
i dont understand the "as const" at the end of these const defined variables.
There was a problem hiding this comment.
There was a problem hiding this comment.
ouff, im looking into this file and i understand what ulysse meant in terms of complexification of the code.
There was a problem hiding this comment.
Readset release management is complex 🥹
| dispatch(ReadsetTableActions.listPage(1)) | ||
| dispatch(ReadsetTableActions.resetPagedItems()) | ||
| dispatch(ReadsetTableActions.setFixedFilter(createFixedFilter(FILTER_TYPE.INPUT_OBJECT_ID, 'dataset__id', dataset.id.toString()))) | ||
| dispatch(ReadsetTableActions.listPage(1)) |
There was a problem hiding this comment.
careful with this piece of code, i believe this function launches a second api call to fetch the current page list
|
This seem to be working well. I am worried that there is no way even for an admin to reset the status to available. This mean mistakes by PMs will force curations. I would prefer this not being the case. It would be great for admins to have a button to reset all readset in a dataset to available as a way to prevent time consuming curations. |
|
One last thing ... Maybe it might be nice to have a small note to mention the user need to release or block all readsets to save the change. Not sure if people will wonder why they can't save if they just click individual readsets status. |
There was a problem hiding this comment.
Also, i believe i understand the complexity of the task itself, although from it looks like, you took a backend approach to a more or less complicated frontend issue
There was a problem hiding this comment.
i wrote some comments on my side, we can discuss them tomorrow if you want
nafiz1001
force-pushed
the
fix-readset-release
branch
from
ea67435
to
2cbf81f
Compare
No description provided.