-
Notifications
You must be signed in to change notification settings - Fork 19
Feature/currency preference #17
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
Feature/currency preference #17
Conversation
cachebag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ensure CI passes too!
backend/src/handlers/auth.rs
Outdated
| // ADDED: Fetch currency | ||
| let currency: (String,) = sqlx::query_as("SELECT currency FROM users WHERE id = ?") | ||
| .bind(claims.sub) | ||
| .fetch_one(&pool) | ||
| .await?; | ||
|
|
||
| Ok(Json(AuthResponse { | ||
| id: claims.sub, | ||
| username: payload.new_username, | ||
| currency: currency.0, // ADDED | ||
| })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The // ADDED comments don't seem to be needed? Same with line 184 above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed on latest PR
| #[derive(Deserialize, ToSchema, Validate)] | ||
| pub struct ChangeCurrencyRequest { | ||
| #[validate(length(min = 3, max = 3))] | ||
| pub currency: String, | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows any three-character string (e.g. "FOO", "BAR", "$$$"). The backend should not accept values outside the supported currency set enforced by your frontend changes
backend/src/handlers/auth.rs
Outdated
| .execute(&pool) | ||
| .await?; | ||
|
|
||
| let user: (String,) = sqlx::query_as("SELECT username FROM users WHERE id = ?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that this is necessary. We already do this?
const response = await api.auth.changeCurrency(...)
updateCurrency(response.currency)username is ignored. I'm pretty sure we can just omit it.
|
Ran CI tests locally and everything was fine - will take a look tomorrow 🙃 ps what features are you looking to add? I noticed 2 others submitted currency features I’m working on: Multiple accounts |
Yeah, a couple others decided to implement this. If this PR was a bit cleaner, I'd consider merging but it looks like one of the others ones handle this a bit better.
This is already implemented, unless I am misunderstanding what you mean by "multiple accounts".
This is also already implemented, albeit for JSON only. Can't imagine CSV format is needed.
This would be a cool addition, I'd be happy to review that PR. |
|
Closing in favor #18 |
Description
Adds user-configurable currency preference to display amounts in their preferred currency.
Changes Made
Backend
currencycolumn to users table with migration (defaults to USD)AuthResponseto include currency field/api/auth/change-currencyendpointFrontend
lib/currency.ts)useCurrencyhook for easy component usageSupported Currencies
USD ($), EUR (€), GBP (£), JPY (¥), CAD (C$ ), AUD (A$), CHF, CNY (¥), INR (₹)
Testing