Add election name to IEC contribution list #466
Conversation
There was a problem hiding this comment.
Mostly seems fine. I think the election.name comparison bit is not working like you intended it to. Other than that, one or two code style comments, and a (maybe) warning about allowing JS to (maybe) handle strings as numbers. I had forgotten just how loosey-goosey JS is. I kind of hate it and I love it in equal measure.
| const election1 = parseElectionName(x); | ||
| const election2 = parseElectionName(y); | ||
|
|
||
| if (election1.name > election2.name) { |
There was a problem hiding this comment.
I'm confused how this is working. It looks to me like parseElectionName returns an object with a property ename, rather than name. Is it possible these cases are never hit because it's always comparing undefined to undefined?
What's the desired behavior here, that elections would sort alphabetically by locale?
| return -1; | ||
| } | ||
|
|
||
| if (election1.year > election2.year) { |
There was a problem hiding this comment.
This is comparing strings. Not sure if there's any "gotchas" with > & < comparing strings in JS. Could be fine, I don't know. Just wanted to point it out in case you'd be concerned.
There was a problem hiding this comment.
Actually it occurs to me that we do not need to (or maybe should not) sort on name. The names will be all the same. I don't think that a IEC will report collections in two different jurisdictions, but if they did then we would want the order to be on the date, regardless of the jurisdiction.
There was a problem hiding this comment.
One thing I forgot to mention in earlier review: I am running into this error with Node 18. SO answers suggests downgrading to Node 16.13.1 (which I did not try) or passing the option --openssl-legacy-provider in NODE_OPTIONS env var, which did work for me. We should either downgrade or set that env var on the npm command that runs Webpack, if possible.
|
@ckingbailey I believe I have addressed all you concerns. |
|
@mikeubell what do you want to do about the Node 18 issue? |
|
I don’t feel qualified to know what the right call is. I see that this commit makes that change. I think its best to to stay as current as possible with the tools as it becomes harder to keep things running as they get older. I guess passing the flag would be the thing to do. |
mikeubell
force-pushed
the
add-election-name
branch
from
61ba67c to
d9b1786
Compare
mikeubell
force-pushed
the
add-election-name
branch
from
d9b1786 to
43c92e3
Compare
This work resolves #435
Since Independent committees can raise money in one election and spend it in another, we are leaving all contributions to a committee in the list but sorting it so that the most recent election's contributions appear first. This is true even when sorting is specified on a different columng
Previews