Dues estimator to reduce currencylayer usage #10767
Conversation
danieljames-dj
force-pushed
the
dues-estimator
branch
from
e28d3c5 to
0d4b87b
Compare
app/webpacker/components/CompetitionForm/FormSections/DuesEstimate.jsx
Outdated
Show resolved
Hide resolved
app/webpacker/components/CompetitionForm/FormSections/DuesEstimate.jsx
Outdated
Show resolved
Hide resolved
danieljames-dj
force-pushed
the
dues-estimator
branch
from
0d4b87b to
622f220
Compare
| import Errored from '../../Requests/Errored'; | ||
| import { fetchJsonOrError } from '../../../lib/requests/fetchWithAuthenticityToken'; | ||
|
|
||
| const CALCULATE_DUES_QUERY_CLIENT = new QueryClient(); |
There was a problem hiding this comment.
Usually we wrap the component in a query client. I don't really know what the difference is though.
There was a problem hiding this comment.
I'm not sure which is the way to be followed. I followed this way because this is how I did in my first use-query implementation of this repo, and I think I copied this way from somewhere else in the same repo.
Please let me know if this needs to be changed, I'm totally fine with switching if the other method is better.
Looking forward for the opinion of @gregorbg too.
There was a problem hiding this comment.
We wrap it everywhere we can. To my knowledge, there is one instance of a const client being passed around, which is WcaSearch (and IdWcaSearch in the same file). That's because these are form components where we cannot be sure in which contexts they are used, and under what circumstances a client may or may not be available.
There was a problem hiding this comment.
In your case, you are not providing an "entry point" component that is injected into our frontend with react_component. So having it this way is fine.
In the future, when you're working with components which are intended to be "entry points" for our Rails erb code using react_component, then you should definitely wrap in a client provider.
| const { | ||
| data, isLoading, isError, | ||
| } = useQuery({ | ||
| queryKey: ['calculateDues'], |
There was a problem hiding this comment.
Does this work? The keys need to include everything that the query depends on. Otherwise, if the query is re-run with a different say base entry fee, then it will retrieve the previous cached data (based on the previous base entry fee), since the keys match. I think.
There was a problem hiding this comment.
Thanks for that. I've changed it, I'm pretty new to useQuery so didn't knew the theory much.
Apparently this was working with constant queryKey as well apparently. I changed the registration fees and opened the component again, and the output is different, and not from cached. I'm not sure why, maybe in local there is no cache.
There was a problem hiding this comment.
I think that since the query client is re-created every time the component is first rendered (since it's defined in the dues estimate file rather than wrapping the entire form), so it's a fresh instance with no cache each time? And you can't change any of the keys without first closing the component and then re-opening it fresh.
Generally, I think the best approach is a single query client at the root of the app, but we can't do that since rails is rendering react components individually, so we don't have a single root in the react world (if I understand correctly).
There was a problem hiding this comment.
That makes sense. I guess these will be changed after the nextJS integration.
danieljames-dj
force-pushed
the
dues-estimator
branch
from
622f220 to
6649604
Compare
| render json: { | ||
| dues_value: total_dues.present? ? total_dues.format : nil, | ||
| dues_value: per_competitor_dues.to_f, |
There was a problem hiding this comment.
Why are you converting this to a float?
There was a problem hiding this comment.
Or to be precise, why are you converting this to a float here? Communicating floating point numbers is always dangerous because of rounding errors, overflows, etc. If the money library provides an integer return value by default, it probably does so for a good reason.
If you want to display an float-like value (with decimal point) to the user, then you are of course free to do so. But the conversion from int to float should only happen immediately before you display it in the frontend.
There was a problem hiding this comment.
Okay, I'm now passing the lowest denomination value and converting it in the frontend. Hope this works.
| import Errored from '../../Requests/Errored'; | ||
| import { fetchJsonOrError } from '../../../lib/requests/fetchWithAuthenticityToken'; | ||
|
|
||
| const CALCULATE_DUES_QUERY_CLIENT = new QueryClient(); |
There was a problem hiding this comment.
We wrap it everywhere we can. To my knowledge, there is one instance of a const client being passed around, which is WcaSearch (and IdWcaSearch in the same file). That's because these are form components where we cannot be sure in which contexts they are used, and under what circumstances a client may or may not be available.
| import Errored from '../../Requests/Errored'; | ||
| import { fetchJsonOrError } from '../../../lib/requests/fetchWithAuthenticityToken'; | ||
|
|
||
| const CALCULATE_DUES_QUERY_CLIENT = new QueryClient(); |
There was a problem hiding this comment.
In your case, you are not providing an "entry point" component that is injected into our frontend with react_component. So having it this way is fine.
In the future, when you're working with components which are intended to be "entry points" for our Rails erb code using react_component, then you should definitely wrap in a client provider.
For getting currency details, we moved to currencylayer API which is paid. Currently, the dues is shown on competition edit page, but it may not be noticed every time by the user. So it has been replaced with a button now as shown below:
When the user clicks the button, they will be able to see the dues estimate. This view not only shows the dues estimate, but also allows user to change the competitor count temporarily and see what is the dues for that competitor count:
The future plan of this component will be to explain the dues calculation method used in this component, so that delegate can easily understand it from here, and hence reduce queries to WFC on 'why is the dues xyz for n competitors'.