-
Notifications
You must be signed in to change notification settings - Fork 1
sync dev to master #1
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
Conversation
| command: | | ||
| sudo apt update | ||
| sudo apt install -y jq python3-pip | ||
| sudo pip3 install awscli --upgrade |
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.
[maintainability]
Using sudo pip3 install can lead to permission issues and conflicts with system packages. Consider using a virtual environment or a tool like pipenv to manage Python dependencies.
| install_deploysuite: &install_deploysuite | ||
| name: Installation of install_deploysuite. | ||
| command: | | ||
| git clone --branch v1.4.19 https://github.com/topcoder-platform/tc-deploy-scripts ../buildscript |
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.
[correctness]
Cloning a specific branch from a repository without pinning to a commit hash can lead to unexpected changes if the branch is updated. Consider using a commit hash to ensure consistency.
| - setup_remote_docker | ||
| - run: *install_dependency | ||
| - run: *install_deploysuite | ||
| - run: docker buildx build --no-cache=true --build-arg RESET_DB_ARG=<<pipeline.parameters.reset-db>> --build-arg SEED_DATA_ARG=${DEPLOYMENT_ENVIRONMENT} -t ${APPNAME}:latest . |
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.
[💡 performance]
Using --no-cache=true in the Docker build command can significantly increase build times. Ensure this is necessary for your use case, as it forces a full rebuild every time.
| filters: | ||
| branches: | ||
| only: | ||
| - master No newline at end of file |
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.
[💡 style]
The file is missing a newline at the end, which is a common convention to avoid issues with some tools and version control systems.
| with: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # The GITHUB_TOKEN is there by default so you just need to keep it like it is and not necessarily need to add it as secret as it will throw an error. [More Details](https://docs.github.com/en/actions/security-guides/automatic-token-authentication#about-the-github_token-secret) | ||
| LAB45_API_KEY: ${{ secrets.LAB45_API_KEY }} | ||
| exclude: '**/*.json, **/*.md, **/*.jpg, **/*.png, **/*.jpeg, **/*.bmp, **/*.webp' # Optional: exclude patterns separated by commas No newline at end of file |
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.
[💡 style]
Consider adding a newline at the end of the file to adhere to POSIX standards and improve compatibility with various tools and systems.
| import { M2MService } from 'src/shared/modules/global/m2m.service'; | ||
|
|
||
| const ADMIN_GROUP_FIELDS = ['status']; | ||
| const ADMIN_GROUP_FIELDS: string[] = []; |
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.
[correctness]
The ADMIN_GROUP_FIELDS constant is now an empty array. If this change is intentional, ensure that any logic relying on this constant is updated accordingly. If not, consider restoring the previous values to avoid potential issues with admin field omission.
| } | ||
|
|
||
| if (criteria.oldId) { | ||
| prismaFilter.where.oldId = criteria.oldId; |
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.
[❗❗ correctness]
The removal of the else block that sets prismaFilter.where.oldId to not: null changes the behavior when criteria.oldId is not provided. Ensure this change is intentional and that the new behavior aligns with the expected logic.
| await checkGroupName(dto.name, '', tx); | ||
|
|
||
| // create group | ||
| const createdBy = authUser.userId ? authUser.userId : '00000000'; |
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.
[💡 maintainability]
The createdBy and createdAt assignments have been extracted to variables. This improves readability and maintainability by avoiding repeated expressions. Ensure that these variables are used consistently throughout the method.
| createdBy, | ||
| createdAt, | ||
| // Initialize updated fields to match created fields on creation | ||
| updatedBy: createdBy, |
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.
[design]
The updatedBy and updatedAt fields are initialized to match the createdBy and createdAt fields on creation. Ensure this behavior is intended and that it aligns with the business logic for newly created groups.
| await checkGroupName(dto.name, '', tx); | ||
|
|
||
| // create group | ||
| const createdBy = authUser.userId ? authUser.userId : '00000000'; |
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.
[maintainability]
The use of a hardcoded fallback value '00000000' for createdBy might not be ideal. Consider using a more meaningful default or handling this case differently to avoid potential confusion or misuse.
|
|
||
| // create group | ||
| const createdBy = authUser.userId ? authUser.userId : '00000000'; | ||
| const createdAt = new Date().toISOString(); |
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.
[correctness]
The createdAt value is generated using new Date().toISOString(). Ensure that the server's timezone settings are consistent and that this format aligns with the rest of your system's date handling to prevent potential discrepancies.
| createdBy, | ||
| createdAt, | ||
| // Initialize updated fields to match created fields on creation | ||
| updatedBy: createdBy, |
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.
[correctness]
Initializing updatedBy and updatedAt to the same values as createdBy and createdAt is logical for creation, but ensure that these fields are properly updated in subsequent operations to reflect actual updates.
| methods: 'GET, POST, OPTIONS, PUT, DELETE, PATCH', | ||
| origin: (requestOrigin, callback) => { | ||
| if (!requestOrigin) { | ||
| return callback(null, false); |
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.
[correctness]
Returning false for a missing requestOrigin might cause issues if there are legitimate requests without an Origin header, such as server-to-server requests. Consider allowing such requests or handling them differently.
| if (isAllowedOrigin(requestOrigin)) { | ||
| return callback(null, requestOrigin); | ||
| } | ||
|
|
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.
[💡 maintainability]
Returning false for origins not in the allow list without logging could make debugging difficult. Consider logging the rejected origin for better traceability.
| */ | ||
| export enum UserRole { | ||
| Admin = 'Administrator', | ||
| Admin = 'administrator', |
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.
[❗❗ correctness]
Changing the value of Admin from 'Administrator' to 'administrator' could impact any logic that relies on case-sensitive string comparison. Ensure that all usages of this enum value are updated accordingly to prevent potential bugs.
| ALL_SCOPE_MAPPINGS[scope].forEach((s) => expandedScopes.add(s)); | ||
| while (queue.length > 0) { | ||
| const scope = queue.shift(); | ||
| if (!scope || expandedScopes.has(scope)) { |
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.
[performance]
The queue.shift() operation is O(n) in time complexity because it requires shifting all elements in the array. Consider using a different data structure, such as a linked list, for better performance if the queue is expected to be large.
No description provided.