PopupMaker/Popup-Maker

Popup doesn't display when combining Query Argument Exists condition with something like OR Pages Selected

marklchaves opened this issue · 4 comments

marklchaves commented

Describe the bug

I’ve noticed a couple of times now that if I combine a Query Argument Exists condition with something like OR Pages Selected, my popup does not display for both the Query Argument Exists condition and for the Pages Selected.

Instead, that OR combination acts like an AND rule meaning the popup displays only if you're on the selected page using the specified query argument.

Targeting: https://share.wppopupmaker.com/xQuzxR79

Debug output: https://share.wppopupmaker.com/geudyEpQ

Site information

Popup Maker version: 1.16.4

WordPress version: 5.9.1

PHP version: 8.0.12

Expected behavior

I expect to see the popup for my Pages Selected condition and Query Argument Exists condition when my targeting is set up like this.

https://share.wppopupmaker.com/xQuzxR79

Current behavior

The popup doesn't display for the Pages Selected and the Query Argument Exists.

But, the popup displays if I'm on a Page Selected page and using the specified query argument at the same time (AND behavior).

Steps to reproduce

  1. Set up a Targeting condition like this https://share.wppopupmaker.com/xQuzxR79
  2. Visit the page where the popup should display. Popup shouldn't display.
  3. Add the query argument. Popup displays.
  4. Go to another page that's not in the targeting rules and add the query argument. Popup doesn't display.

Errors

Debug output: https://share.wppopupmaker.com/geudyEpQ

Additional context

Reproduced by Bel too.

danieliser commented

@marklchaves This one needs full testing as I had to make tweaks in a few places to get it properly ironed out. I tried to do it with scalpel-like precision to only change minimum needed.

That entire process needs to be optimized or rewritten from ground up with testing similar to that of Content Control, but we already knew that.

These minimal changes should fingers crossed have no impact other than resolving this mixmatch condition issue.

Commits are self labeled, but would appreciate eyes and at least a quick test that your usual test popups all still work.

  • 👍1
danieliser commented

As soon as you test develop its ready for a merge & release

marklchaves commented

Hi @danieliser,

Here's what I've tested so far (all passing). Let me know if there are any specific use cases I missed that you want covered.

Thanks!

Testing Query Parameter fix

  1. OP: Auto Open Sample Page, Contact Page OR Query Argument Exists. PASSED
  2. Auto Open Query Argument Exists. PASSED
  3. Auto Open Posts OR Query Argument Exists/Is. PASSED
  4. Auto Open Selected Posts OR/AND Query Argument Exists. PASSED
  5. Auto Open Pages OR Query Argument Exists. PASSED
  6. Auto Open Selected Pages OR/AND Query Argument Exists.
  7. Click Open Selected Posts AND Query Parameter Is
  8. Click Open Query Parameter Is
  9. Regress: Auto Open targeting Home Page. PASSED
  10. Regress: Auto Open targeting posts. PASSED
  11. Regress: Auto Open selected posts AND Query Argument Exists OR Firefox. PASSED
danieliser commented

Awesome, looks like you covered most of the commonly used ones. If they work now (better than before) I think we are good.

I tried to only make additive changes that should not have effect on existing stuff other than improving it.

  • ❤️1