Feature/alcs 2325 Tag and category implementation #1939
Conversation
fbarreta
commented
Oct 28, 2024
•
fbarreta
commented
- Backend module refactor (Created tag module)
- Implementation of Tag and Tag Categories on ALCS Admin.
fbarreta
changed the title
| this.showNameWarning = true; | ||
| this.name = ''; | ||
| } else { | ||
| throw e; |
There was a problem hiding this comment.
When you're catching the error and it's not meeting your condition you're throwing it again, where is it being handled in the upstream components?
| } | ||
|
|
||
| private handleError(e: any) { | ||
| const res = e as HttpErrorResponse; |
There was a problem hiding this comment.
if the error is a HttpErrorResponse, then shouldn't the handling be done at the service level? That's the responsibility of the services to deal with the networking.
| if (res.error.statusCode === HttpStatusCode.Conflict && res.error.message.includes('update or delete on table')) { | ||
| this.toastService.showErrorToast('Category is associated with tags. Unable to delete.'); | ||
| } else { | ||
| throw e; |
| this.showNameWarning = true; | ||
| this.name = ''; | ||
| } else { | ||
| throw e; |
| if (res.error.statusCode === HttpStatusCode.Conflict && res.error.message.includes('update or delete on table')) { | ||
| this.toastService.showErrorToast('Tag is associated with files. Unable to delete.'); | ||
| } else { | ||
| throw e; |
| try { | ||
| return await firstValueFrom(this.http.delete<TagCategoryDto>(`${this.url}/${code}`)); | ||
| } catch (e) { | ||
| throw e; |
| "bracketSpacing": true, | ||
| "endOfLine": "auto" | ||
| } |
There was a problem hiding this comment.
can you please explain about the changes here?
There was a problem hiding this comment.
This endOfLine was just freaking me out on my VSCode and fixed my CR/CRLF warnings hope it won't cause any harm, and added some of the same bracketSpacing as our frontends.
| export class TagController { | ||
| constructor(private service: TagService) {} | ||
|
|
||
| @Get('/:pageIndex/:itemsPerPage') |
There was a problem hiding this comment.
In pagination and filtering, query parameters would be used instead of URL parameters.
/?pageIndex=1&itemsPerPage=10
| const msg = (e as QueryFailedError).message; | ||
| throw new HttpException(msg, HttpStatus.CONFLICT); | ||
| } else { | ||
| throw e; |
There was a problem hiding this comment.
What will happen if we just throw the error here?
| @@ -0,0 +1,10 @@ | |||
| @use 'sass:math'; | |||
|
|
|||
| @function rem($pixels, $context: 16) { | |||
There was a problem hiding this comment.
The reason that we did not put rem function in alcs-frontend was that alcs-frontend would be only used on desktop devices so rem would not be useful.
| this.showNameWarning = false; | ||
| } | ||
|
|
||
| private handleError(e: any) { |
There was a problem hiding this comment.
Really not a fan of any. It can lead to so many issues. Could we try to resolve this type up front? It seems like you're already expecting it to be a HttpErrorResponse, could we use this as the param type?
There was a problem hiding this comment.
There are a few other cases of the exact same thing. If you figure out a good solution, please apply to other cases.
