Skip to content

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.

@jef jef changed the title Fixed notifications to respect add to cart config fix: notifications to respect add to cart config Apr 24, 2025
@jef
Copy link
Owner

jef commented Apr 24, 2025

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

Ah! I do like the consistency, so thank you for that.

It would be totally acceptable to simply have both links, as we do with the email notifications.

I could see this being the case, but I like this for the reasons you mentioned.

If you could fix up the linting issues, I'll merged this in shortly after! Thanks @CardenB 😁

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.

All fixed up. Thank you very much for your contribution. I'm sure people will love this :)

@jef jef merged commit 68db9e2 into jef:main Apr 24, 2025
3 checks passed
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