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

Support resource identifier on customer group in PriceDraft #676

Merged

Conversation

leungkinghin-ct
Copy link
Contributor

Description

Change from Reference object to ResourceIdentifier object so that user can put customer group key into key field, not id field.
https://docs.commercetools.com/api/releases/2021-02-03-reference-customer-groups-by-key

Todo

  • Tests
    • Unit
    • Integration
  • Documentation
  • Add Release Notes entry.

* <p>Note: The key of the customer group reference taken from the value of the id field of the
* reference.
* <p>Note: The key of the customer group reference taken from the value of the key field of the
* resource identifier.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this Note? We can remove it, Since we don't have confusion anymore using key value in id field. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docs/RELEASE_NOTES.md Show resolved Hide resolved
for those references you have to provide the `key` value on the `id` field of the reference. This means that calling `getId()` on the reference should return its `key`.

````java
final PriceDraft priceDraft = PriceDraftBuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

This example should not be removed, instead, you might update the customer group with ResourceIdentifier.ofKey..

I would move the sentence on line 120: Some references... with changing to,

  • The product variant attributes with a type ReferenceType do not support the ResourceIdentifier yet, for those references you have to provide the key value on the id field of the reference. This means that calling getId() on the reference should return its key.

Copy link
Contributor Author

@leungkinghin-ct leungkinghin-ct Feb 26, 2021

Choose a reason for hiding this comment

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

If update it with ResourceIdentifier.ofKey, this example is no longer related to ReferenceType.
Hence the example looks a bit irrelevant to line 120. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that my comment is quite clear. Let me explain again:

The example code snippet should not be removed and should be updated with ofKey but the sentence above it would be better to move after this snippet example because it only explains the ReferenceType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense if we move the snippet before the sentence.
3cd0d9b

build.gradle Show resolved Hide resolved
@leungkinghin-ct leungkinghin-ct removed the request for review from lojzatran February 26, 2021 07:50
* master:
  fix deployment url.
  Add timeouts.
  remove duplicated text.
  Set lib versiob properly.
  Remove unneeded comment.
  Change checklist.
  Remove oss-publish step.
  Clean up docs.
  Add missing quote.
  use different urls for snapshot and release deployments.
  Cleanup.
  Add release version.
  Directly push artifacts to mvn central.
  Put additional note for the JDK compability.

# Conflicts:
#	docs/README.md
@ahmetoz ahmetoz changed the base branch from master to prepare_4.0.0 February 26, 2021 13:54
@leungkinghin-ct leungkinghin-ct merged commit 97724de into prepare_4.0.0 Feb 26, 2021
@ahmetoz ahmetoz deleted the support-resourceidentifier-for-customer-group branch February 26, 2021 15:16
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