Currency Conversion #536

vacom13 · 2021-11-18T07:04:57Z

Added a fully fledged currency conversion feature.
Implements #129

benbusby

Thanks @vacom13! I did a quick review, but haven't had a chance to manually test out how it works yet.

My main concern is the reliance on the obfuscated class names from Google. These are liable to change at any point in the future, so I try to not rely on them unless necessary. In this case, I think at least one solution would be finding the currency disclaimer link and then traversing up the tree until an expected class is found. That way there's only one class to change later (if it does change) rather than a large number of classes.

Otherwise it looks good so far!

app/static/js/currency.js

app/utils/results.py

app/routes.py

app/utils/results.py

benbusby · 2021-11-19T00:46:31Z

app/utils/results.py

+    """
+    # Element before which the code will be changed
+    # (This is the 'disclaimer' link)
+    element1 = soup.select_one('[class="nXE3Ob"]')


I think this is fine since it's a single class, but it kinda makes me wonder if we should start maintaining an enum of classes used by Google, and then reference them using clearer names. Then it would also only require modifying one file if the class names ever change. So something like:

class GClass(Enum): result_class = 'ZINbbc' currency_disclaimer = 'nXE30b' # etc

and then reference in these instances using something like:

element1 = soup.select_one(f'[class="{GClass.currency_disclaimer}"]')

Don't worry about doing any of that in this PR, I can handle it later. I'm more just curious if you had any thoughts?

Hey @benbusby. Sorry, I have been down with the flu and so didn't get time to check these out before 😅. I do think it would be a good idea to maintain an enum of classes used by google.

Maybe after merging this pr, I could take up that issue. Since I am the one who has been using these classes a bunch!

No worries! Hope you're feeling better!

vacom13 · 2021-11-19T06:32:52Z

@benbusby sure I will take a look at all of them 😅

vacom13 · 2021-11-26T13:52:23Z

@benbusby I have done all the changes. But it shows that the branch has conflicts. How do I got about this?

vacom13 · 2021-11-26T13:59:27Z

I will just open a new PR I suppose?

vacom13 · 2021-11-26T14:18:55Z

Okay, I think I got it 😅

vacom13 · 2021-11-26T14:24:22Z

I went ahead and did away with most of those google classes. They are just present in the CSS for now, I think. If you have an idea about how to go about those, I will gladly make the changes.

benbusby

Hey @vacom13, this looks good! One recommendation though (see below), and I'm also getting a 500 error if I search for an exact conversion (i.e. "11 eur to usd").

benbusby · 2021-11-28T03:24:45Z

app/utils/results.py

+        for i in range(4):
+            currency_link = currency_link.parent
+        currency1 = currency_link
+        for i in range(5):
+            currency1 = currency1.parent
+        for i in range(2):
+            currency1 = currency1.previous_sibling


I think looking for a specific class name would actually be a better approach -- if we switch to traversing up N number of parents, and the layout for the conversion card returned by Google gets changed at some point, it'll be more effort to determine how deep the child element is nested. As long as we keep it to one class name, it should be easy enough to maintain in the future.

Something like (pseudocode, not tested):

while currency_link.parent and 'ZINbbc' not in currency_link.attrs['class']: currency_link = currency_link.parent

vacom13 · 2021-11-28T12:05:15Z

@benbusby okay will tackle this right away 😅

benbusby · 2021-11-28T15:18:32Z

app/utils/results.py

+    """
+    soup = BeautifulSoup(response, 'html.parser')
+    currency_link = soup.find('a', {'href': 'https://g.co/gfd'})
+    if currency_link and currency_link.text.lower() == 'disclaimer':


Also, does this still work when the search language is changed? If not, it's probably good enough to just check if the currency link exists.

I will check it out

vacom13 · 2021-12-02T18:24:38Z

@benbusby So I was checking the languages and well, I am running into a value error. In the example below as you can see that the floating numbers are using commas.

benbusby · 2021-12-03T20:19:37Z

@vacom13 ah. You could probably just perform a quick replace on the value before converting to float, a la .replace(',', '.') (sorry if that doesn't format properly, I'm not at my computer).

vacom13 · 2021-12-05T12:21:59Z

@benbusby The format was fine. No worries. I did run into more issues using the .replace(). If I asked for the output of lets say 10,000 usd to inr, the output (in English) came out to be 752,415.00 Indian Rupee( Referring to currency2, the string that I am using to make my own conversion factor). The , in the result would then get replaced by a .. Similarly, I checked for the language Portuguese and they used , in place of . to indicate the start of the decimal numbers and . instead of , (Basically the roles of , and . were reversed). However, I think I have managed to make it work now.

benbusby

OK @vacom13, two more small changes (really just one change repeated twice) and then I'll merge. Thanks again for taking care of this! It seems to work really well, nice job.

app/utils/results.py

vacom13 · 2021-12-07T05:31:56Z

@benbusby I well fetched and merged the changes before committing my own and this seems to be giving a test failed error 😢. I don't think it has anything to do with my code

benbusby · 2021-12-07T05:50:59Z

@vacom13 yeah, that test fails randomly. I've been meaning to re-write it so that it's more reliable, but haven't gotten around to it yet. I just re-ran the test suite though and everything is passing now.

vacom13 · 2021-12-07T05:58:18Z

