Closed Nil2000 closed 1 month ago
β±οΈ Estimated effort to review: 3 π΅π΅π΅βͺβͺ |
π§ͺ PR contains tests |
π No security concerns identified |
β‘ Key issues to review Possible Typo There's a potential typo in the `enivronment` property name at line 19. It should be `environment`. Possible Typo There's a potential typo in the `enivronment` property name at line 19. It should be `environment`. Possible Typo There's a potential typo in the `enivronment` property name at line 113. It should be `environment`. |
Category | Suggestion | Score |
Best practice |
Use a constant or enum for field names to improve code maintainability___ **Consider using a constant or enum for the 'slug' property name to improvemaintainability and reduce the risk of typos.** [apps/api/src/variable/service/variable.service.ts [124-128]](https://github.com/keyshade-xyz/keyshade/pull/468/files#diff-db1895137a9c036373abefb8c924150d4f2022fd6d4f5271cc6c95c443bf01cfR124-R128) ```diff environment: { select: { - slug: true + [EnvironmentFields.SLUG]: true } }, ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 7 |
Maintainability |
Extract repeated selection object into a constant to reduce duplication___ **Consider extracting the repeated environment selection object into a reusableconstant to reduce code duplication.** [apps/api/src/variable/service/variable.service.ts [253-257]](https://github.com/keyshade-xyz/keyshade/pull/468/files#diff-db1895137a9c036373abefb8c924150d4f2022fd6d4f5271cc6c95c443bf01cfR253-R257) ```diff -environment: { - select: { - slug: true - } -}, +environment: ENVIRONMENT_SELECT_OBJECT, ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 7 |
Possible bug |
β Correct the spelling of a property name to ensure accurate typing___Suggestion Impact:The typo in the property name 'enivronment' was corrected to 'environment', as suggested. code diff: ```diff - enivronment: { + environment: { ```typing and avoid potential bugs.** [packages/api-client/src/types/variable.types.d.ts [19-21]](https://github.com/keyshade-xyz/keyshade/pull/468/files#diff-a5cc3a8d29899e24783200cd22d84f671e390b3fb0efc276f85840fc6d5ed0f3R19-R21) ```diff -enivronment: { +environment: { slug: string } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 7 |
Possible issue |
β Fix a typo in a test assertion to ensure it correctly checks the property___Suggestion Impact:The typo in the property name 'enivronment' was corrected to 'environment' in the test assertion, as suggested. code diff: ```diff - expect(variable.data.versions[0].enivronment.slug).toBe(environment.slug) + expect(variable.data.versions[0].environment.slug).toBe(environment.slug) ```corrected type definition and ensure the test passes.** [packages/api-client/tests/variable.spec.ts [113]](https://github.com/keyshade-xyz/keyshade/pull/468/files#diff-44c34cd8f05455a5232b41d07eb6770f8a7c179bd2302fff90ba968c44690de2R113-R113) ```diff -expect(variable.data.versions[0].enivronment.slug).toBe(environment.slug) +expect(variable.data.versions[0].environment.slug).toBe(environment.slug) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 7 |
π‘ Need additional feedback ? start a PR chat
Hi @rajdip-b , Require some help here.
Should this be environment-2-0
since we are passing environment2.id
in environmentId
field?
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 48.01%. Comparing base (
ce50743
) to head (7f83d64
). Report is 183 commits behind head on develop.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
In places where we are selecting both environmentId and environment slug, its better to include it by using
environment.id
andenvironment.slug
:environment: { id: true slug: true }
Ok made these changes but did you notice about the environment2
test mentioned earlier? It is still coming.
This step is failing: https://github.com/keyshade-xyz/keyshade/actions/runs/11092075120/job/30816652494?pr=468#step:10:486
Yeah already mentioned the issue this is always pointing to environment-1-0
slug or Environment 1
though I have pointed that to environment2
. I am unable to get why this is happening. Can it be issue at the mapping part?
@Nil2000 sorry about the delay, i have fixed the error. I would just need you to remove environmentId
and have environment.id
everywhere like i mentioned earlier.
https://github.com/keyshade-xyz/keyshade/issues/486
This is a related issue that you can pick up.
@Nil2000 can you please add this change I mentioned?
@Nil2000 sorry about the delay, i have fixed the error. I would just need you to remove
environmentId
and haveenvironment.id
everywhere like i mentioned earlier.
@Nil2000 can you please add this change I mentioned?
@Nil2000 sorry about the delay, i have fixed the error. I would just need you to remove
environmentId
and haveenvironment.id
everywhere like i mentioned earlier.
Working on it. Would be done by today
@rajdip-b The test issue is still present
:tada: This PR is included in version 2.6.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:
Description
Add
environmentSlug
in multiple places across thevariable
moduleFixes #431
Dependencies
Mention any dependencies/packages used
Future Improvements
Mention any improvements to be done in future related to any file/feature
Mentions
Mention and tag the people
Screenshots of relevant screens
Add screenshots of relevant screens
Developer's checklist
If changes are made in the code:
Documentation Update