Skip to content

Conversation

@fpotier
Copy link
Member

@fpotier fpotier commented Dec 2, 2025

Description

V3 api should return "publisher": "string" not "publisher: { "name": "string" }

Motivation

Fixes

Changes:

  • fix publisher property in reponse
  • use class-transformer for serialization of the reponse

Tests included

  • Included for each change/fix?
  • Passing?

Documentation

  • swagger documentation updated (required for API changes)
  • official documentation updated

official documentation info

@fpotier fpotier force-pushed the fix/publisheddata-dto-v3 branch 3 times, most recently from ac4160f to 6ed3505 Compare December 2, 2025 15:41
@fpotier fpotier marked this pull request as ready for review December 2, 2025 15:50
@fpotier fpotier requested a review from a team as a code owner December 2, 2025 15:50
@fpotier fpotier force-pushed the fix/publisheddata-dto-v3 branch from 6ed3505 to 69aafd1 Compare December 3, 2025 08:56
@fpotier fpotier force-pushed the fix/publisheddata-dto-v3 branch from 69aafd1 to 10a6335 Compare December 3, 2025 09:03
Copy link
Member

@minottic minottic left a comment

Choose a reason for hiding this comment

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

are there tests anywhere (either unit or mocha) that check the transformations?

Copy link
Member

@minottic minottic left a comment

Choose a reason for hiding this comment

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

I have a bit the impression tests are missing, at least for the bug fix, I expected to see a test now.

For what concerns the refactor, that looks very nice IMO! Was this mapping ever tested, even before your refactor?

@fpotier
Copy link
Member Author

fpotier commented Dec 5, 2025

I have a bit the impression tests are missing, at least for the bug fix, I expected to see a test now.

For what concerns the refactor, that looks very nice IMO! Was this mapping ever tested, even before your refactor?

tests for the mapping were not really existing, my last commit adds some

@fpotier fpotier force-pushed the fix/publisheddata-dto-v3 branch from 69abcee to 8358def Compare December 5, 2025 13:02
@fpotier
Copy link
Member Author

fpotier commented Dec 5, 2025

this also fixes a bug where publicationYear would always be overwritten with the current year

Copy link
Member

@minottic minottic left a comment

Choose a reason for hiding this comment

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

I think the tests from 8358def are great, but since they also required a bunch of bug fixes, would it make sense to move that commit to a dedicated PR (if cherry-picking the commit into master is not too conflict prone)? then we limit this to a refactor and the git history will look cleaner.

@minottic minottic self-requested a review December 5, 2025 13:39
Copy link
Member

@minottic minottic left a comment

Choose a reason for hiding this comment

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

I think the tests from 8358def are great, but since they also required a bunch of bug fixes, would it make sense to move that commit to a dedicated PR (if cherry-picking the commit into master is not too conflict prone)? then we limit this to a refactor and the git history will look cleaner.

actually forget this, I missed before that this PR already contains bug fixes, and it's not only a refactor.

Maybe it's worth though to open an issue to refactor the convertObsoleteToCurrent in the same way leveraging DTOs

@fpotier fpotier merged commit 8f630a3 into master Dec 8, 2025
13 checks passed
@fpotier fpotier deleted the fix/publisheddata-dto-v3 branch December 8, 2025 08:37
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.

4 participants