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

Fix price in offering info #59

Conversation

VleraBllacaa
Copy link
Contributor

@VleraBllacaa VleraBllacaa commented Oct 19, 2023

I recreated test_data because it was not up to date with the latest website changes.
I fixed also the tests because didn't have the start_time and end_time added in the site (N/A) and also the same for days.

price = float(price_text.strip().removeprefix("$") or 0)

else:
price = 0
Copy link
Owner

Choose a reason for hiding this comment

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

This brings up a question for me as to whether or not "no price" is a valid price for a course offering and if that means something different from 0. What are you thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

@VleraBllacaa Sorry, I was a little quick with the merge, but I'd still be interested in your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonbiemond In my view, we can confidently assume that a course will always come with a specific cost. The previous issue arose when the site's structure was altered, causing it to consistently display a price of $0.

To enhance the user experience on our frontend, given that we anticipate having to pay a certain amount, excluding $0, I propose representing a $0 price with a hyphen ("-"). This change will make it less misleading for users, as they might otherwise think the course is free. Additionally, if you encounter a "-" in the price field on the table, it's a clear indicator that there might be script issues or alterations to the webpage's structure.

Copy link
Owner

Choose a reason for hiding this comment

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

@VleraBllacaa A quick check in the database confirms your assumption. I like your idea and have created an issue (#68), feel free to take it on!

@jonbiemond
Copy link
Owner

This looks awesome, thank you so much! Thanks for also updating the test data and tests, I know that's not exactly ideal in the way it is setup now. Perhaps in the future I change it to be pattern matching instead of literal values. And also add a test on live data that can be run periodically.

@jonbiemond jonbiemond merged commit 82dd4a4 into jonbiemond:main Oct 19, 2023
6 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.

2 participants