Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
DatasetsListService.deletedCondition, you can simplify the implementation to directly return the boolean expression (return dataset.datasetlifecycle.archiveStatusMessage === 'deleted';) instead of using anifstatement. - Now that
setFiltersActionacceptsPartial<DatasetFilters>, it may be worth tightening usage by explicitly specifying which fields are expected in each call site (e.g., via helper functions or typed wrappers), to avoid accidentally omitting or overwriting critical filter properties over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DatasetsListService.deletedCondition`, you can simplify the implementation to directly return the boolean expression (`return dataset.datasetlifecycle.archiveStatusMessage === 'deleted';`) instead of using an `if` statement.
- Now that `setFiltersAction` accepts `Partial<DatasetFilters>`, it may be worth tightening usage by explicitly specifying which fields are expected in each call site (e.g., via helper functions or typed wrappers), to avoid accidentally omitting or overwriting critical filter properties over time.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Address sourcery comment
nitrosx
left a comment
There was a problem hiding this comment.
It looks good to me.
I have two concerns:
- the archive workflow is hard coded
- the dataset lifecycle management
but we can discuss and brainstorm about them at a later stage
I have been playing a bit around with configurable-actions (see #2350 and #2351) and there I replaced the hardcoded actions in batch and the dataset ones with a config. Doing that, I also thought we could replace these tiles with another configurable action, but I was debating since the config would become massive and we would loose all the ngrx store capabilites (e.g. the breadcrumb would break). But probably something to discuss further. Maybe another small comment is that this PR adds to something already hardcoded, so if we want to remove the hardcoded part maybe it should be at a later stage (essentially I agree with your comment). Maybe an easy win would be to provide via the config the list of datasetlifecycle values we are displaying in the tiles and that would be mirrored in the GET filter (to keep the config slim)
what do you mean here? |
Description
It adds a deleted view fetching from "datasetLifecycle.archiveStatusMessage": "deleted", when jobs are enabled
It also improves the breadcrumb removing code duplication and reusing the RxJS reducer as well as avoiding to override the skip
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Add a deleted archive view mode for datasets and reuse centralized filter state when navigating via breadcrumbs and archive mode controls.
New Features:
Enhancements:
Tests: