Skip to content
This repository has been archived by the owner on Jul 17, 2020. It is now read-only.

Donation Values Treated as Strings #243

Closed
davycheung opened this issue Sep 22, 2017 · 9 comments · Fixed by #246
Closed

Donation Values Treated as Strings #243

davycheung opened this issue Sep 22, 2017 · 9 comments · Fixed by #246

Comments

@davycheung
Copy link
Contributor

Looks like the donation values are being concatenated instead of being summed up.

screen shot 2017-09-21 at 9 59 03 pm

screen shot 2017-09-21 at 9 59 34 pm

@cjhaviland
Copy link

I am just getting my local instance setup but I'd like to take a crack at this if no one minds.

@ik226
Copy link
Contributor

ik226 commented Sep 26, 2017

just saw this comment.. I have just made PR on this. Can you please look into it and let me know if you have other approaches to fix this issue? Thanks!

@jspaine
Copy link
Contributor

jspaine commented Sep 26, 2017

Yeah that looks fine. Sorry @cjhaviland, hope you can find something else you'd like to try.

Edit: actually, donation values are also summed on the server (see #245) to generate the receipt email, guess this will be a problem there too so maybe it's better to make sure they're stored as numbers instead.

@cjhaviland
Copy link

I think wrapping it in a number should be fine, but like I said I haven't gotten a chance to get everything up and running yet to look. I would definitely check that there is validation on the input so we can only accept numbers.

@jspaine
Copy link
Contributor

jspaine commented Sep 27, 2017

The value input on the client and the mongoose schema are both type number. Not sure why this is a problem, and only with user entered data (seems fine with seed data).

@ik226
Copy link
Contributor

ik226 commented Sep 28, 2017

I believe that redux-form is returning values as string. (In components/DonationCreate.js, we are converting item.value to Number in order to calculate total item amount due to this reason).

@kenjiO
Copy link
Contributor

kenjiO commented Sep 28, 2017

I think it would make the backend more robust not to assume that the item.value property is going to be a number. If another api client is ever used or something breaks the validation in the front end it would be good for the server to handle something that can't be converted to a number. Maybe
change (acc, item) => acc + Number(item.value) to something like

(acc, item) => {
  // TODO:  check if Number(item.value) is NaN.  If so throw BadRequestError
  return acc + Number(item.value) 
}

@cjhaviland, do you want to take this?

@kenjiO kenjiO reopened this Sep 28, 2017
@kenjiO
Copy link
Contributor

kenjiO commented Nov 6, 2017

We still need to validate that the value properties for the items are numeric on the server. If anyone ones to take this please leave a comment.

@logangingerich
Copy link
Contributor

I should be able to work on this over the next few days!

logangingerich pushed a commit to logangingerich/pantry-for-good that referenced this issue Nov 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants