Tons of additions by brachetti · Pull Request #4 · ChannelApe/bigcommerce-sdk

brachetti · 2018-05-23T05:28:42Z

Hey there,

This is an introduction of several new endpoints present in the v3 API.
Also some small additions on concepts:

Paginated responses are marked with an Interface, because we needed that for ... well, pagination ;-)

More changed incoming :-)

…iants call

…a products request

GetCategories and getCategoriesAsTree

Add options for getProducts

Change type of customFieldId to String

… additions * 'additions' of github.com:brachetti/bigcommerce-sdk: Create response for custom field creation Change customFieldId to int Replace product with product id Add create product custom field id method Change type of customFieldId to String

Fix CustomFieldResponse

Additional metadata for categories

sortOrder, metaKeywords, searchKeywords and customURL

additions

rjdavis3 · 2018-05-23T15:35:55Z

@brachetti thanks for taking the time to open up this pull request. Our developers are a bit swamped at the moment so we will need about 2 weeks to review your code and ensure it plays well with our integrations.

One of our developers will review the request and offer feedback soon.

ryankazokas

Great additions to the SDK!

Here are some changes you'd have to make for functional purposes, as well as to meet code standards that we have.

One thing we are adding that we forgot to mention here is that we use gitflow. This means PR's for new features should be opened for merging within develop, while hotfixes go directly to master. We will be adding this to the contribution.md doc to avoid confusion in the future, but in the meantime if you aren't familar with gitflow, check out this article:
https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow

ryankazokas · 2018-07-02T13:06:33Z

+				.queryParam(PAGE, page);
+
+		if (parentId != null) {
+			webTarget = webTarget.queryParam(PARENT_ID, parentId);


Nothing to fix here for now, but if we add more filters to this filter we may want to consider making this more scalable such as adding a parameter list that we can build.

ryankazokas · 2018-07-02T13:07:20Z

 	private static final String IMAGES = "images";
-	private static final int MAX_LIMIT = 250;
+	private static final String PARENT_ID = "parent_id";
+	public static final int MAX_LIMIT = 250;


This field doesn't need to be public.

I see where you are coming from, but when we used the library we put pagination outside and wanted to rely on a number that is more tightly bound to bigcommerce than our application.

I'm not sure I understand. Can you please describe your use case on code outside of this file needing to know the max limit? If calling code is concerned about the default max size, can't they just use the public methods that allow them to specify the limit?

I can try. We had in our project something like

public List<Category> getAllCategories(BigcommerceSdk client, Integer parentId) { return paginate( (page) -> client.getCategories(parentId, page, BigcommerceSdk.MAX_LIMIT), START_PAGE ); }

where paginate() was a function that then used the information from the response to, well ... paginate :-)

We felt, that our application should not decide what's the paging limit.

I see what you mean @brachetti . All i was getting at was to let the sdk do the magic, so your app doesn't need that parameter at all. From the above, it doesn't look like it really needs it.

We do similar things in our apps that use the sdk. See the getProducts method in the sdk. This hides the constant so the calling function just has to pass the page.

public Products getProducts(final int page, String... includes) { return getProducts(page, MAX_LIMIT, includes); }

So using the public method without the limit should work:

public Categories getCategories(final int page) { return getCategories(page, MAX_LIMIT); }

If you did this, your app wouldn't need to know the max limit and we'd be able to keep it private.

I see what you are trying to do nod

What do you think about including pagination in the SDK itself, so you rather end up with a getAllProducts() method? I feel this would improve usability a lot

Yeah, I agree. That's a great idea. That'd be a nice abstraction from having to deal with the pagination at all.
I'd just suggest if this is something you wanted to work on, please open up a GitHub enhancement and submit a new PR for the issue. Thanks!

Here is the contributing.md we are working on. It includes the part about suggesting a new enhancement:
https://github.com/ChannelApe/bigcommerce-sdk/blob/feature/Contribution_Doc/CONTRIBUTING.md#suggesting-a-new-feature

It's in #5 , I'll base the new branch off this one, since I need the PaginatedModel<T> interface

ryankazokas · 2018-07-02T13:07:40Z

 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.Response.Status;

+import com.bigcommerce.catalog.models.*;


Please refrain from using wildcard imports and import the specific classes you need. It may need all of the classes from that package now, but as we add more models/packages this may change.

See. 3.3.1 of our style guide:
https://channelape.github.io/styleguide/java/javaguide.html

ryankazokas · 2018-07-02T13:24:24Z

 @XmlAccessorType(XmlAccessType.FIELD)
 @JsonInclude(Include.NON_NULL)
