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

Initial support for Custom Collections #26

Merged
merged 6 commits into from
Apr 2, 2020

Conversation

RomanTheLegend
Copy link
Contributor

Basic functionality for creating Custom Collections in Shopify.

Submitting this as small PR just to get the initial feedback on compliance with your style and requirements.

In future PRs I'm planning to further extend the functionality:

  • update collections
  • Include/exclude products in collections (initially via "collect" call, the at some later time also as collection's arguments
  • possibly image submission
  • More unit tests :)

@ryankazokas ryankazokas self-requested a review November 8, 2019 17:52
@ryankazokas
Copy link
Member

@RomanTheLegend thanks for the PR! I'll review this weekend. It looks like your commit isn't passing the CI build due to excessive log length though. Can you ensure you run mvn install locally and push up the pitest history file that gets generated after it successfully builds on your local machine.

@RomanTheLegend
Copy link
Contributor Author

Whelp, that's strange... First I though that's because my branch was forked from master, not develop, but then I took pitestHostory from develop and results are the same

@ryankazokas
Copy link
Member

@RomanTheLegend looks like that didn't work. Is that just the pittest file from the current develop or is that the one that resulted from your mvn install on your local machine. It should build correctly if you build that file locally based on your changes.
If you already did do that I can check out your branch later tonight and see what's going on with it.

@RomanTheLegend
Copy link
Contributor Author

I tried both clean mvn install - first push
and mvn install on top of checked out pitestHistory:

git checkout upmaster/develop src/test/resources/pitestHistory
mvn install
git add src/test/resources/pitestHistory
git commit --amend --no-edit
git push --force-with-lease

"upmaster" is ChannelApe repo

@rjdavis3
Copy link
Member

@RomanTheLegend @ryankazokas the PIT history file can sometimes get corrupt. @RomanTheLegend can you try deleting the file locally then recreating it by running mvn install? You can just check in the changed file.

You can read more on incremental analysis with PIT here:
https://pitest.org/quickstart/incremental_analysis/

Also just be aware that the build will take quite a while without this file. We have an open issue ( #13) to speed up the build.

@RomanTheLegend
Copy link
Contributor Author

This time ran tests on my other machine and it miraculously worked 😕

@ryankazokas ryankazokas self-assigned this Nov 14, 2019
Copy link
Member

@ryankazokas ryankazokas left a comment

Choose a reason for hiding this comment

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

Overall good work on this PR. Thanks @RomanTheLegend !. I have some minor feedback. Once you are complete let me know and i'll re-review and do some user testing to confirm things are working as expected

@RomanTheLegend
Copy link
Contributor Author

Thank you for feedback, I will address these issues on weekend

@RomanTheLegend
Copy link
Contributor Author

@ryankazokas Please check if everything's ok

Copy link
Member

@ryankazokas ryankazokas left a comment

Choose a reason for hiding this comment

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

Just add your missing test coverage here and this looks good for us to merge. If at all possible, can you please do this sooner rather than later? We are working on upgrading the sdk to support cursor pagination since it will not be supported soon for some of the API's we are using. We can handle the updates for these changes if they are already merged in, other wise we'll ask you to hold on this PR until those are complete and have you make those changes after we update the develop version of the SDK.



@Test
public void simpleProductCreationRequest() {
Copy link
Member

Choose a reason for hiding this comment

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

Change this test to a given/when/then format like the rest of the tests.

@@ -3182,6 +3185,64 @@ public void givenStoreWith305ProductsWhenRetrievingProductsThenReturnShopifyProd
}
}

@Test
public void givenSomeCustomColelctionsCreationRequestCreateAndReturnCustomCollection() throws JsonProcessingException {
Copy link
Member

Choose a reason for hiding this comment

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

Nice job on this test. Just add coverage on the other two methods you added and this looks good for us to merge in.

  1. getCustomCollections(final int page, final int pageSize)
  2. getCustomCollections()

@rjdavis3 rjdavis3 merged commit 02d7b8f into ChannelApe:develop Apr 2, 2020
@rjdavis3
Copy link
Member

rjdavis3 commented Apr 6, 2020

Implemented by 2.0.0 release.

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