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

Add/usage tracking improvements #2593

Merged
merged 20 commits into from
Oct 4, 2023
Merged

Conversation

mikeyarce
Copy link
Member

Fixes #2580

Changes proposed in this Pull Request

  • Usage tracking checkbox in Setup is now checked by default
  • The "Usage Tracking" notice will show up 1 week after the plugin has been activated
  • When the plugin is updated, we'll show the "Usage Tracking" notice again, but only one time. If it's dismissed, it won't show up on future updates
  • I also added a few target="_blank" attributes to some links that did not have them.

Testing instructions

  • Pull this branch to a test site
  • Make sure you DON'T have usage tracking enabled
  • Go to run a setup on this page: /wp-admin/index.php?page=job-manager-setup
  • See that the box is checked by default
  • Click Save and Skip setup and make sure the option is saved properly

  • To mimic the 1 week wait, you can edit the time in delay_tracking_notice to display the notice.
  • To mimic a plugin update, you can edit the WP_Job_manager::updater() function so that the condition is true and so that the code runs
  • Make sure that the usage tracking prompt shows up after that.

@mikeyarce mikeyarce requested a review from a team September 26, 2023 21:10
@gikaragia gikaragia added this to the 1.42.0 milestone Sep 29, 2023
@@ -11,22 +11,34 @@
?>
<h3><?php esc_html_e( 'Welcome to the Setup Wizard!', 'wp-job-manager' ); ?></h3>

<script type="text/javascript">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we don't do this in PHP directly. Changed that in 00ba0f0

@mikeyarce let me know if there is something that I'm missing here.

@gikaragia gikaragia self-assigned this Oct 2, 2023
*
* @since $$next-version$$
*/
public function set_activation_time() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that creating a function that has a single statement makes code a bit harder to read, especially when this function is called only once. You can argue that sometimes when a single statement is very complicated, it makes it easier to understand it if you give it a name with a function but I don't think this is the case here.

* @since $$next-version$$
*/
public function set_activation_time() {
set_transient( 'job_manager_activation_time', time() );
Copy link
Contributor

Choose a reason for hiding this comment

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

Transients should be thought of as a cache that can be deleted any time. In our case this won't be a big issue but I think that having an option for that will be better.

Copy link
Contributor

@gikaragia gikaragia left a comment

Choose a reason for hiding this comment

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

Since you are on Support Rotation this week Mikey, I'll pick up the changes for this PR. I just left the comments for documentation purposes.

*/
function delay_tracking_notice() {
$activation_time = get_transient( 'job_manager_activation_time' );
if ( $activation_time && ( time() - $activation_time ) > 604800 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

WP defines a few useful constants for these:

	define( 'MINUTE_IN_SECONDS', 60 );
	define( 'HOUR_IN_SECONDS', 60 * MINUTE_IN_SECONDS );
	define( 'DAY_IN_SECONDS', 24 * HOUR_IN_SECONDS );
	define( 'WEEK_IN_SECONDS', 7 * DAY_IN_SECONDS );
	define( 'MONTH_IN_SECONDS', 30 * DAY_IN_SECONDS );
	define( 'YEAR_IN_SECONDS', 365 * DAY_IN_SECONDS );

@gikaragia
Copy link
Contributor

Hey @fjorgemota since the rest of the team is in support rotation, can you take a look at these changes? The testing instructions should be updated as:

  • To simulate a week passing you can update job_manager_display_usage_tracking_once to a small number.
  • To simulate a new plugin installation, you can delete wp_job_manager_version.
  • Any changes happen on plugin activation so to test you should: 1) Deactivate WPJM 2) Do any changes to the db 3) Re-enable the plugin.

@gikaragia gikaragia force-pushed the add/usage-tracking-improvements branch from 94c4ef3 to 3ac64ce Compare October 2, 2023 15:05
@fjorgemota fjorgemota self-requested a review October 2, 2023 21:46
Copy link
Member

@fjorgemota fjorgemota left a comment

Choose a reason for hiding this comment

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

This is looking generally good. I'll test it further tomorrow.

From what I'm seeing, when I deactivate and then activate, the notice looks to be appearing even when the option job_manager_display_usage_tracking_once is in the future (with value 1696893421), after removing the option wp_job_manager_version. I'm not sure if I'm doing something wrong, but I'll test more tomorrow.

lib/usage-tracking/class-usage-tracking-base.php Outdated Show resolved Hide resolved
lib/usage-tracking/class-usage-tracking-base.php Outdated Show resolved Hide resolved
includes/class-wp-job-manager-install.php Show resolved Hide resolved
lib/usage-tracking/class-usage-tracking-base.php Outdated Show resolved Hide resolved
@gikaragia gikaragia force-pushed the add/usage-tracking-improvements branch from 4001a4b to bdc97ca Compare October 3, 2023 10:25
@gikaragia
Copy link
Contributor

gikaragia commented Oct 3, 2023

the notice looks to be appearing even when the option job_manager_display_usage_tracking_once is in the future

Well spotted! The logic was flawed, fixed in cc81563

Copy link
Member

@fjorgemota fjorgemota left a comment

Choose a reason for hiding this comment

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

Wow! Thanks a lot for making these changes.

I reviewed them all and now the feature works properly. Also tried updating the option job_manager_display_usage_tracking_once to a value generated by strtotime('+1 minute'); and I could confirm that now the feature works beautifully.

Also, the linting changes look great.

Thanks a lot! Approved!

(I still left two very minor comments with suggestions, but I believe you can consider the PR as approved after the changes)

includes/class-wp-job-manager-post-types.php Outdated Show resolved Hide resolved
includes/class-wp-job-manager-usage-tracking.php Outdated Show resolved Hide resolved
@gikaragia gikaragia force-pushed the add/usage-tracking-improvements branch from 3667af2 to 55b537b Compare October 4, 2023 10:05
@gikaragia
Copy link
Contributor

There are some issues unrelated to this PR. I'll go ahead with merging this one and fix them as part of a separate PR

@gikaragia gikaragia merged commit 7749db6 into trunk Oct 4, 2023
2 of 8 checks passed
@gikaragia gikaragia deleted the add/usage-tracking-improvements branch October 4, 2023 10:47
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.

Improve usage tracking on installation
3 participants