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

Closes #728 Replace out of quota notice by upsell block #752

Merged
merged 13 commits into from
Nov 8, 2023

Conversation

remyperona
Copy link
Contributor

@remyperona remyperona commented Oct 27, 2023

Description

Display an upsell banner and offer users who are out of the quota to upgrade to a higher plan.

Remove the banner at the top, we don’t need both banners since the same information would be displayed.

Fixes #728

Type of change

  • Enhancement (non-breaking change which improves an existing functionality).

Is the solution different from the one proposed during the grooming?

No

Checklists

Generic development checklist

  • My code follows the style guidelines of this project, with adapted comments and without new warnings.
  • I have added unit and integration tests that prove my fix is effective or that my feature works.
  • The CI passes locally with my changes (including unit tests, integration tests, linter).

Test summary

  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I validated all Acceptance Criteria of the related issues. (If applicable, provide proof).
  • I validated all test plan the QA Review asked me to.

@remyperona remyperona self-assigned this Oct 27, 2023
@remyperona remyperona added this to the 2.1.3 milestone Oct 27, 2023
@remyperona remyperona requested a review from a team October 27, 2023 14:50
@remyperona remyperona marked this pull request as ready for review October 27, 2023 14:50
Copy link
Contributor

@wordpressfan wordpressfan left a comment

Choose a reason for hiding this comment

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

Good job Boss, very minor change requests or questions to discuss.

classes/Notices/Notices.php Show resolved Hide resolved
classes/Notices/Notices.php Show resolved Hide resolved
views/part-upsell.php Outdated Show resolved Hide resolved
views/part-upsell.php Outdated Show resolved Hide resolved
@vmanthos vmanthos self-requested a review November 2, 2023 11:11
@vmanthos
Copy link
Contributor

vmanthos commented Nov 3, 2023

@Tabrisrp The icon /assets/images/stormy.svg that should be displayed in the banner isn't visible:
image

It should be:
image

This is because it is of the same color as the background.

Using the filter property can resolve this.
https://developer.mozilla.org/en-US/docs/Web/CSS/filter#browser_compatibility

Can you please look into this?

@vmanthos
Copy link
Contributor

vmanthos commented Nov 3, 2023

@Tabrisrp I got the following PHP fatal error:

PHP Fatal error:  Uncaught TypeError: call_user_func_array(): Argument #1 ($callback) must be a valid callback, class Imagify\Notices\Notices does not have a method "renew_almost_over_quota_notice" in /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php:312
Stack trace:
#0 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(334): WP_Hook->apply_filters()
#1 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#2 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/classes/User/User.php(221): do_action()
#3 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/classes/User/User.php(241): Imagify\User\User->get_percent_consumed_quota()
#4 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/views/part-upsell.php(8): Imagify\User\User->get_percent_unconsumed_quota()
#5 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/classes/class-imagify-views.php(590): include('...')
#6 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/classes/class-imagify-views.php(605): Imagify_Views->get_template()
#7 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/views/part-settings-account.php(33): Imagify_Views->print_template()
#8 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/classes/class-imagify-views.php(590): include('...')
#9 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/classes/class-imagify-views.php(605): Imagify_Views->get_template()
#10 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/views/page-settings.php(87): Imagify_Views->print_template()
#11 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/classes/class-imagify-views.php(590): include('...')
#12 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/classes/class-imagify-views.php(605): Imagify_Views->get_template()
#13 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify/inc/classes/class-imagify-views.php(242): Imagify_Views->print_template()
#14 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(310): Imagify_Views->display_settings_page()
#15 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(334): WP_Hook->apply_filters()
#16 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#17 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-admin/admin.php(259): do_action()
#18 /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-admin/options-general.php(10): require_once('...')
#19 {main}
  thrown in /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php on line 312

It seems that the renew_almost_over_quota_notice() method isn't there anymore.

If that's the way it should be, the following should be removed as well:

add_action( 'imagify_not_almost_over_quota_anymore', [ $this, 'renew_almost_over_quota_notice' ] );

@vmanthos
Copy link
Contributor

vmanthos commented Nov 6, 2023

@Tabrisrp There is another fatal error coming from here:

$upgrade = esc_esc_html__html_e( 'Upgrade your plan now to keep optimizing your images.', 'imagify' );

esc_esc_html__html_e() is not defined.

Can you please look into this as well? 🙏

@vmanthos
Copy link
Contributor

vmanthos commented Nov 6, 2023

@Tabrisrp Thank you for the fixes.

I don't see the implementation of the Black Friday promotion where text and URLs for upgrades should differ from the ones usually used.

Should this be part of the specific PR?

@remyperona
Copy link
Contributor Author

@vmanthos In the acceptance criteria, Piotr specified that the promo changes are not to be included in this PR

@vmanthos
Copy link
Contributor

vmanthos commented Nov 6, 2023

Thank you for checking, @Tabrisrp!

That was in bold, but still I managed to miss it. 😅

@vmanthos
Copy link
Contributor

vmanthos commented Nov 7, 2023

@Tabrisrp @wp-media/productimagify The notice is correctly displayed and it's dismissable.
However, I don't see a way for that to be displayed again to the same user except for uninstalling the plugin.

According to the specs doc:

The banner should be dismissable (and once it’s dismissed it shouldn’t be displayed until the user is out of the quota again)

Currently, the banner is displayed when the unconsumed quota is less than 20% of the plan's quota. If the banner is dismissed and more quota is used the banner isn't displayed.

Should it be when the unconsumed quota is 0%, or anything different? 🙏

@remyperona
Copy link
Contributor Author

The dismiss is reset if the quota gets back above 20%, and the banner should show again after that when it gets below 20%.

Copy link
Contributor

@vmanthos vmanthos left a comment

Choose a reason for hiding this comment

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

Working as expected! 👍

TestRail report: testrail-report-542.pdf

NOTE

Black Friday promotion deals haven't been implemented in the current PR.

@vmanthos vmanthos added this pull request to the merge queue Nov 8, 2023
Merged via the queue into develop with commit 1d202ce Nov 8, 2023
6 checks passed
@vmanthos vmanthos deleted the enhancement/728-upsell-banner branch November 8, 2023 10:00
@vmanthos vmanthos mentioned this pull request Nov 8, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display an upsell banner for the users out of the quota
4 participants