-public class Brand {
+public class Brand implements Serializable {


The serializable class Brand does not declare a static final serialVersionUID field of type long

Please add a generated serialVersionUID

ryankazokas · 2018-07-02T13:28:01Z

+@JsonInclude(NON_NULL)
+public class CustomUrl implements Serializable {
+
+	@XmlElement(name = "url")


No need for the annotation here. The marshaler will automatically match fields by name. The only time we use this is when the field in JSON is snake case while we like to keep our fields camelcase.

ryankazokas · 2018-07-02T13:50:28Z

 import javax.xml.bind.annotation.XmlRootElement;

+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import org.apache.commons.lang3.builder.EqualsBuilder;


Please remove this import if you find a way to not use it.

ryankazokas · 2018-07-02T13:51:38Z

 import javax.xml.bind.JAXBException;
 import javax.xml.bind.Marshaller;

+import com.bigcommerce.catalog.models.*;


Please remove your wildcard import
https://channelape.github.io/styleguide/java/javaguide.html#s3.3.1-wildcard-imports

ryankazokas · 2018-07-02T13:52:51Z

+
+@XmlAccessorType(XmlAccessType.FIELD)
+@JsonInclude(NON_NULL)
+public class CustomUrl implements Serializable {


Please add the generated serialVersionUID here.

ryankazokas · 2018-07-02T13:53:55Z

+import javax.xml.bind.annotation.XmlRootElement;
+import javax.xml.bind.annotation.XmlTransient;
+import java.util.LinkedList;
+import java.util.List;


Please remove these unused imports.

ryankazokas · 2018-07-02T13:58:03Z

 		assertEquals(expectedBrands.get(0).getSearchKeywords(), actualBrands.getBrands().get(0).getSearchKeywords());
 		assertEquals(expectedBrands.get(0).getMetaDescription(), actualBrands.getBrands().get(0).getMetaDescription());
 		assertEquals(expectedBrands.get(0).getMetaKeywords(), actualBrands.getBrands().get(0).getMetaKeywords());
+		assertEquals(expectedBrands.get(0).getCustomUrl().getUrl(), actualBrands.getBrands().get(0).getCustomUrl().getUrl());


It's good that you added coverage on the fields you added for brands. Can you also add some code coverage for the new public sdk methods.

getCategoriesAsTree

getCategories

updateCategory

createCategory

createProductCustomField

deleteProductCustomField

brachetti · 2018-07-11T09:25:31Z

@ryankazokas should I change the PR to go into develop then?

ryankazokas · 2018-07-11T13:32:38Z

Yes @brachetti , please switch this PR to go into develop. Thanks!

rjdavis3 · 2018-07-11T19:35:35Z

Hey @brachetti thanks so much for this PR. If you can change the merge branch from master to develop then we can accept this PR. @ryankazokas will then implement some of the code review feedback he has for you.

rjdavis3

Please change merge branch from master to develop. All other code review feedback can be implemented by @ryankazokas on develop.

We will then add all your changes into our next minor release.

… from PR #4

ryankazokas and others added 30 commits February 23, 2018 07:50

Merge branch 'hotfix/1.2.1'

1862674

Add (paged) call for getting all variants

a0ce822

Add Marker Interface for paginated modelsˆ

490f421

add common method for getting content in paginated models

32846fa

Add optional sku_id field to Variantˆ

9c251a1

Ignore (so far) unknown properties for variants returned from all var…

fa3132e

…iants call

Ignore meta information on VariantResponse

f8a9de0

Ignore unknown properties for product entities

428311f

Ignore unknown properties for pagination as part of the response for …

fcf76ca

…a products request

GetCategories and getCategoriesAsTree

003d9ab

Merge pull request #1 from seghersm/additions

724724a

GetCategories and getCategoriesAsTree

Add deleting product custom fields

d4a0327

Make Brands a paginated modelˆ

4a1d075

Add Custom Url as attribute to Brands

a4f9ed5

GetCategories and getCategoriesAsTree

99e8cb7

Make Brands a paginated modelˆ

7140f9d

Add options for getProducts

131cdb2

Merge remote-tracking branch 'upstream/additions' into additions

c4f3bcf

Fix custom field path and query parameter

0435ed9

Add back getProducts() method with just page argument

af7f81e

Add back old getProducts with page and limit arguments

b46d848

Merge pull request #2 from rashiq/additions

44fb934

Add options for getProducts

Change type of customFieldId to String

0390937

create and update category entities

fe5093b

Add create product custom field id method

bcdce25

Replace product with product id

e6dfaeb

Change customFieldId to int

c21889a

Create response for custom field creation

2e78944

Merge pull request #3 from rashiq/additions

41e03aa

Change type of customFieldId to String

Fix CustomField Response

9ceb4a2

rashiq and others added 12 commits May 16, 2018 19:05

Remove meta

bf0659a

Ignore meta

6794dfd

Getting categories by parent id

f69abd7

Merge pull request #4 from rashiq/additions

1467d64

Fix CustomFieldResponse

Additional metadata for categories

3ee6631

Additional metadata for categories

Merge pull request #5 from seghersm/additions

59442e0

Additional metadata for categories

add page title and meta description on product

abf476f

Added Product fields

4a1c79e

sortOrder, metaKeywords, searchKeywords and customURL

product response corrections, include meta

2377aea

Merge branch 'additions' into additions

c78ef71

Merge pull request #6 from seghersm/additions

f5a5253

additions

rjdavis3 requested review from rjdavis3 and ryankazokas May 23, 2018 16:23

rjdavis3 assigned rjdavis3 and ryankazokas May 23, 2018

ryankazokas suggested changes Jul 2, 2018

View reviewed changes

rjdavis3 suggested changes Jul 19, 2018

View reviewed changes

brachetti changed the base branch from master to develop July 20, 2018 10:45

Ronald Brachetti added 3 commits July 20, 2018 13:10

Replace wildcard imports

d546e24

Requested changes from review

01acf26

Rename field sku_id to skuId

445bbff

brachetti mentioned this pull request Jul 24, 2018

Internalize-pagination #6

Open

rjdavis3 approved these changes Jul 27, 2018

View reviewed changes

rjdavis3 merged commit 967880d into ChannelApe:develop Jul 27, 2018

ryankazokas added a commit that referenced this pull request Jul 30, 2018

Add category retrieval and product custom field creation and deletion…

9398078

… from PR #4

Conversation

brachetti commented May 23, 2018

Uh oh!

rjdavis3 commented May 23, 2018

Uh oh!

ryankazokas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryankazokas Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brachetti commented Jul 11, 2018

Uh oh!

ryankazokas commented Jul 11, 2018

Uh oh!

rjdavis3 commented Jul 11, 2018

Uh oh!

rjdavis3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ryankazokas Jul 20, 2018 •

edited

Loading