Skip to content

Fix/update collection dto partial#423

Open
AnneGerlach wants to merge 5 commits intoIQSS:developfrom
nfdi4health:fix/update-collection-dto-partial
Open

Fix/update collection dto partial#423
AnneGerlach wants to merge 5 commits intoIQSS:developfrom
nfdi4health:fix/update-collection-dto-partial

Conversation

@AnneGerlach
Copy link
Copy Markdown

What this PR does / why we need it:

  • This PR relaxes the typing of the UpdateCollection use case to allow partial collection updates.

  • Native Dataverse collection updates do not require a fully populated object, only the fields to be changed need to be provided.

  • This change aligns the client typing with that behavior and avoids unnecessary TypeScript warnings in downstream usage.

  • This is a typing-only change with no runtime impact.

Which issue(s) this PR closes:

  • Closes -

Related Dataverse PRs:

  • Depends on -

Special notes for your reviewer:

  • TypeScript typing change only (Partial)

  • No API or runtime behavior changes

Suggestions on how to test this:

Is there a release notes or changelog update needed for this change?:

  • Yes, small changelog entry

Additional documentation:

@pdurbin pdurbin moved this to Ready for Triage in IQSS Dataverse Project Feb 17, 2026
@scolapasta scolapasta moved this from Ready for Triage to Ready for Review ⏩ in IQSS Dataverse Project Feb 17, 2026
@cmbz cmbz added FY26 Sprint 17 FY26 Sprint 17 (2026-02-11 - 2026-02-25) FY26 Sprint 18 FY26 Sprint 18 (2026-02-25 - 2026-03-11) labels Feb 25, 2026
@cmbz cmbz added this to the 6.11 milestone Mar 11, 2026
@cmbz cmbz added the FY26 Sprint 20 FY26 Sprint 20 (2026-03-26 - 2026-04-08) label Mar 27, 2026
@ChengShi-1 ChengShi-1 self-assigned this Apr 3, 2026
@ChengShi-1 ChengShi-1 self-requested a review April 3, 2026 02:34
Copy link
Copy Markdown
Contributor

@ChengShi-1 ChengShi-1 left a comment

Choose a reason for hiding this comment

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

Thank you for the change and your contribution. This is a good point. I tested the native api, and it does support partial updates.

However, I think we may need to do more tweaks, if we want to change it to "partial". createCreateOrUpdateRequestBody() is currently written for create/full-update semantics. Also, the current frontend code and tests also rely on full-update semantics. If we want to support partial updates, we will likely need broader changes across the DTOs, request-building logic, and tests, since the current design is aligned around full replacement.

@ChengShi-1 ChengShi-1 moved this from Ready for Review ⏩ to In Review 🔎 in IQSS Dataverse Project Apr 3, 2026
@ekraffmiller
Copy link
Copy Markdown
Contributor

Thanks for the PR, it would be a good addition to allow this, since it is allowed on the API.
I agree with @ChengShi-1, the current implementation of createCreateOrUpdateRequestBody() needs to change, to allow for partially populated DTO. Also, a test to show that it update is successful with a partially populated DTO. With those changes, I think it could be merged.

@ekraffmiller ekraffmiller added the GREI Re-arch GREI re-architecture-related label Apr 8, 2026
@cmbz cmbz added the FY26 Sprint 21 FY26 Sprint 21 (2026-04-08 - 2026-04-22) label Apr 8, 2026
@ChengShi-1 ChengShi-1 assigned AnneGerlach and unassigned ChengShi-1 Apr 10, 2026
@pdurbin pdurbin removed this from the 6.11 milestone Apr 15, 2026
…rtial<CollectionDTO> and dedicated request builder @createUpdateRequestBody
@AnneGerlach
Copy link
Copy Markdown
Author

AnneGerlach commented Apr 16, 2026

Thanks for the feedback! 🌻
I updated the implementation accordingly.

  • updateCollection now accepts Partial<CollectionDTO>
  • introduced a dedicated createUpdateRequestBody() for partial updates
  • kept createCreateOrUpdateRequestBody() unchanged for create/full-update semantics

For the update case, only explicitly provided fields (!== undefined) are sent, so omitted fields remain unchanged.

The metadataBlocks logic now follows the Dataverse API semantics:

  • only included if relevant fields are provided
  • inheritance flags (inherit...FromParent) are respected and prevent conflicting fields
  • avoids invalid combinations (e.g. metadataBlockNames + inheritMetadataBlocksFromParent=true)

