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

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

Open
wants to merge 9 commits into
base: add/support-core-gtin-field
Choose a base branch
from

Conversation

martynmjones
Copy link
Contributor

@martynmjones martynmjones commented Sep 20, 2024

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:

  1. 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
  2. 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
  3. 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:

Screenshot 2024-09-20 at 17 46 09

Detailed test instructions:

  1. 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
  2. Add or edit a product and confirm that the GTIN field is available on the Google for WooCommerce tab
  3. Update WooCommerce to version 9.2.0 or higher
  4. Add or edit a product and confirm that the GTIN field is read-only and the tooltip directs merchants to the Inventory tab
  5. 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
  6. 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

@martynmjones martynmjones self-assigned this Sep 20, 2024
@github-actions github-actions bot added the changelog: update Big changes to something that wasn't broken. label Sep 20, 2024
@martynmjones martynmjones changed the base branch from develop to add/support-core-gtin-field September 23, 2024 18:02
@martynmjones martynmjones requested a review from a team September 24, 2024 21:40
@martynmjones martynmjones marked this pull request as ready for review September 24, 2024 21:40
Comment on lines +166 to +180
public function set_readonly( $value = false ) {
$this->is_readonly = $value;

return $this;
}
/**
* @param bool $value
*
* @return InputInterface
*/
public function set_disabled( $value = false ) {
$this->is_disabled = $value;

return $this;
}
Copy link
Contributor

@puntope puntope Sep 25, 2024

Choose a reason for hiding this comment

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

WDYT of adding the types in the arguments and signature fore these 2?

public function set_readonly(bool $value = false ): InputInterface

Copy link
Contributor

@puntope puntope Sep 25, 2024

Choose a reason for hiding this comment

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

Another concern I have is that disabled is the right name to use for a hidden field. As in HTML disabled inputs are still readable. So I would suggest is_hidden / set_hidden but not a blocker tho, just following the naming for PR name and issue description:

... GTIN field in Google for WooCommerce will need to be hidden for new users of the extension and made read-only for existing users...

*
* @var string
*/
private $hidden_from = '2.8.5';
Copy link
Contributor

@puntope puntope Sep 25, 2024

Choose a reason for hiding this comment

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

COuld we add the keyword version somewhere in this field, also should this be a constant instead?

Comment on lines +77 to +85
protected function gla_installed_after( $date ): bool {
$gla_installed = $this->options->get( OptionsInterface::INSTALL_TIMESTAMP, false );

if ( false === $gla_installed ) {
return false;
}

return ( $gla_installed >= strtotime( $date ) );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we using this?

@@ -60,6 +73,9 @@ class Input extends Form implements InputInterface {
public function __construct( string $type, string $block_name ) {
$this->type = $type;
$this->block_name = $block_name;

$this->set_options_object( woogle_get_container()->get( OptionsInterface::class ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to instantiate the container in all the inputs? Maybe we can just do this in GTIN?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: update Big changes to something that wasn't broken.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GTIN] Hide the Global Trade Item Number field for new users and make read-only for existing users
2 participants