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

[GTIN] Hide GTIN in product editor #2690

Merged
merged 6 commits into from
Nov 22, 2024
Merged

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Nov 22, 2024

Changes proposed in this Pull Request:

Closes part of #2684

This PR fixes a bug in which GTIN field was not hidden or disabled in Product Block Editor. Additionally fixes some e2e tests in product editor and classic editor

Screenshots:

Screenshot 2024-11-22 at 18 09 11

Detailed test instructions:

  1. Setup Product Block editor
  2. Set the WP Option gla_install_versionas < 2.8.8
  3. Go to Google for WooCommerce tab in the product editor
  4. GTIN should be readonly
  5. Set the WP Option gla_install_versionas >= 2.8.8
  6. Go to Google for WooCommerce tab in the product editor
  7. GTIN should not be rendered in the UI
  8. Install YOAST for WooComemerce
  9. Repeat steps 2-7

Additional details:

Changelog entry

Fix - Hide or disable GTIN in Product Block editor

@puntope puntope self-assigned this Nov 22, 2024
@puntope puntope requested a review from a team November 22, 2024 14:21
@github-actions github-actions bot added type: bug The issue is a confirmed bug. changelog: fix Took care of something that wasn't working. labels Nov 22, 2024
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 64.7%. Comparing base (5d03086) to head (4aece62).
Report is 7 commits behind head on develop.

Files with missing lines Patch % Lines
src/Admin/ProductBlocksService.php 50.0% 1 Missing ⚠️
...ernal/DependencyManagement/CoreServiceProvider.php 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             develop   #2690   +/-   ##
=========================================
  Coverage       64.7%   64.7%           
- Complexity      4665    4667    +2     
=========================================
  Files            799     799           
  Lines          24640   24645    +5     
  Branches        1242    1242           
=========================================
+ Hits           15938   15944    +6     
+ Misses          8529    8528    -1     
  Partials         173     173           
Flag Coverage Δ
js-unit-tests 62.4% <ø> (ø)
php-unit-tests 65.3% <71.4%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/Admin/Input/Input.php 100.0% <100.0%> (+2.5%) ⬆️
src/Admin/ProductBlocksService.php 96.1% <50.0%> (-0.7%) ⬇️
...ernal/DependencyManagement/CoreServiceProvider.php 0.0% <0.0%> (ø)
---- 🚨 Try these New Features:

@puntope puntope marked this pull request as ready for review November 22, 2024 14:31
Copy link
Contributor

@mikkamp mikkamp left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I tested all combinations of versions both with Yoast enabled and disabled, and I'm getting the expected results.

Both the unit tests and E2E tests pass locally now as well.

I'll go ahead and approved the PR. If there aren't any drastic changes with setting gla_install_version during E2E testing and changing the expected fields, then I'm ok with getting the PR merged without an additional review. We'll need to have it ready early on Monday to not block any of the other branches being released.

@puntope puntope merged commit f17e3c0 into develop Nov 22, 2024
16 checks passed
@puntope puntope deleted the fix/hide-gtin-in-product-editor branch November 22, 2024 17:30
@ianlin ianlin mentioned this pull request Nov 26, 2024
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants