feat: 1270 bo agrement documents tabs#1233
Merged
Conversation
revu-bot
reviewed
Mar 24, 2026
Collaborator
revu-bot
left a comment
revu-bot
left a comment
There was a problem hiding this comment.
PR Review: feat/1270 - BO Agrément Documents Tab
This PR adds document display to the "Documents" tab in the BO (Dreets) agrément view, moves the AgrementDocuments component to shared-ui, and refactors the getById repository method to support a withDetails flag. It also fixes a bug in frontend-usagers where agrementCourant was used instead of agrementEnTraitement.
Key Issues Found
| File | Lines | Severity | Issue |
|---|---|---|---|
agrements.service.ts |
22–38 | CRITICAL | Sequential await in loops causes N+1 performance problem |
agrements.service.ts |
30–33 | CRITICAL | Unsafe property access on bilanHebergement.adresse without null guard |
agrements.test.ts |
176–179 | IMPORTANT | Test asserts agrementFiles.length >= 2 but fixture likely creates 0 files — test may be vacuously passing or fragile |
Documents.vue |
28–35 | IMPORTANT | DREETS/OVA split logic based on userId === null is fragile and undocumented |
agrements.repository.ts |
29–136 | IMPORTANT | Large inline SQL with template-literal conditionals is hard to maintain and test |
revu-bot
reviewed
Mar 24, 2026
revu-bot
reviewed
Mar 24, 2026
Comment on lines
+176
to
+179
| expect(response.body.agrement.agrementFiles).toBeDefined(); | ||
| expect(response.body.agrement.agrementFiles.length).toBeGreaterThanOrEqual( | ||
| 2, | ||
| ); |
Collaborator
There was a problem hiding this comment.
[IMPORTANT] Test assertion on agrementFiles.length >= 2 is fragile / likely wrong
buildAgrementFixtureandcreateAgrementare used to create the agrément, but there is no code in the visible test setup that inserts rows intofront.agrement_files.- If the fixture does not create file rows,
agrementFileswill be an empty array[]and[].length >= 2isfalse— the test will fail in CI. - If the fixture does create files, the assertion should be exact (
toBe(2)) so regressions are caught. - Either way, the test should explicitly create the expected file records and assert the exact count.
Action required: Verify that buildAgrementFixture / createAgrement inserts at least 2 agrement_files rows, or add a dedicated helper (e.g. createAgrementFile) and call it twice before the assertion. Replace toBeGreaterThanOrEqual(2) with toBe(2) once the setup is deterministic.
l-scherer
force-pushed
the
feat/1270-bo-agrement-document-tabs
branch
from
228354f to
47939a9
Compare
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
revu-bot
reviewed
Mar 24, 2026
Collaborator
revu-bot
left a comment
revu-bot
left a comment
There was a problem hiding this comment.
PR Review: feat/1270 — BO Agrément Documents Tab
This PR consolidates the getById logic for agréments into a shared repository/service and exposes the Documents tab to the BO (Dreets) side. The refactoring direction is sound, but there are several issues worth addressing.
| File | Lines | Severity | Issue |
|---|---|---|---|
shared/agrements/agrements.service.ts |
19–22 | CRITICAL | getFileMetaData called without error handling; one failure aborts all files |
shared/agrements/agrements.repository.ts |
15 | IMPORTANT | Comment says "Repository Admin" but this is now a shared repository |
shared/agrements/Documents.vue |
30–38 | IMPORTANT | Business logic (DREETS vs OVA split) relies on userId === null with no documentation |
__tests__/admin/agrements.test.ts |
176–179 | IMPORTANT | Test asserts agrementFiles.length >= 2 but fixture creates 0 files — assertion will always fail or is misleading |
usagers/agrements/agrements.repository.ts |
~248 | IMPORTANT | getByOrganismeId still duplicates the full query logic now in the shared repository |
shared/agrements/agrements.service.ts |
26–28 | MINOR | adresse?.id guard added for bilanHebergement but not for agrementSejours in the old code — now fixed, but getFileMetaData still uses non-null assertion doc.fileUuid! |
revu-bot
reviewed
Mar 24, 2026
Comment on lines
+19
to
+24
| await Promise.all( | ||
| (agrement.agrementFiles ?? []).map(async (doc) => { | ||
| const meta = await getFileMetaData(doc.fileUuid!); | ||
| Object.assign(doc, meta); | ||
| }), | ||
| ); |
Collaborator
There was a problem hiding this comment.
[CRITICAL] getFileMetaData failures silently abort all file metadata enrichment
Promise.allwill reject as soon as anygetFileMetaDatacall throws, leaving the entireagrementFilesarray without metadata.doc.fileUuid!uses a non-null assertion — iffileUuidisnull/undefinedin the DB, this will throw at runtime.- The old sequential
for...ofloop had the same problem, but now it's more visible.
Proposed fix: wrap each call in a try/catch so a single bad file doesn't break the whole response.
Suggested change
| await Promise.all( | |
| (agrement.agrementFiles ?? []).map(async (doc) => { | |
| const meta = await getFileMetaData(doc.fileUuid!); | |
| Object.assign(doc, meta); | |
| }), | |
| ); | |
| await Promise.all( | |
| (agrement.agrementFiles ?? []).map(async (doc) => { | |
| if (!doc.fileUuid) return; | |
| try { | |
| const meta = await getFileMetaData(doc.fileUuid); | |
| Object.assign(doc, meta); | |
| } catch (err) { | |
| // log but don't fail the whole request for one bad file | |
| // log.w("getFileMetaData failed for", doc.fileUuid, err); | |
| } | |
| }), | |
| ); |
revu-bot
reviewed
Mar 24, 2026
achorein
reviewed
Mar 24, 2026
l-scherer
force-pushed
the
feat/1270-bo-agrement-document-tabs
branch
from
47939a9 to
7fbc70e
Compare
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
revu-bot
reviewed
Mar 26, 2026
Collaborator
revu-bot
left a comment
revu-bot
left a comment
There was a problem hiding this comment.
PR Review: feat/1270 — BO Agrément Documents Tab
This PR refactors the getById logic for agrements into a shared service/repository layer and exposes the documents tab in the BO (Dreets) interface. The overall approach is sound — deduplication via shared layer is a good architectural move.
Key Issues Found
| File | Lines | Severity | Issue |
|---|---|---|---|
shared/agrements/agrements.service.ts |
19–23 | CRITICAL | getFileMetaData errors are silently swallowed; a single failing file aborts all metadata enrichment |
shared/agrements/agrements.service.ts |
26–35 | IMPORTANT | bh.adresse?.id guard is missing for sejours; original code had no guard either — now partially fixed but inconsistently |
shared/agrements/Documents.vue |
30–38 | IMPORTANT | DREETS/OVA split relies on userId === null which is a fragile, undocumented business rule with no type safety |
__tests__/admin/agrements.test.ts |
249–252 | IMPORTANT | Assertion toBeGreaterThanOrEqual(2) is too loose — it doesn't verify the actual content or structure of agrementFiles |
usagers/agrements/agrements.repository.ts |
257–264 | MINOR | getByOrganismeId still contains the full duplicated query that was just extracted into the shared repository |
revu-bot
reviewed
Mar 26, 2026
Comment on lines
+19
to
+24
| await Promise.all( | ||
| (agrement.agrementFiles ?? []).map(async (doc) => { | ||
| const meta = await getFileMetaData(doc.fileUuid!); | ||
| Object.assign(doc, meta); | ||
| }), | ||
| ); |
Collaborator
There was a problem hiding this comment.
[CRITICAL] getFileMetaData errors abort all file enrichment silently
Promise.allwill reject on the first failinggetFileMetaDatacall, leaving the entireagrementFilesarray un-enriched and propagating an unhandled rejection to the caller.- The original
usagersservice used a sequentialfor...ofloop which had the same problem, but now this affects both BO and usager paths. - Each file's metadata fetch should be isolated so one failure doesn't block the others.
Suggested change
| await Promise.all( | |
| (agrement.agrementFiles ?? []).map(async (doc) => { | |
| const meta = await getFileMetaData(doc.fileUuid!); | |
| Object.assign(doc, meta); | |
| }), | |
| ); | |
| await Promise.all( | |
| (agrement.agrementFiles ?? []).map(async (doc) => { | |
| try { | |
| const meta = await getFileMetaData(doc.fileUuid!); | |
| Object.assign(doc, meta); | |
| } catch (err) { | |
| log.w(`getFileMetaData failed for uuid ${doc.fileUuid}`, err); | |
| } | |
| }), | |
| ); |
revu-bot
reviewed
Mar 26, 2026
Comment on lines
+249
to
+252
| expect(response.body.agrement.agrementFiles).toBeDefined(); | ||
| expect(response.body.agrement.agrementFiles.length).toBeGreaterThanOrEqual( | ||
| 2, | ||
| ); |
Collaborator
There was a problem hiding this comment.
[IMPORTANT] Test assertion is too weak to catch regressions
toBeGreaterThanOrEqual(2)only checks that some files exist; it does not verify the shape of the returned objects, nor that the DREETS/OVA split (the core feature of this PR) is correct.- If the mapper silently drops
userId, the Documents tab will mis-categorise every file but this test will still pass. - Suggested additions:
- Assert that at least one file has
userId === null(DREETS) and at least one hasuserId !== null(OVA). - Assert that each file has the expected fields (
id,fileUuid,category).
- Assert that at least one file has
Suggested change
| expect(response.body.agrement.agrementFiles).toBeDefined(); | |
| expect(response.body.agrement.agrementFiles.length).toBeGreaterThanOrEqual( | |
| 2, | |
| ); | |
| expect(response.body.agrement.agrementFiles).toBeDefined(); | |
| expect(response.body.agrement.agrementFiles.length).toBeGreaterThanOrEqual(2); | |
| const dreetsFile = response.body.agrement.agrementFiles.find( | |
| (f: { userId: number | null }) => f.userId === null, | |
| ); | |
| const ovaFile = response.body.agrement.agrementFiles.find( | |
| (f: { userId: number | null }) => f.userId !== null, | |
| ); | |
| expect(dreetsFile).toBeDefined(); | |
| expect(ovaFile).toBeDefined(); | |
| expect(dreetsFile).toHaveProperty('fileUuid'); | |
| expect(dreetsFile).toHaveProperty('category'); |
l-scherer
force-pushed
the
feat/1270-bo-agrement-document-tabs
branch
from
833fe7d to
f461621
Compare
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
l-scherer
temporarily deployed
to
build-review-auto
GitHub Actions
Inactive
|
|
🎉 Deployment for commit f461621 : Ingresses
Docker images
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket(s) lié(s)
1270
Description
Alimentation de l'onglet "Documents" côté Dreets
Screenshot / liens loom
Check-list
console.log<script lang="ts">Testing instructions
Se connecter en BO avec un compte régional
- [ ] Aller dans le menu Agréments
- [ ] Filtrer pour trouver un agrément possédant des fichiers
- [ ] Cliquer sur "Voir"
- [ ] Cliquer sur l'onglet "Documents"
- [ ] Les documents doivent apparaitre et être téléchargeables. Les sont scindés en "DREETS" et "OVA"