Okay Great!

FrankelJb · 2022-06-11T18:51:32Z

Sorry for the necrobump...

Experiencing this on 0.7.3. Running the Docker image on k3s. The issue seems to be resolved, started occurring again today. Logs below:

File "/usr/local/lib/python3.11/site-packages/flask/app.py", line 2446, in wsgi_app
response = self.full_dispatch_request()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/flask/app.py", line 1951, in full_dispatch_request
rv = self.handle_user_exception(e)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/flask/app.py", line 1820, in handle_user_exception
reraise(exc_type, exc_value, tb)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/flask/_compat.py", line 39, in reraise
raise value
^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/flask/app.py", line 1949, in full_dispatch_request
rv = self.dispatch_request()
^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/flask/app.py", line 1935, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/whoogle/app/routes.py", line 94, in decorated
return f(*args, **kwargs)
^^^^^^^^^^^^^^^^^^
File "/whoogle/app/routes.py", line 53, in decorated
return f(*args, **kwargs)
^^^^^^^^^^^^^^^^^^
File "/whoogle/app/routes.py", line 361, in search
conversion = check_currency(str(response))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/whoogle/app/utils/results.py", line 268, in check_currency
while 'class' not in currency_link.attrs or \
^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'attrs'

vacom13 added 6 commits November 18, 2021 12:26

Updated Currency Conversion Feature

d9b6b7c

Correcting syntax

9257733

Pep8 formatting changes

2158998

Pep8 updates

5b8aa37

Pep8 update

265f274

Pep8 update

3d1cb3d

benbusby requested changes Nov 19, 2021

View reviewed changes

vacom13 added 4 commits November 26, 2021 19:35

Requested Changes

b01f251

upstream update

9fad8f7

Adding Imports

c3a327b

Formatting Update

458ff47

benbusby linked an issue Nov 28, 2021 that may be closed by this pull request

[FEATURE] Currency conversion #129

Closed

benbusby requested changes Nov 28, 2021

View reviewed changes

benbusby reviewed Nov 28, 2021

View reviewed changes

vacom13 and others added 2 commits December 5, 2021 16:37

Merge branch 'benbusby:main' into main

5037370

Fixing Issues with the Conversion Feature

eac7c12

benbusby requested changes Dec 7, 2021

View reviewed changes

app/utils/results.py Outdated Show resolved Hide resolved

app/utils/results.py Outdated Show resolved Hide resolved

vacom13 and others added 2 commits December 7, 2021 10:58

Merge branch 'benbusby:main' into main

2378500

Minor changes

dfaa49b

benbusby approved these changes Dec 7, 2021

View reviewed changes

benbusby merged commit 7bea634 into benbusby:main Dec 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Currency Conversion #536

Currency Conversion #536

vacom13 commented Nov 18, 2021

benbusby left a comment

benbusby Nov 19, 2021

vacom13 Nov 23, 2021

vacom13 Nov 23, 2021

benbusby Nov 23, 2021

vacom13 commented Nov 19, 2021 •

edited

Loading

vacom13 commented Nov 26, 2021

vacom13 commented Nov 26, 2021

vacom13 commented Nov 26, 2021

vacom13 commented Nov 26, 2021

benbusby left a comment

benbusby Nov 28, 2021

vacom13 commented Nov 28, 2021

benbusby Nov 28, 2021

vacom13 Nov 29, 2021

vacom13 commented Dec 2, 2021

benbusby commented Dec 3, 2021

vacom13 commented Dec 5, 2021 •

edited

Loading

benbusby left a comment

vacom13 commented Dec 7, 2021 •

edited

Loading

benbusby commented Dec 7, 2021

vacom13 commented Dec 7, 2021

FrankelJb commented Jun 11, 2022

Currency Conversion #536

Currency Conversion #536

Conversation

vacom13 commented Nov 18, 2021

benbusby left a comment

Choose a reason for hiding this comment

benbusby Nov 19, 2021

Choose a reason for hiding this comment

vacom13 Nov 23, 2021

Choose a reason for hiding this comment

vacom13 Nov 23, 2021

Choose a reason for hiding this comment

benbusby Nov 23, 2021

Choose a reason for hiding this comment

vacom13 commented Nov 19, 2021 • edited Loading

vacom13 commented Nov 26, 2021

vacom13 commented Nov 26, 2021

vacom13 commented Nov 26, 2021

vacom13 commented Nov 26, 2021

benbusby left a comment

Choose a reason for hiding this comment

benbusby Nov 28, 2021

Choose a reason for hiding this comment

vacom13 commented Nov 28, 2021

benbusby Nov 28, 2021

Choose a reason for hiding this comment

vacom13 Nov 29, 2021

Choose a reason for hiding this comment

vacom13 commented Dec 2, 2021

benbusby commented Dec 3, 2021

vacom13 commented Dec 5, 2021 • edited Loading

benbusby left a comment

Choose a reason for hiding this comment

vacom13 commented Dec 7, 2021 • edited Loading

benbusby commented Dec 7, 2021

vacom13 commented Dec 7, 2021

FrankelJb commented Jun 11, 2022

vacom13 commented Nov 19, 2021 •

edited

Loading

vacom13 commented Dec 5, 2021 •

edited

Loading

vacom13 commented Dec 7, 2021 •

edited

Loading