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

Fixed notifications to respect add to cart config #3229

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CardenB
Copy link
Contributor

@CardenB CardenB commented Mar 11, 2025

Description

Twilio and pushover notifications would always use add to cart links despite config. I changed this behavior.

Testing

Tested in prod locally 😅

@CardenB CardenB requested a review from jef as a code owner March 11, 2025 06:48
@MrSpookyAngel
Copy link

Line 29 for pushover also requires updating.

Copy link
Owner

@jef jef left a comment

Choose a reason for hiding this comment

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

Is the idea here that folks without auto add to cart wouldn't want the cart URL? They would want the link?

I don't think that's the right assumption, but could be swayed...

@CardenB
Copy link
Contributor Author

CardenB commented Mar 16, 2025

Yes. My motivation was that I was never successfully able to add a 5090 without the original link.

Also, when there are load issues with the website, it’s impossible to refresh the store page from the add to cart link. You need the original link.

It would be totally acceptable to simply have both links, as we do with the email notifications. It would be a simple change to shift to that behavior. I made the assumption that there was a single link for brevity purposes.

At a minimum, the link behavior is not at all consistent between notification methods, so this moves toward that direction.

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.

3 participants