(see: https://guides.dataverse.org/en/latest/api/native-api.html#update-a-dataverse-collection)

I also added partial update coverage in both:

  • CollectionsRepository.test
  • UpdateCollection.test

Optional: I did not rename createCreateOrUpdateRequestBody() to avoid additional overhead in this PR, although a name like createCreateRequestBody() would be more precise semantically.

@AnneGerlach AnneGerlach requested a review from ChengShi-1 April 16, 2026 07:41
@ChengShi-1 ChengShi-1 self-assigned this Apr 20, 2026
Copy link
Copy Markdown
Contributor

@ChengShi-1 ChengShi-1 left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for the changes, Anne.

@github-project-automation github-project-automation Bot moved this from In Review 🔎 to Ready for QA ⏩ in IQSS Dataverse Project Apr 21, 2026
Copy link
Copy Markdown
Contributor

@ChengShi-1 ChengShi-1 left a comment

Choose a reason for hiding this comment

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

Could you also fix the integration error?

FAIL test/integration/collections/CollectionsRepository.test.ts (62.014 s)
  ● CollectionsRepository › getCollectionItems for published tabular file › should return collection items given a valid collection alias

    expect(received).toBe(expected) // Object.is equality

    Expected: "test-file-4.tab"
    Received: "test-file-4.tsv"

      1013 |       expect(actualFilePreview.fileType).toBe('Tab-Delimited')
      1014 |       expect(actualFilePreview.md5).toBe(expectedFileMd5)
    > 1015 |       expect(actualFilePreview.name).toBe(expectedFileName)
           |                                      ^
      1016 |       expect(actualFilePreview.publicationStatuses).toContain(PublicationStatus.Published)
      1017 |       expect(actualFilePreview.sizeInBytes).toBe(137)
      1018 |       expect(actualFilePreview.url).not.toBeUndefined()

      at test/integration/collections/CollectionsRepository.test.ts:1015:38
      at fulfilled (test/integration/collections/CollectionsRepository.test.ts:5:58)

@ChengShi-1 ChengShi-1 moved this from Ready for QA ⏩ to In Review 🔎 in IQSS Dataverse Project Apr 21, 2026
@cmbz cmbz added the FY26 Sprint 22 FY26 Sprint 22 (2026-04-22 - 2026-05-06) label Apr 22, 2026
@AnneGerlach
Copy link
Copy Markdown
Author

Could you also fix the integration error?

FAIL test/integration/collections/CollectionsRepository.test.ts (62.014 s)
  ● CollectionsRepository › getCollectionItems for published tabular file › should return collection items given a valid collection alias

    expect(received).toBe(expected) // Object.is equality

    Expected: "test-file-4.tab"
    Received: "test-file-4.tsv"

      1013 |       expect(actualFilePreview.fileType).toBe('Tab-Delimited')
      1014 |       expect(actualFilePreview.md5).toBe(expectedFileMd5)
    > 1015 |       expect(actualFilePreview.name).toBe(expectedFileName)
           |                                      ^
      1016 |       expect(actualFilePreview.publicationStatuses).toContain(PublicationStatus.Published)
      1017 |       expect(actualFilePreview.sizeInBytes).toBe(137)
      1018 |       expect(actualFilePreview.url).not.toBeUndefined()

      at test/integration/collections/CollectionsRepository.test.ts:1015:38
      at fulfilled (test/integration/collections/CollectionsRepository.test.ts:5:58)

I reran the integration tests locally.
All CollectionsRepository.test.ts tests pass on my branch, so I wasn’t able to reproduce the error (either when running the test in isolation or as part of the full integration suite).

I do see other integration test failures in DatasetsRepository, but those also occur on develop for me, so they seem unrelated to this change.

Full test suite:

image

Test in isolation:

image

Could you also check this locally on your side?

@ChengShi-1
Copy link
Copy Markdown
Contributor

Hi Anne, I was able to reproduce the error locally when using an outdated Dataverse version. After updating to the latest dev branch, the issue no longer occurs. Since the GitHub check is still failing, it might be running against an outdated version. I’ll re-run it later to confirm

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

Labels

FY26 Sprint 17 FY26 Sprint 17 (2026-02-11 - 2026-02-25) FY26 Sprint 18 FY26 Sprint 18 (2026-02-25 - 2026-03-11) FY26 Sprint 20 FY26 Sprint 20 (2026-03-26 - 2026-04-08) FY26 Sprint 21 FY26 Sprint 21 (2026-04-08 - 2026-04-22) FY26 Sprint 22 FY26 Sprint 22 (2026-04-22 - 2026-05-06) GREI Re-arch GREI re-architecture-related

Projects

Status: In Review 🔎

Development

Successfully merging this pull request may close these issues.

6 participants