Skip to content

feat(gitlab): upstream support for role binding on NestJS#2100

Open
shikanime wants to merge 1 commit intopr1958from
pr2100
Open

feat(gitlab): upstream support for role binding on NestJS#2100
shikanime wants to merge 1 commit intopr1958from
pr2100

Conversation

@shikanime
Copy link
Copy Markdown
Member

@shikanime shikanime commented Apr 16, 2026

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

2 similar comments
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

2 similar comments
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

Comment thread apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts Outdated
externUid: user.externUid,
provider: user.provider,
admin: user.admin,
auditor: user.auditor,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Ptet faire un type dédié "UserDefinition" ? On le répète un peu trop, partout, là.

value: true,
},
})
return result?.value ?? null
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: on throw pas, là ?

Copy link
Copy Markdown
Member Author

@shikanime shikanime Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non, c'est tout à fait volontaire. Une valeur manquante signifie simplement que le flag n'a jamais été modifié côté UI (aka rien de spécial à faire) et c'est tout à fait acceptable.

Comment thread apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts Outdated

await Promise.all(project.members.map(async ({ user }) => {
const gitlabUser = await this.gitlab.upsertUser(user)
const isAdmin = adminRoleId ? user.adminRoleIds?.includes(adminRoleId) : undefined
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Un booléen ne devrait pas être autre chose que true ou false. Je sais que undefined est falsy, mais c'est inconfortable d'avoir un isXXXX qui contient des trucs inattendus.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On l'avais deja vus dans l'ancien server. Le nommage est peut-etre pas correct.

undefine -> garder l'etat actuel
false -> deactiver
true -> activer

Comme l'update se fait sur un role a la fois cote console, on fait de l'update "partiel" de l'etat GitLab.

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

4 similar comments
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Hey !

The security scan report for the current pull request is available here.

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr>
@cloud-pi-native-sonarqube
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

built refactor Refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants