Skip to content
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

feat: contribution zod #1056

Merged
merged 39 commits into from
Oct 9, 2023
Merged

feat: contribution zod #1056

merged 39 commits into from
Oct 9, 2023

Conversation

Viczei
Copy link
Contributor

@Viczei Viczei commented Oct 4, 2023

No description provided.

@Viczei Viczei self-assigned this Oct 4, 2023
@gitguardian
Copy link

gitguardian bot commented Oct 4, 2023

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
8246187 Generic High Entropy Secret db42d1a .npmrc View secret
8246187 Generic High Entropy Secret 999b06d .npmrc View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

</Box>
<Box sx={{ width: "30%" }}>
{answer && (
{answer?.id && answer?.statuses && (
Copy link
Contributor

@carolineBda carolineBda Oct 6, 2023

Choose a reason for hiding this comment

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

On peut avoir dans commentaire sans avoir des statuses non ?

Copy link
Contributor

Choose a reason for hiding this comment

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

En effet, il faut plutôt gérer le cas en dessous comme pour les comments :

<Comments
              answerId={answer.id}
              comments={answer.answerComments ?? []}
              statuses={answer.statuses ?? []}
            />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oui dans ce cas, il attend un tableau vide. La vérif check uniquement qu'il ne soit pas null ou undefined

@@ -30,7 +30,9 @@ function concatAndSort(
})[] = [...comments, ...statuses];
return all
.map((notif) => {
notif.createdAtDate = new Date(notif.createdAt);
notif.createdAtDate = notif?.createdAt
Copy link
Contributor

Choose a reason for hiding this comment

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

c'est pas possible de ne pas avoir de createdAd. tu as eu le cas ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pas dans les contrib, c'est vrai. Mais dans les pages info j'ai le cas ou l'on commence à créer une page et du coup il n'y a pas de createdAt encore généré. Je ne sais pas si plus tard, on aura ce cas ici. Je peux le remettre en obligatoire dans zod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah non je mélange avec le createdAt de answer. Dans tous les cas, je vais mettre tous nos createdAt en obligatoire dans les types contrib. Ca sera mieux en effet.


const StyledFormHelperText = styled(FormHelperText)(({ theme }) => {
return {
color: theme.palette.error.main,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -54,9 +56,20 @@ export const FormRadioGroup = ({
/>
))}
</RadioGroup>
{error && error.message === "Required" ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{error && error.message === "Required" ? (
{error && error.message === "Required" ?? (

<StyledFormHelperText>
Un élément doit être sélectionner
</StyledFormHelperText>
) : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
) : null}
)}

@Viczei Viczei temporarily deployed to review-auto October 6, 2023 10:06 — with GitHub Actions Inactive
</Box>
<Box sx={{ width: "30%" }}>
{answer && (
{answer?.id && answer?.statuses && (
Copy link
Contributor

Choose a reason for hiding this comment

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

En effet, il faut plutôt gérer le cas en dessous comme pour les comments :

<Comments
              answerId={answer.id}
              comments={answer.answerComments ?? []}
              statuses={answer.statuses ?? []}
            />

Comment on lines 72 to 74
kaliReferences: data.id
? formatKaliReferences(data.id, data.kaliReferences)
: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Comme on ne peut pas créer des réponses mais seulement en éditer, on doit toujours avoir un id aussi.

Comment on lines +44 to +46
const aggregatedRow = rows.flatMap(({ answers }) =>
answers?.length ? (answers as Answer[]) : []
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Etrange, on ne peut pas avoir de question sans liste de réponse, pour on vérifie que c'est un tableau ici ?

@@ -22,7 +23,10 @@ export const QuestionRow = (props: { row: QueryQuestion }) => {
<TableCell component="th" scope="row">
<strong>{row.order}</strong> - {row.content}
</TableCell>
<StatusRecap answers={row.answers} uniqKey={`${row.id}-statuses`} />
<StatusRecap
answers={row.answers as Answer[]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi devoir faire un as maintenant ? On perd en typage ici 🤔

@@ -37,15 +37,15 @@ export const EditQuestionAnswerList = ({
{answers?.map((answer) => {
return (
<TableRow
key={answer.agreement.id}
key={answer?.agreement?.id}
Copy link
Contributor

Choose a reason for hiding this comment

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

comment l'answer peut être undefined ici ?


const answerRelationSchema = answerBaseSchema.extend({
agreement: agreementSchema.optional(),
statuses: z.array(answerStatusSchema).optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

On n'a pas une liste de status même vide ?

otherReferences: z.array(otherReferenceSchema),
cdtnReferences: z.array(cdtnReferenceSchema),
contentFichesSpDocument: documentSchema.nullable().optional(),
question: questionBaseSchema.optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

On a obligatoirement un question associée

cdtnReferences: z.array(cdtnReferenceSchema),
contentFichesSpDocument: documentSchema.nullable().optional(),
question: questionBaseSchema.optional(),
answerComments: z.array(commentsSchema).optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem, mais la liste peut être vide non ?

});

export const questionRelationSchema = questionBaseSchema.extend({
answers: z.array(answerBaseSchema.deepPartial()).optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Une question à obligatoriement une liste de réponse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sauf si on l'inclus pas dans le graphql

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je pense qu'il faut bien distinguer les typages de backend et les typages de frontend

Comment on lines 17 to 18
_createdAt &&
createdAt &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Oui, une référence ne peut pas avoir de createdAt null

@Viczei Viczei temporarily deployed to review-auto October 6, 2023 12:38 — with GitHub Actions Inactive
@Viczei Viczei temporarily deployed to review-auto October 6, 2023 13:22 — with GitHub Actions Inactive
@Viczei Viczei temporarily deployed to review-auto October 6, 2023 14:49 — with GitHub Actions Inactive
Victor Zeinstra and others added 3 commits October 6, 2023 16:50
* feat: contrib answer CC hover + CC search + delete question

* fix: permettre accordéon dans accordéon

* chore: review

* chore: fix test

---------

Co-authored-by: Victor Zeinstra <[email protected]>
@m-maillot m-maillot temporarily deployed to review-auto October 9, 2023 07:22 — with GitHub Actions Inactive
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

🎉 Deployment for commit 3ac2d7a :

Ingresses
Docker images
  • 📦 docker pull harbor.fabrique.social.gouv.fr/cdtn/cdtn-admin/contributions:sha-3ac2d7aece02e04f356b0a0caea8cd2cc39e36f3
  • 📦 docker pull harbor.fabrique.social.gouv.fr/cdtn/cdtn-admin/export:sha-3ac2d7aece02e04f356b0a0caea8cd2cc39e36f3
  • 📦 docker pull harbor.fabrique.social.gouv.fr/cdtn/cdtn-admin/frontend:sha-3ac2d7aece02e04f356b0a0caea8cd2cc39e36f3
  • 📦 docker pull harbor.fabrique.social.gouv.fr/cdtn/cdtn-admin/hasura:sha-3ac2d7aece02e04f356b0a0caea8cd2cc39e36f3
Debug

@m-maillot m-maillot merged commit 6f8021f into master Oct 9, 2023
@m-maillot m-maillot deleted the feat/contribution-zod branch October 9, 2023 09:04
m-maillot added a commit that referenced this pull request Oct 9, 2023
@m-maillot m-maillot restored the feat/contribution-zod branch October 9, 2023 09:57
@m-maillot m-maillot temporarily deployed to review-auto October 9, 2023 09:57 — with GitHub Actions Inactive
@m-maillot m-maillot deleted the feat/contribution-zod branch October 9, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants