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

Refactor: Improve SalesBanner Maintainability #7454

Closed
wants to merge 5 commits into from

Conversation

JoshuaHungDinh
Copy link
Contributor

@JoshuaHungDinh JoshuaHungDinh commented Jul 17, 2024

Resolves: GIVE-1020

Description

This pull request enhances the extensibility of the main SalesBanner class by refactoring the GivingTuesdayBanner details into its own dedicated class. This separation improves maintainability and makes it easier to extend the functionality in the future.

In relation to GIVE-1020, a user was encountering a fatal error that we were unable to replicate. However, Rick provided a workaround that alleviated the issue for the user. This involved ensuring that the isShowing logic for GivingTuesdayBanner returns false when it should not load. One issue we noticed in the original implementation is that the SalesBanner class was unnecessarily loading scripts because this logic only checked if a user was on a "give-forms" page.

In the newly created GivingTuesdayBanner class, the isShowing method has been updated with a conditional check, ensuring that banner scripts only load when banners are available. This change aims to provide relief to users experiencing similar issues and improves the overall maintainability of the class.

Affects

SalesBanners

Visuals

Testing Instructions

We should retest the current banners to ensure they continue to work.

Admin Banners

  • Create a new test site & install/activate GiveWP or use a local server.
  • Add New Plugin > Upload Plugin > upload the attached file or activate the give plugin.
  • Click through the Donations, Donors, and Reports pages.
  • Verify the banners alternate between: GiveWP Plus plan & Peer-to-Peer.
  • Add a Basic license.
  • Click back through the same pages.
  • Verify you see the same banners that were shown before you added a license.
  • Now, add a Plus license instead.
  • Click back through the same pages.
  • Verify the banners alternate between: GiveWP Pro plan & Peer-to-Peer.
  • Now, add a Pro and Agency license instead.
  • Click back through the same pages.
  • Verify only a stellar banner is shown.
  • Test that the P2P banner never displays if P2P is active.
  • Test that once a banner is closed it remains closed.
  • Verify the links for each banner match the pricing plan.

Reports Widget Banners

  • Visit the WordPress dashboard.
  • Locate the GiveWP widget on the page.
  • Verify the banner is displayed and the link takes you to the appropriate page.
  • Verify after closing the banner, it is removed and remains closed.

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Includes unit tests
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@JoshuaHungDinh JoshuaHungDinh changed the title Refactor: create SalesBanner abstraction Refactor: split GivingTuesdayBanner for Improved Maintainability Jul 18, 2024
@JoshuaHungDinh JoshuaHungDinh changed the title Refactor: split GivingTuesdayBanner for Improved Maintainability Refactor: Improve SalesBanner Maintainability Jul 18, 2024
@JoshuaHungDinh JoshuaHungDinh marked this pull request as ready for review July 18, 2024 16:12
Copy link
Contributor

@pauloiankoski pauloiankoski left a comment

Choose a reason for hiding this comment

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

It does look good!

@JoshuaHungDinh
Copy link
Contributor Author

JoshuaHungDinh commented Aug 6, 2024

Closed reference: GIVE-1020

Will reconsider these changes during the next round of banners.

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.

2 participants