Skip to content

Conversation

@robbiebel
Copy link

@robbiebel robbiebel commented Oct 4, 2024

Your checklist for this pull request

Thanks for sending a pull request! Please make sure you click the link above to view the contribution guidelines, then fill out the blanks below.

🚨Please review the guidelines for contributing to this repository.

  • Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Make sure you are requesting to pull request from a topic/feature/bugfix/devops branch (right side). Don't pull request from your master!
  • Have you ensured/updated that CLI tests to extend coverage to any new logic. Learn how to modify the tests here.

What does this implement/fix? Explain your changes.


Product Variations Shipping Classes was not implemented correctly.

Does this close any currently open issues?


No

Any relevant logs, error output, GraphiQL screenshots, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Any other comments?


It's a small fix to understand easily to look changes.

@kidunot89 kidunot89 self-requested a review October 11, 2024 14:45
Copy link
Collaborator

@kidunot89 kidunot89 left a comment

Choose a reason for hiding this comment

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

@robbiebel Sorry for the slow review of this. It's failing CI and I can't merge it as long as it is.

@robbiebel
Copy link
Author

Hi @kidunot89, I'm also sorry for slowness on my side.

I have tried before create first PR and now again. But I have not succeed testing on my local although I tried a few things.

testable_app | Warning: The 'woocommerce' plugin could not be found.
testable_app | Warning: The 'woocommerce-gateway-stripe' plugin could not be found.
testable_app | Warning: The 'wp-graphql' plugin could not be found.
testable_app | Warning: The 'wp-graphql-jwt-authentication' plugin could not be found.
testable_app | Plugin 'wp-graphql-woocommerce' activated.
testable_app | Error: Only activated 1 of 5 plugins.
testable_app exited with code 1

@robbiebel robbiebel requested a review from kidunot89 October 18, 2024 14:34
Copy link
Collaborator

@kidunot89 kidunot89 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 I understand what you're trying to achieve here but there is some work to be done. See my comments and also take a look at CONTRIBUTING.md

},
'shippingClassId' => function () {
return ! empty( $this->wc_data->get_image_id() ) ? $this->wc_data->get_shipping_class_id() : null;
return ! empty( $this->wc_data->get_shipping_class_id() ) ? $this->wc_data->get_shipping_class_id() : null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch

Comment on lines +234 to +236
'shippingClassId' => function () {
return ! empty( $this->wc_data->get_shipping_class_id() ) ? $this->wc_data->get_shipping_class_id() : null;
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary, why can't we continue to used the snake case shipping_class_id

],
'shippingClass' => [
'type' => 'String',
'type' => 'ShippingClass',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This type ShippingClass does not exist.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

That means this needs to be provided with a resolver that resolves to the TermNode. The resolver should looks something like this.

'resolve'     => static function ( $source, array $args, $context  ) {
        $id = $source->shipping_class_id;
        if ( $id ) {
	        return $context->get_loader( "term" )->load_deferred( $id );
        }

        return null;
},

@robbiebel
Copy link
Author

I think I understand what you're trying to achieve here but there is some work to be done. See my comments and also take a look at CONTRIBUTING.md

I already followed CONTRIBUTING.md but not succeeded even I have good experience in Docker. The CONTRIBUTING.md has many missing parts for fresh install.

But I achieved to run docker applications by tracing the code and filling the missing parts. I recommend that updating the CONTRIBUTING.md

@robbiebel robbiebel closed this Jan 26, 2025
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.

2 participants