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: [GROOT-1550] introduce proposal for fixing update behavior in taxonomy #2469

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

LiamStokingerContentful
Copy link
Contributor

Summary

This proposal looks to handle a mismatch in how taxonomy update methods are used when compared to other entities.

Long term plan in next major version is to:

  • introduce new patch method
  • change behavior of update method to use PUT

Short term plan is to:

  • introduce new patch method
  • mark update as deprecated, and explain change to PUT
  • introduce temporary updatePut method and mark as deprecated, so that we can use PUT requests for the time being
  • create change log post explaining upcoming changes

Description

Motivation and Context

Checklist (check all before merging)

  • Both unit and integration tests are passing
  • There are no breaking changes
  • Changes are reflected in the documentation

When adding a new method:

  • The new method is exported through the default and plain CMA client
  • All new public types are exported from ./lib/export-types.ts
  • Added a unit test for the new method
  • Added an integration test for the new method
  • The new method is added to the documentation

Copy link
Contributor

@AdrianLThomas AdrianLThomas left a comment

Choose a reason for hiding this comment

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

Nice one Liam, this makes sense to me: but would love Tundra's eyes on it too.

@LiamStokingerContentful
Copy link
Contributor Author

Nice one Liam, this makes sense to me: but would love Tundra's eyes on it too.

They seem to agree with the general idea - I would recommend we approve this and merge it into my other PR, and they can then review that all together 👍

@LiamStokingerContentful LiamStokingerContentful marked this pull request as ready for review November 19, 2024 08:58
@LiamStokingerContentful LiamStokingerContentful requested a review from a team as a code owner November 19, 2024 08:58
data,
{
headers: {
'X-Contentful-Version': params.version ?? 0,
Copy link
Member

Choose a reason for hiding this comment

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

why do we want to provide a default version? i think the error will be confusing in case it doesn't match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, can remove

@@ -51,10 +51,13 @@ export type ConceptSchemePlainClientAPI = {
* @returns the updated Concept Scheme
* @throws if the request fails
* @see {@link https://www.contentful.com/developers/docs/references/content-management-api/#/reference/taxonomy/concept-scheme}
* @deprecated The behavior of this method as a PATCH is being deprecated, and will be replaced with a PUT in the next major version. Use the `patch` method instead.
Copy link
Member

Choose a reason for hiding this comment

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

in the next major version

how can we make sure we don't forget to update it?

Copy link
Member

Choose a reason for hiding this comment

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

btw the last time we updated the major version was more than a year ago. long process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, but the impression I get from Tundra from our slack thread is that they'll work with us on this as planning for the next version.

Copy link
Member

Choose a reason for hiding this comment

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

what just if...

import * as package from 'package.json'

it('should not pass if major version is updated', () => {
  if (package.version.split('.')[0] === 12) {
    expect('updatePut' in sdk).to.be(false)
  }
})

@LiamStokingerContentful LiamStokingerContentful merged commit b61d95e into GROOT-1550 Nov 19, 2024
1 of 2 checks passed
@LiamStokingerContentful LiamStokingerContentful deleted the GROOT-1550-new-endpoints branch November 19, 2024 12:12
LiamStokingerContentful added a commit that referenced this pull request Nov 19, 2024
…axonomy (#2469)

* feat: [GROOT-1550] introduce proposal for fixing update behavior in taxonomy

* chore: [GROOT-1550] remove default version for taxonomy updates
LiamStokingerContentful added a commit that referenced this pull request Nov 21, 2024
* feat: [GROOT-1550] add support for taxonomy PUT methods

* feat: [GROOT-1550] introduce proposal for fixing update behavior in taxonomy (#2469)

* feat: [GROOT-1550] introduce proposal for fixing update behavior in taxonomy

* chore: [GROOT-1550] remove default version for taxonomy updates

* fix: [GROOT-1550] remove patch header from taxonomy put method
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.

3 participants