Hide `GTIN` field for new installs make readonly for existing installs #2622

martynmjones · 2024-09-20T16:59:03Z

Changes proposed in this Pull Request:

Adds restrictions to the GTIN field in Google for WooCommerce depending on the version of WooCommerce installed and what version of Google for WooCommerce was first installed.

The following conditions apply:

If WooCommerce 9.2.0 or higher is installed and Google for WooCommerce is installed for the first time the GTIN field in G4W is hidden
If WooCommerce 9.2.0 or higher is installed and Google for WooCommerce had already been installed on a website then the GTIN field in G4W is read-only
If any version of WooCommerce lower than 9.2.0 is installed then the GTIN field in G4W continues to function as it previously did

Additionally, a new option is added called INSTALL_VERSION for tracking what version of Google for WooCommerce was first installed on a website 43a76c3.

Closes #2615

Screenshots:

Detailed test instructions:

Checkout update/2615-hide-gtin-for-new-users-make-readonly-for-exisitng on a site with WC 9.1.0 installed that has already onboarded with Google for WooCommerce
Add or edit a product and confirm that the GTIN field is available on the Google for WooCommerce tab
Update WooCommerce to version 9.2.0 or higher
Add or edit a product and confirm that the GTIN field is read-only and the tooltip directs merchants to the Inventory tab
Install the extension from this branch on a new test site and go through onboarding
- Alternatively, run the following WP Cli command to simulate the first install
  wp option update gla_install_version 2.8.4
Confirm that the GTIN field is not displayed on the Google for WooCommerce tab when editing a product

Changelog entry

Update - Restrict the GTIN field based on the version of WooCommerce installed and the initial version of G4W installed

…ed and what version of WC is installed

src/Admin/Input/Input.php

src/Admin/Product/Attributes/Input/GTINInput.php

src/HelperTraits/Utilities.php

src/Admin/Input/Input.php

…readonly-for-exisitng

…-for-new-users-make-readonly-for-exisitng

codecov · 2024-10-06T22:04:17Z

Codecov Report

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

Project coverage is 65.5%. Comparing base (b29ce70) to head (a2c3654).
Report is 15 commits behind head on add/support-core-gtin-field.

Files with missing lines	Patch %	Lines
src/Admin/Product/Attributes/Input/GTINInput.php	88.9%	1 Missing ⚠️
src/Internal/InstallTimestamp.php	0.0%	1 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##             add/support-core-gtin-field   #2622      +/-   ##
================================================================
+ Coverage                           62.7%   65.5%    +2.8%     
- Complexity                             0    4608    +4608     
================================================================
  Files                                319     474     +155     
  Lines                               5074   19299   +14225     
  Branches                            1231       0    -1231     
================================================================
+ Hits                                3180   12638    +9458     
- Misses                              1721    6661    +4940     
+ Partials                             173       0     -173

