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 product from cart until available date.. #2720

Merged
merged 19 commits into from May 7, 2020

Conversation

mc12345678
Copy link
Contributor

This additional/removable code offers an opportunity for a product
to be fully defined in the store, but not shown as available,
upcoming, new, etc (because it is disabled) until the product
available date at which point it appears in the store as an
enabled product ready for the customer to see and/or purchase.

This additional/removable code offers an opportunity for a product
to be fully defined in the store, but not shown as available,
upcoming, new, etc (because it is disabled) until the product
available date at which point it appears in the store as an
enabled product ready for the customer to see and/or purchase.
@drbyte
Copy link
Member

drbyte commented Aug 11, 2019

This PR reads more like a plugin than a core-code component.

@drbyte drbyte requested a review from zcwilt August 11, 2019 16:49
@mc12345678
Copy link
Contributor Author

Seeing as just about everything is in question of needing to be in core code versus being an addition, I hesitated on incorporating directly into "harder to edit" core code. (the edit isn't really difficult, just one of those things where it is unknown how others have possibly modified the related code to suit their needs.)

It could be incorporated more as part of the core, the existing could possibly be modified a little to be more adaptable or this could remain outside of the core code and instead be a plugin. As usual, open to thoughtful recommendations.

@drbyte
Copy link
Member

drbyte commented Aug 15, 2019

Suggestions:

  • Can we move these function definitions into functions_products.php ?
  • And then remove the config autoload file by merging these additions into the core autoloader ... perhaps as 150 alongside the sales/specials init. (You've got 149 now, so let's just collapse it into 150 with the other.)
  • And then add a config switch for enable/disable it. Maybe group 9-Stock or 24-IndexListing? Or is someplace else better? Open to discussion.

@mc12345678
Copy link
Contributor Author

Suggestions:

  • Can we move these function definitions into functions_products.php ?
  • And then remove the config autoload file by merging these additions into the core autoloader ... perhaps as 150 alongside the sales/specials init. (You've got 149 now, so let's just collapse it into 150 with the other.)
  • And then add a config switch for enable/disable it. Maybe group 9-Stock or 24-IndexListing? Or is someplace else better? Open to discussion.

Sorry for the delay in response. Good, so bring this into the "routine" code, be glad to. Answers: yes, yes, initially was thinking 9-Stock, but then also 1-My Store (there is product control there relating to out-of-stock action), but still looking/considering.

Is it ok if $_SESSION['today_is'] is taken from date to datetime aware (perhaps an additional variable could/should be used)? It would knowingly affect sales, specials, and featured product if $_SESSION['today_is'] is modified as these look to verify that the date has changed. They do not evaluate the current datetime to see if it is greater than a database value. Or is the datetime aspect something that should be addressed as part of #2397?

@drbyte
Copy link
Member

drbyte commented Aug 19, 2019

I think it makes sense to have consistency with sales/specials/featured date parsing. No point introducing new variables for accomplishing the same task.

While I hate the use of 0001-01-01 dates/times, I think this isolated feature should be treated on its own, assuming as little variation from core code as possible. Keep it atomic. Other much bigger issues should be handled separately, otherwise it's impossible to follow the bouncing ball when looking back into history to figure out why/when something was changed or understand something why something might operate differently, etc.

These functions were moved into products_functions by discussion/request in zencart#2720.
Ensures loading of `functions_products.php` and enables disabled product that were marked as having a future products_available_date where the current date is equal or beyond the date it was to become available.
As requested in zencart#2720, the call to the functions to enable disabled product was moved into: `includes/init_includes/init_special_funcs.php` which eliminated the need for loading `includes/init_includes/init_special_funcs_disabled_upcoming.php` and therefore eliminated the need for this separate config file.
This functionality was moved into `includes/init_includes/init_special_funcs.php` as discussed in zencart#2720.
Allow the enablement of product that have a historical product_available_date if the constant is defined and set to support enablement of this feature.

Consideration was given for testing/checking against `!= '0'`; however, seems that such an option would only be necessary to support some sort of third, fourth, etc... condition which is not clear what it may be and therefore likely would require modification of this area of the code if generated.  By the check specifically against `== '1'`, whatever defines it (file or database) if it is equal to 1 then this area of code will execute.

Alternatively, for those that do mass installations, if they choose not to include the option in the database or choose to remove it, then code will continue to operate without execution of this feature.
Made spacing of 4 spaces and removed parentheses around require/require_once calls.  Also corrected an issue with the earlier incorporation of `functions_products` where the closing parenthesis was omitted.
Adds the admin control for enabling the feature to enable product that are in the disabled status and has a `products_available_date` in the database that has come to be or has passed.
Default set to `0` to allow store owners to enable this feature potentially after a review of the database/product.  This is to prevent the sudden appearance of disabled product that happened to have a date entered in the `products_available_date` field.
Although original code had been adapted from other similar (but more complex) queries, the additional parentheses are unnecessary and I believe this was pointed out in earlier code review.
Use `foreach` instead of the older combination of `while (!$variable->EOF)` with `$variable->MoveNext()`
This also eliminated the need to check for the number of records returned.
@mc12345678
Copy link
Contributor Author

Completed the move of the files into existing Zen Cart core files, added an admin controllable switch located in group 9 just after the handling of out-of-stock product with the default set to disabled upon installation. Did not adjust the products_available_date check to being datetime specifically and updated the functions_products.php file to PSR-2.

There are multiple commits, some made to correct mistakes I made while incorporating the new code, but generally as a change to each file and in an add before delete sequence. If desired to keep more than one commit, then could combine some of them such as simultaneous add and delete together, later edits of new code to be combined with the first time the new code is provided, etc...

I believe this is ready for review at leisure.

A not incorporated consideration was the ability to activate this feature during the install/update process. I don't yet see the necessity though it may be nice.

As requested in the PR discussion, updated the `configuration_title` and `configuration_description` to try to provide an improved explanation of what this feature does.

Note, that as a result of working through this explanation, identified that enablement occurs without consideration of stock quantity or any other potential reason one may wish the product to remain disabled.  This is a further reason why the default is for this feature to be off upon install/upgrade so that consideration may be given to data modification and/or the desire to turn it on.
As requested in the PR discussion, updated the `configuration_title` and `configuration_description` to try to provide an improved explanation of what this feature does.

Note, that as a result of working through this explanation, identified that enablement occurs without consideration of stock quantity or any other potential reason one may wish the product to remain disabled.  This is a further reason why the default is for this feature to be off upon install/upgrade so that consideration may be given to data modification and/or the desire to turn it on.
@drbyte
Copy link
Member

drbyte commented Jan 21, 2020

Idea, not fully explored yet: would making the config switch options be "automatic" or "manual" allow the description/title to be expressed simpler?

@scottcwilson
Copy link
Sponsor Contributor

Suggested title/description:
"Hide upcoming products until available date"
"Treat upcoming products with available dates in the future as disabled, so customers cannot see them."

The default for this should be OFF - most people want to start marketing and get excitement building BEFORE launch day.

@torvista
Copy link
Member

torvista commented Apr 9, 2020

Late to the party here..so this is a global setting, affecting all products with a future availability date?

@mc12345678
Copy link
Contributor Author

@scottcwilson,

The default for this should be OFF - most people want to start marketing and get excitement building BEFORE launch day.

I am thinking that the default for a fresh install would be to have this enabled for automatic, but for an upgrade to be off. Offers full capability for new user (no need to ask how to make it work, rather would ask how to turn it off if found to be an "issue") and then for those performing a database upgrade would not affect how data was maintained. Albeit upon turning it on from an off status, changes would be made as applicable to the data.

@torvista,

so this is a global setting, affecting all products with a future availability date?

Yes, when this feature is "on" (recently suggested to be "automatic") then all products that have a future date that are disabled would remain "inaccessible" to the front end until the date has come to be. At that point the product would become enabled and included in all places that the active product was attached. For those that wish to use "future planning" or teasers of the product while this is on/automatic, leaving the product enabled with a future date would allow the product to be presented for advertising and as otherwise allowed/setup pre-orders. This feature may apply more to dated "advertising" or sales allowance that the product (e.g., physical, virtual, printed) becomes immediately available at the point in time designated.

@scottcwilson
Copy link
Sponsor Contributor

I am thinking that the default for a fresh install would be to have this enabled for automatic, but for an upgrade to be off.

This is confusing. Just keep it off for both upgraders and new installers - people who want it will turn it on.

This is a nice feature, I'm glad you added this capability, but it should be off unless someone wants it.

In preparation for future updates to the date/time processing
to use NULL instead of the historical default of `'0001-01-01 00:00:00'`
choosing to use NULL instead which is supported by the field.

This reduces one change to the overall system.  Check for whether
to process the record still compares against `'0001-01-01'`
in the event other "systems" clear the field to be appear as
empty.
For new installation (not using a previous database), set this to
automatic to enable product that are disabled and have a future
date.  For upgrades left this as Manual so that a database update
will not automatically enable product that were disabled but had
a future date.

Previous operation without this additional feature would have
left the product disabled from viewing on the store until the
product were enabled.
@mc12345678 mc12345678 requested a review from drbyte April 28, 2020 11:03
@zcwilt
Copy link
Member

zcwilt commented May 7, 2020

Subject to further review we should move this to 'on hold`

@mc12345678
Copy link
Contributor Author

Anything specific that could be addressed/corrected for moving forwards? E.g. Updates to other date related functions to set dates to null instead of 0001-01-01, the use of null as the final state, the install VS upgrade paths, anything I can do...

@zcwilt
Copy link
Member

zcwilt commented May 7, 2020

To be more specific, I had approved this, but want to wait for another approval, but if it's going to be merged it needs to be soon to allow for live testing before release.

@mc12345678
Copy link
Contributor Author

Understand. Thank you very much for the feedback and clarification.

@drbyte drbyte merged commit 96ab081 into zencart:v157 May 7, 2020
/**
* require product functions one time such that if previously loaded will not cause an error here.
*/
require_once DIR_WS_FUNCTIONS . 'functions_products.php';
Copy link
Member

Choose a reason for hiding this comment

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

@mc12345678
Given that init_general_funcs.php already has a require for functions_products.php which loads at breakpoint 60, why are we requiring it again here again in init_speical_funcs.php which loads at breakpoint 150?

Ref: #2551

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In essence, to allow such a comment/question/tracking to identify the dependency of these changes on functions_products.php... Possibly could have just added a note to indicate that dependency; however, also wanted to be sure that it was actually included at or before the subsequent code in the event the file's load point was otherwise changed. I recall there being some "adjustments" being made to its (functions_products.php) load point and/or inclusion.

As a "final product" I would say that identifying the dependency as a code comment should be sufficient and that the line could otherwise be removed.

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.

None yet

6 participants