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

Merge query parameters back after Tweakwise Personal Merchandier Refresh #83

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

Beagon
Copy link
Contributor

@Beagon Beagon commented Aug 22, 2023

There is an issue with the refresh the Personal Merchandiser does and query parameters used by marketing teams to track clicks from newsletters/blogs etc.

This reads the query parameters if there were any on the PM refresh (and other unintended refreshes)

@ah-net
Copy link
Collaborator

ah-net commented Aug 23, 2023

@Beagon Thx for the pull request.

What is the use case for the flip function you've added? Why do they need to be flipped?

@Beagon
Copy link
Contributor Author

Beagon commented Aug 28, 2023

They have to be flipped because otherwise the function wouldn't merge in the right priority in some instances, for example:

Lets say you are on the following page:
https://example.io/category?p=3

And you use the pagination to go to page 4. If you don't flip the priority it would not update the page number in the URL since.

We would've expected https://example.io/category?p=4 but it remained https://example.io/category?p=3. Of course the rest of the page was correct, just the updated URL by the JS wasn't correct.

@ah-net
Copy link
Collaborator

ah-net commented Sep 26, 2023

@Beagon I've added an change to make sure that the page paremeter is an int, to prevent xss issues

@ah-net ah-net merged commit 1e91f83 into EmicoEcommerce:master Sep 26, 2023
@ah-net
Copy link
Collaborator

ah-net commented Feb 6, 2024

@Beagon This pull request seems to be causes this issue #119. To fix #119 we will be reverting this change unless you have another solution for this, or an good explanation as to why this change is needed.

The most marketing tools i've seen use the parameter to set an cookie. So it's not important that on an page request the parameter still persists.

@Beagon
Copy link
Contributor Author

Beagon commented Feb 7, 2024

As I'm not currently working with Magento I do not have time to propose a different solution for this issue. Feel free to reverse the change.

@Beagon Beagon deleted the TweakPMFix branch February 7, 2024 11:04
ah-net added a commit that referenced this pull request Feb 15, 2024
@ah-net ah-net mentioned this pull request Feb 15, 2024
ah-net added a commit that referenced this pull request Feb 15, 2024
ah-net added a commit that referenced this pull request Feb 21, 2024
ah-net added a commit that referenced this pull request Feb 21, 2024
ah-net added a commit that referenced this pull request Feb 21, 2024
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

2 participants