Flag	Coverage Δ
js-unit-tests	`?`
php-unit-tests	`65.5% <92.3%> (?)`

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%> (ø)`
src/Admin/Product/Attributes/AttributesForm.php	`97.3% <100.0%> (ø)`
src/Admin/Product/Attributes/Input/GTINInput.php	`92.9% <88.9%> (ø)`
src/Internal/InstallTimestamp.php	`0.0% <0.0%> (ø)`

... and 789 files with indirect coverage changes

mikkamp

Thanks for all the changes here. I have done the testing and I can confirm that the field is shown as normal for WC < 9.2. For newer versions of WC it's shown as a read only field:

And with the install later than 2.8.5 it's completely removed:

Should we keep a note that we need to tweak that version, I notice it's already been bumped from 2.8.4 > 2.8.5 but by the time we release this we might be at a newer version.

I went through the code and added a few opinionated comments, but these aren't strictly necessary to merge this PR, so I've gone ahead and approved anyway.

mikkamp · 2024-10-18T11:36:34Z

src/Admin/Input/Input.php

+	/**
+	 * @var bool
+	 */
+	protected $is_readonly = false;


Not really necessary since we don't do it in most other classes. But in theory we support PHP 7.4+ which means we can declare type for class properties. Just looks odd to set the type only in the doc-block and not actually restrict it in the class.

The reason I didn't do it is because the rest of the files (and other files like this one) have not the type set neither. So I bet for consistency in regards this topic.

mikkamp · 2024-10-18T11:38:53Z

src/Admin/Input/Input.php

+	 *
+	 * @return InputInterface
+	 */
+	public function set_readonly( $value ): InputInterface {


Similar comment here, if we are stating in the doc-block that $value should be a bool why don't we restrict it by setting the type? That's supported well before PHP 7.4

Adjusted here fa973aa

mikkamp · 2024-10-18T11:57:20Z

src/Admin/Product/Attributes/Input/GTINInput.php

@@ -24,5 +35,28 @@ public function __construct() {

 		$this->set_label( __( 'Global Trade Item Number (GTIN)', 'google-listings-and-ads' ) );
 		$this->set_description( __( 'Global Trade Item Number (GTIN) for your item. These identifiers include UPC (in North America), EAN (in Europe), JAN (in Japan), and ISBN (for books)', 'google-listings-and-ads' ) );
+		$this->set_options_object( woogle_get_container()->get( OptionsInterface::class ) );


This line is a little confusing to me. In other classes we use the OptionsAwareTrait which will automatically set the Options object within the class so we don't need to set it for each class individually. However for an input field it's not shared within the container class so it doesn't automatically set this object.

Since this class differs and it only needs to access options once, my suggestion would be to not use the OptionsAwareTrait and get the options object only once when needed. For example:

$options = woogle_get_container()->get( OptionsInterface::class ); $initial_version = $options->get( OptionsInterface::INSTALL_VERSION, false );

It would make it harder to mock, but I don't see us testing with a mocked options object for this class.

Adjusted here a2c3654

puntope · 2024-10-18T23:57:04Z

Should we keep a note that we need to tweak that version, I notice it's already been bumped from 2.8.4 > 2.8.5 but by the time we release this we might be at a newer version.

Correct @mikkamp Thanks for catching that. Added here #2648

martynmjones added 6 commits September 20, 2024 14:39

Add method to Input to disable field

a32ce73

Add Utilities trait to Input class and add readonly setting

5f4d2f8

Add utility to test if extension was installed after a specific date

7384a0a

Add functionality to disable form attribute

2893750

Disable or make GTIN field readonly depending on when GLA was install…

37edb29

…ed and what version of WC is installed

Update conditional restrictions

3c5007c

martynmjones self-assigned this Sep 20, 2024

github-actions bot added the changelog: update Big changes to something that wasn't broken. label Sep 20, 2024

phpcs fix

44ae293

martynmjones changed the base branch from develop to add/support-core-gtin-field September 23, 2024 18:02

martynmjones added 2 commits September 24, 2024 21:47

Add option to track version at initial install

43a76c3

Rely on initial install version to restrict GTIN field

34a858c

martynmjones requested a review from a team September 24, 2024 21:40

martynmjones marked this pull request as ready for review September 24, 2024 21:40

puntope reviewed Sep 25, 2024

View reviewed changes

src/Admin/Input/Input.php Outdated Show resolved Hide resolved

puntope reviewed Sep 25, 2024

View reviewed changes

src/Admin/Product/Attributes/Input/GTINInput.php Outdated Show resolved Hide resolved

puntope reviewed Sep 25, 2024

View reviewed changes

src/HelperTraits/Utilities.php Outdated Show resolved Hide resolved

puntope reviewed Sep 25, 2024

View reviewed changes

src/Admin/Input/Input.php Outdated Show resolved Hide resolved

puntope added 11 commits October 6, 2024 16:18

Remove unused utility

91fc9f0

Tweak names

1e3c808

Tweak names

e6d52b7

Tweak names and logic

fcaca2e

Add tests

29e2417

Merge branch 'develop' into update/2615-hide-gtin-for-new-users-make-…

2e1f800

…readonly-for-exisitng

Remove dupl. tests

b29ce70

Merge branch 'add/support-core-gtin-field' into update/2615-hide-gtin…

4e8fc0d

…-for-new-users-make-readonly-for-exisitng

PHPCS

475711c

Tweak tests

9340831

Tweak tests

d677d3d

puntope requested a review from a team October 16, 2024 13:50

puntope self-assigned this Oct 17, 2024

mikkamp approved these changes Oct 18, 2024

View reviewed changes

puntope mentioned this pull request Oct 18, 2024

[GTIN] Set Extension version to the right version after releasing the feature #2648

Open

puntope added 2 commits October 18, 2024 18:01

Add types for method arguments

fa973aa

Tweak options logic

a2c3654

puntope merged commit 4f6c307 into add/support-core-gtin-field Oct 19, 2024
12 checks passed

puntope deleted the update/2615-hide-gtin-for-new-users-make-readonly-for-exisitng branch October 19, 2024 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide `GTIN` field for new installs make readonly for existing installs #2622

Hide `GTIN` field for new installs make readonly for existing installs #2622

martynmjones commented Sep 20, 2024 •

edited

Loading

codecov bot commented Oct 6, 2024 •

edited

Loading

mikkamp left a comment

mikkamp Oct 18, 2024

puntope Oct 18, 2024

mikkamp Oct 18, 2024

puntope Oct 19, 2024

mikkamp Oct 18, 2024

puntope Oct 19, 2024

puntope commented Oct 18, 2024

Hide GTIN field for new installs make readonly for existing installs #2622

Hide GTIN field for new installs make readonly for existing installs #2622

Conversation

martynmjones commented Sep 20, 2024 • edited Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Changelog entry

codecov bot commented Oct 6, 2024 • edited Loading

Codecov Report

mikkamp left a comment

Choose a reason for hiding this comment

mikkamp Oct 18, 2024

Choose a reason for hiding this comment

puntope Oct 18, 2024

Choose a reason for hiding this comment

mikkamp Oct 18, 2024

Choose a reason for hiding this comment

puntope Oct 19, 2024

Choose a reason for hiding this comment

mikkamp Oct 18, 2024

Choose a reason for hiding this comment

puntope Oct 19, 2024

Choose a reason for hiding this comment

puntope commented Oct 18, 2024

Hide `GTIN` field for new installs make readonly for existing installs #2622

Hide `GTIN` field for new installs make readonly for existing installs #2622

martynmjones commented Sep 20, 2024 •

edited

Loading

codecov bot commented Oct 6, 2024 •

edited

Loading