Fix #42: Improve price string parsing.

[biancadanforth]

• Update how we pull the price string from extracted Fathom price elements to provide main and subunits (e.g. dollars and cents) if available.
• Add price string cleaning method to remove extra characters (like commas) that were causing price parsing to fail.
• Handle case when price string parsing fails after successful extraction by returning null from extractProduct in fathom_extraction.js (the same thing that happens for unsuccessful extraction).
  • This will guarantee we never see the “blank panel” reported in Improve price string parsing when there is a single price on the page #79 and Etsy: Partial Doorhanger on Product Page #88.
  • PR Fix #84: Improve fallback extraction #114 will ensure fallback extraction also returns null when price string parsing fails.

Price element innerText strings now supported as a result of these changes:

• "$1327 /each" (Home Depot example page)
• "$1,049.00" (Amazon example page)
• "US $789.99" (Ebay example page)
• "$4.99+" (Etsy example page)

Note: This does not handle the case where there is more than one price for the product page (e.g. if we see a range of prices such as "$19.92 - $38.00" or if the price changes based on size/color, etc.); that’s handled by Issue #86.

[biancadanforth]

@Osmose , ready for your review!

In particular, I wasn't sure the best way to handle the main/subunits situation inside of fathom_extraction.js. Right now, if there are subunits, extractedElements.price maps to an object literal, else an HTML object. extractedProducts.price from extraction (what we send on to the background script) is always a string though.

[Osmose]

This is a great first pass, nice work!

Thinking about the original issue, my first thought of how to frame the problem would be this:

Currently, we treat the price as a string and the . as a singular separator between unit and subunit of currency. Some sites, however, split the subunit into a separate DOM element without a . between.

So the goal should be to describe price parsing like this:

1. Start at the first digit after a $ symbol, or the first digit if there is no $ symbol.
2. Add digits to the unit number, ignoring , symbols, until a "separator" is found
   • A separator is defined as either a . symbol, or a break in DOM elements.
3. When a separator is found, convert the collected unit number to an integer, and store as a unit integer in a list.
4. Repeat steps 2 and 3, appending unit integers to the list, until the last digit is found. That is the final unit integer.
5. If the unit integer list has 0 or > 2 elements, parsing fails. Otherwise, multiply the first unit integer by 10, add the second unit integer to it (if it exists), and return that as the currency amount.

The hard part, of course, is treating both a . and a DOM element break the same: as separators. You can do that in two steps: First split the matched element into child elements and take their innerText values, then split each innerText value and split it on . symbols, and flatten to a single list.

Lemme know if that helps!

[Osmose]

Essentially, this adds an intermediate state for prices:

Price string from page -> Price string in "$NX.XX" format -> Integer amount in subunits (e.g. 1000 for $10)

There's not much point in having the intermediate format. Previously, we sent the price string from the page through until we created the price object, and then converted it to subunits.

There's two places it makes sense to perform the conversion from "price string from page" to "integer amount":

1. At the point of extraction, i.e. the content script
2. At the point of persistence, i.e. priceFromExtracted.

It looks like this PR is favoring (1) over (2), which is fine. That means the content script should probably be sending the extracted product with an integer amount instead of a string.

[Osmose]

An if statement in a loop that only affects one of the loop iterations is typically a code smell; it's hard to keep track of mentally, and doesn't scale well as the special cases grow. In cases like this, the better solution is to defer to the things you're looping over for their behavior.

In other words, it seems like the specific special cases for each feature in PRODUCT_FEATURES is spread amongst a few different places:

• extractProduct pulls a different attribute for the image feature only.
• runRuleset is now performing some extra post-processing for price features.
• Not right now, but a future version of hasAllFeatures could be doing some extra validation as well.

It'd be better if each feature was a thing (an object?) that knew all the special cases it needs. Maybe something like:

const FEATURE_DEFAULTS = {
  getValueFromFnode(fnode) {
    return fnode.element.innerText;
  },
};

const PRODUCT_FEATURES = {
  image: {
    ...FEATURE_DEFAULTS,
    getValueFromFnode(fnode) {
      return fnode.element.src;
    },
  }.
};

It's not a complete example but that's one possible shape. If you gather all the feature-specific needs in those objects, you can write generic loops and other things for the rest of the code that don't need to worry about special cases.

[Osmose]

Having the returned extracted element sometimes be a special "price object" and testing that later is harder to maintain than being consistent. You may have to refactor more than just this method, but consistency in the values of maps or arrays is very valuable for writing maintainable code.

My first instinct is that runRuleset being a standalone method from extractProduct may not be the best split of functions.

[Osmose]

The way this is implemented, the last element containing a digit is the subunit, not the second.

Instead of returning an object with two entries and no way to distinguish weird edge cases (such as more than 2 elements with digits), what if this just returned a list of all the child elements containing digits? Then a method calling this could fail if more than two results are in the returned array, as that's not a case we are capable of handling right now.

[Osmose]

Same point above about spreading customization per-feature across several methods applies here too.

[Osmose]

The priceStr variable isn't providing any clarity here, we could just assign the value directly.

[Osmose]

If the elements themselves are never used except for getting their innerText, there's no point passing them around as elements until now, just pass around the innerText value from the start.

[biancadanforth]

@Osmose Ready for a re-review; thank you for the feedback. Also I'm standing next to you. :)

[Osmose]

Thanks for the update! The new feature list is a great improvement. Nice work.

[Osmose]

yassssss

delete that code yassssssss

[Osmose]

No need for this variable, just pass it directly to getAllTokens.

[Osmose]

This function and getPriceTokens are small enough that we can probably just fold them into getAllTokens and use the docstrings as comments above them. Then getAllTokens completely abstracts the token-processing pipeline to make getPriceIntegerInSubunits simpler.

[Osmose]

The Integer part of the method name is unnecessary, the InSubunits bit is clear enough about the nature of the conversion.

[Osmose]

Instead of an early exit and this ternary statement, it may be clearer to check for successful cases and then default to returning NaN as a failure case at the end.

If the getAllTokens method is updated to return "units" (i.e. have it run the parseInt over every unit it discovers, and call it extractUnits) then this method can be simplified even more.

[Osmose]

The thing being passed in isn't a list of tokens, it's a list of child nodes. Presumably tokens are what's being returned?

[Osmose]

We're also using dom node splits as a separator.

[biancadanforth]

Yes, but not in this method.

[Osmose]

You can use split and flat to simplify this:

const allTokens = (
  tokens
  .map(token => (
    token.textContent.split(/[,$]/) // Split by , or $
  ))
  .flat()
);

You don't need to worry about empty strings at the end since the "has a digit" test later on will strip them.

[Osmose]

Oh wait, @mythmon keeps telling me to use flatMap and he's totally right:

const allTokens = tokens.flatMap(token => token.textContent.split(/[,$]/));

[biancadanforth]

Wow.

[biancadanforth]

@Osmose Ready for another look. Greatly simplified and rebased.

I noticed here and on master, the live loading that should be working from #115 is not working anymore... I'll look into it tomorrow.

[Osmose]