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

feat: max price filtering #383

Merged
merged 22 commits into from
Oct 2, 2020
Merged

feat: max price filtering #383

merged 22 commits into from
Oct 2, 2020

Conversation

xninjax
Copy link
Contributor

@xninjax xninjax commented Sep 29, 2020

Description

This feature will address issues such as: Let us set price ranges to avoid 3rd party sellers/price gouging #310

Improves filtering by allowing a supplied MAX_PRICE in .env. This can help filter out price gouging sellers and is an alternate to having to specifically list banned sellers.

It would be better to have an array of prices (one for each card type) - but this should suffice as a framework to build upon.

Testing

test:series card was used against all stores (updated some test card links for euro stores, to ensure euro price formatting was taken care of properly).

Ensured that euro price formatting i.e 1.234,99 was converted to 'standard' formatting i.e 1234.99.

  • Tested against multiple amazon-de/nl products with varying number of digits in the pricetag, to ensure conversion works over a wide range of supplied prices
  • store model must include optional euroFormat: true for conversion to take place (see example below)

Ensured that Max Price was obeyed

  • MAX_PRICE="5" was set in .env and all test cards were confirmed to be skipped over. A new logger.info message is produced anytime the store price is greater than the threshold price.
  • MAX_PRICE="" and MAX_PRICE="2000" was set in .env, and all test cards were again able to trigger in-stock alerts.

Note: Manufacturer direct sales sites (ASUS, NVIDIA, etc) do not have max_price selectors currently set - and is likely not required.

Reproducing the test is pretty easy, set a MAX_PRICE in .env to a low enough value. Enable test:series card. Output will display a logger.info message including the store card price vs threshold price, if test card price exceeds max price .

Optional Max price container must be set in store model, under 'labels', for price filtering to function for that store.

maxPrice: {
		container: 'span[class*="PriceString"]',
		euroFormat: true
	  }

euroFormat is optional, and only required when store provides euro price formatting - as described above.

Functional demonstration

Test 1
MAX_PRICE="0.1"
image
✅ amazon-de and amazon-nl demonstrate proper currency format conversion (converts to dollars) and price limit filtering activated
✅ newegg-ca demonstrates proper currency format checking (remains as dollars) and price limit filtering activated
✅ ASUS demonstrates ability to trigger alert due to no price container set in asus store model

Test 2
MAX_PRICE="325"
image
✅ newegg-ca and amazon-de price remains above MAX_PRICE, and price limit filtering is activated
✅ amazon-nl is now below MAX_PRICE and thus allowed to trigger alert
✅ ASUS same as before

 - currently one price for all cards per store (should implement price per card)
 - currently hardcoded into model (should implement in .env).
 - See newegg-ca for example.
 - todo: MAX_PRICE per card?
 - fight with linter, linter won
added amazon and amazon-ca container tags
… should read "The time in milliseconds" ...
@xninjax xninjax requested a review from jef as a code owner September 29, 2020 20:58
@xninjax
Copy link
Contributor Author

xninjax commented Sep 29, 2020

@jef sorry for all the commits in this branch... I'm definitely not a git-guru by any means ¯\_(ツ)_/¯

Hope it wasn't too early for a PR, as testing on other pages hadn't been done (aside from amazon/-ca and newegg/-ca), but the framework is there now. My intent of max price should really only apply to sites with 3rd party marketplace sellers anyhow... I wouldn't imagine a need for max price on the manufactures sites?

EDIT: Changed PR to draft: found issue with price parsing for locales that use comma ',' as decimal place i.e € 832,38.
Will apply conditional logic to hopefully handle this, then re-submit PR.

UPDATE: All stores with exception of manufacturers retail sites have a Max price filter selector applied and tested. During the testing found some selectors on amazons and bestbuys to be quite elusive! However, I believe I have found some reliable selectors to use.

@xninjax xninjax marked this pull request as draft September 29, 2020 22:04
@karmanbadhesha
Copy link
Contributor

I had accomplished something similar by looking at the "Sold and shipped by Amazon.ca" but it can result in cases where a cheap 3rd party reseller listing might get missed! Price is definitely the way to go

 - all stores with exception of manufactures direct sales sites
 - euro price format conversion implemented
 - new type ‘Pricing’ created
 - euro test cards links updated for more realistic testing (amazons)
@xninjax xninjax marked this pull request as ready for review September 30, 2020 03:42
jef
jef previously requested changes Sep 30, 2020
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.

Overall, I really like it! I appreciate the contribution! I know a lot of people will like this. Especially with Amazon third party sellers.

Just a few comments. Thanks for all the help!

@@ -145,6 +145,35 @@ async function lookupCardInStock(store: Store, page: Page, link: Link) {
}
}

if (store.labels.maxPrice && Config.store.maxPrice) {
Copy link
Owner

Choose a reason for hiding this comment

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

Do you mind abstracting this logic in this if block just like the if statements above and below using the pageIncludesLabels?

It would help with complexity.

That being said, take a look at that function. I believe what you're wanting is something very similar. As I think your approach is great, but I think there is some extra work you're doing here that may not be needed since we have those helper functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes no worries, I'll take a look, appreciate the feedback! 😎

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks! Look forward to see what's to come.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Abstraction injection sequence complete 😄

  • Could not utilize pageIncludesLabels directly - as a mathematical comparison was needed, but it became a good jumping off point as reference for a new helper function...
    Open to feedback 👍

src/store/model/adorama.ts Show resolved Hide resolved
simplified code:
- utilizing extractPageContents helper function
- utilized ternary operators where possible
- cardPriceLimit returns 'null' if no price limit or card price ok, otherwise returns actual card price for use in logger.info message
…price-check-testing

# Conflicts:
#	.env-example
#	src/store/model/amazon-de.ts
#	src/store/model/amazon-nl.ts
@xninjax xninjax requested a review from jef October 1, 2020 02:24
@jef
Copy link
Owner

jef commented Oct 1, 2020

I think this is ready to ship! I just had a question on the containers. How come they seem more "complex"? Why not just grab the main selector that holds the price? Was it not working accordingly?

@jef jef dismissed their stale review October 1, 2020 17:28

Looks good, just some comments now

@xninjax
Copy link
Contributor Author

xninjax commented Oct 1, 2020

Good question about the containers, what I was finding was that the original/simple selectors were sometimes returning a completely different value... It appeared some js would modify prices and containers after load, likely bot protection 🤖 The more complex container matching helped ensure to grab the final result container with the actual price 🏆 😁

@DishBrain
Copy link

Hey there, I was just testing this out against amazon.ca where the TUF OC has a scalper trying to sell it for $1,879.99 CDN and it wasn't working.
This is what a price span looks like on amazon.ca:
<span id="priceblock_ourprice" class="a-size-medium a-color-price priceBlockBuyingPriceString">CDN$&nbsp;1,879.99</span>

I looked into it, and in the cardPriceLimit function, at the return statement, the cardpriceNumber is showing as 1.879 instead of 1879

@xninjax
Copy link
Contributor Author

xninjax commented Oct 2, 2020

Hey there, I was just testing this out against amazon.ca where the TUF OC has a scalper trying to sell it for $1,879.99 CDN and it wasn't working.
This is what a price span looks like on amazon.ca:
<span id="priceblock_ourprice" class="a-size-medium a-color-price priceBlockBuyingPriceString">CDN$&nbsp;1,879.99</span>

I looked into it, and in the cardPriceLimit function, at the return statement, the cardpriceNumber is showing as 1.879 instead of 1879

Thanks for letting me know about this! - I'll take a look. Perhaps the regex parsing needs some tweaking. Do you have a link to the item still? EDIT: I found the card on amazon. will test.

As for the price container - amazon currently using span[class*="PriceString"]' as a selector. It has wildcard matching, as sometimes the id changes to something like span id="priceblock_saleprice" while other times it's like what you posted here. The selector seems to have picked the correct container, but regex is not parsing the decimal place correctly now.

@DishBrain
Copy link

DishBrain commented Oct 2, 2020

Thanks for letting me know about this! - I'll take a look. Perhaps the regex parsing needs some tweaking. Do you have a link to the item still? EDIT: I found the card on amazon. will test.

As for the price container - amazon currently using span[class*="PriceString"]' as a selector. It has wildcard matching, as sometimes the id changes to something like span id="priceblock_saleprice" while other times it's like what you posted here. The selector seems to have picked the correct container, but regex is not parsing the decimal place correctly now.

Glad you found the card, the scalper has been putting it up and taking it down repeatedly, so it’s not always there with that price.

And yes, I was using the selector as you had it with the wildcard matching. I threw in some log statements and saw that it was grabbing that whole string of CDN$&nbsp;1,879.99. I didn’t break apart your one line that did the regex matching and joining after to see exactly where this was occurring.

It’s possible I messed something up, as I more or less manually copied your changes into my locally cloned master, for some reason git wouldn’t let me pull in your forked branch. So maybe I missed something when I was doing that, not sure.

@xninjax
Copy link
Contributor Author

xninjax commented Oct 2, 2020

Glad you found the card, the scalper has been putting it up and taking it down repeatedly, so it’s not always there with that price.

And yes, I was using the selector as you had it with the wildcard matching. I threw in some log statements and saw that it was grabbing that whole string of CDN$&nbsp;1,879.99. I didn’t break apart your one line that did the regex matching and joining after to see exactly where this was occurring.

It’s possible I messed something up, as I more or less manually copied your changes into my locally cloned master, for some reason git wouldn’t let me pull in your forked branch. So maybe I missed something when I was doing that, not sure.

Ok, found the issue.
After I changed to passing the regex as a variable into the parser (instead of hard-coded), I must have missed re-testing with prices that contain commas (or contains periods in the case of euro format). 🤦‍♂️

By removing the quotations around the regex stored into the priceSeperator variable, everything began to work again.

Should be: const priceSeperator = query.euroFormat ? /\./g : /,/g; <note no quotes around regex

Thank-you @HickTrick for pointing this out - big save!

(incoming commit soon)

Test on the amazon-ca scalper card
image

Test on Euro laptop EDIT: test on Euro laptop with cents (can never do too much thorough testing! 😄 )
image
image

@DishBrain
Copy link

Ok, found the issue.

(incoming commit soon)

Awesome, glad you found it and figured out the solution! Glad to help!

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.

Very thorough work done here. I really appreciate the time spent and the considerations you made. Thanks for making this project better!

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.

4 participants