-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: new sdk frontend code refactor part 4 #1647
refactor: new sdk frontend code refactor part 4 #1647
Conversation
…ontend into SWAP-4278-new-sdk-frotnend-code-refactor-part-4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @martin-trajanovski - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider splitting large refactoring PRs like this into smaller, more focused changes to make review easier - while this one is cohesive enough to be acceptable, smaller PRs reduce review burden
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -44,11 +47,10 @@ export class AdminTabComponent implements OnInit, OnDestroy { | |||
.pipe(take(1)) | |||
.subscribe((user) => { | |||
if (user && this.dataset) { | |||
let job: JobClass; | |||
let job: CreateJobDto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Initialize the job object properly to avoid potential runtime errors
The job object is declared but not initialized. Consider using an object literal to properly initialize all required fields of CreateJobDto.
this.datasetsService.datasetsControllerFullquery(query, limits).pipe( | ||
map((datasets) => | ||
fromActions.fetchDatasetsCompleteAction({ datasets }), | ||
this.datasetsService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider centralizing JSON serialization logic in the service layer rather than scattering it across effects
The scattered JSON.stringify()
calls throughout the effects make the code harder to maintain. Consider moving JSON serialization into the service layer:
// In DatasetsService:
datasetsControllerFullquery(limits: any, query: any) {
return this.http.post<Dataset[]>(
`${this.baseUrl}/Datasets/fullquery`,
{
limits: JSON.stringify(limits),
query: JSON.stringify(query)
}
);
}
// Then effects become cleaner:
fetchDatasets$ = createEffect(() => {
return this.actions$.pipe(
// ...
mergeMap(({ query, limits }) =>
this.datasetsService
.datasetsControllerFullquery(limits, query)
.pipe(
map((datasets) => fromActions.fetchDatasetsCompleteAction({ datasets })),
catchError(() => of(fromActions.fetchDatasetsFailedAction())),
),
),
);
});
This centralizes the serialization logic in the service layer where the API is called, making the effects simpler and more consistent.
@@ -2,7 +2,7 @@ | |||
<ng-container *ngIf="datablocks$ | async as datablocks"> | |||
<h3>Datablocks: {{ datablocks.length }}</h3> | |||
<ul *ngFor="let block of datablocks"> | |||
<li>{{ block.id }}</li> | |||
<li>{{ block._id }}</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use the field id
, which is consistent with the data model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I see this but in the backend we are not exposing the id
as @ApiProperty
in the response we only have _id
defined and I wasn't sure if this was by purpose or we need to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked that we don't even have the id property in the datablock schema in the backend. Here it is defined: https://github.com/SciCatProject/scicat-backend-next/blob/master/src/datablocks/schemas/datablock.schema.ts#L26. So maybe this is a bigger issue to address on the backend and define it. But we will need a database migration as well to copy the _id
field to the new id
as well and expose it.
Description
Short description of the pull request
Motivation
Background on use case, changes needed
Fixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
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
Refactor the frontend code to improve type safety and alignment with the backend by replacing DatasetClass with OutputDatasetObsoleteDto and JobClass with CreateJobDto. Remove incorrect type assertions and use JSON.stringify for parameters in effects. Update package dependencies to the latest versions.
Enhancements:
Chores: