Configurable member types #50
Conversation
There was a problem hiding this comment.
Looks like a great change, I think this is going to make things far more configurable.
Just a few suggestions...
- There's a stray FIXME without context. Could we fix it or leave a hint about what needs fixing?
- A few of these functions are getting large. I'd love to see them broken up into named bits to make it clearer what they're doing.
- There's complex logic generating SQL that is an excellent candidate for it's own function.
- Spotted a few places with magic numbers. Comments or named constants would make it easier to follow.
- Some small tests would be wonderful :)
Hope this helps, push back if any of it doesn't sit the style of the project.
kansa/lib/mail.js
Outdated
| .map(({ id, name }) => ({ id, name })) | ||
| return { | ||
| attending, | ||
| email: res[0].email, | ||
| hugo_members, | ||
| key: res[0].key, | ||
| membership: mt[mi], | ||
| membership: res[0].membership, // FIXME |
There was a problem hiding this comment.
What needs fixing here? Could you describe it in this comment, or just fix it ;-).
There was a problem hiding this comment.
Fair enough. How about just putting that in the comment?
// FIXME will come back to this for issue #49
kansa/lib/people.js
Outdated
| data.email ? tx.oneOrNone(`SELECT key FROM Keys WHERE email=$(email)`, data) : {}, | ||
| tx.none(`INSERT INTO Log ${log.sqlValues}`, log) | ||
| ])) | ||
| .then(([{ hugo_nominator, hugo_voter, next_email, prev_email, name }, key]) => { | ||
| .then(([{ hugo_nominator, wsfs_member, next_email, prev_email, name }, key]) => { |
There was a problem hiding this comment.
This method is getting really large. Would you consider extracting some of this into callable functions?
kansa/lib/public.js
Outdated
| FROM people p | ||
| LEFT JOIN membership_types m USING (membership) | ||
| WHERE ${queryParts.join(' AND ')} AND | ||
| m.allow_lookup = true`, queryValues |
There was a problem hiding this comment.
The queryParts construction logic is looking quite complex. Could we extract this to a function?
This would make it easy to write a little test ;-).
kansa/lib/purchase.js
Outdated
| const mp = prices[p.data.membership] | ||
| if (typeof mp !== 'number' || mp < 0) { | ||
| throw new InputError(`Membership type not available for purchase: ${JSON.stringify(p.data.membership)}`) | ||
| } |
There was a problem hiding this comment.
This method is getting really large. Would you consider breaking up into more functions?
kansa/lib/siteselect.js
Outdated
| FROM people p | ||
| LEFT JOIN siteselection_votes s ON (p.id = s.person_id) | ||
| LEFT JOIN membership_types m USING (membership) | ||
| WHERE p.id = $1 AND m.wsfs_member = true`, id) |
There was a problem hiding this comment.
Magic numbers in switch case. Is there any way we could name these numbers?
Either that or a nice little comment about what they represent?
Vote seems like an important function. Could we get a little test going for it?
There was a problem hiding this comment.
Tbh I can't remember why I ended up doing transactions this way; probably misunderstood that a construction like db.tx(t => t.one(...).then(res => t.one(...))) wouldn't work for some reason. Should refactor all of these.
kansa/lib/upgrade.js
Outdated
| email = prev.email; | ||
| member_number = prev.member_number; | ||
| const log = new LogEntry(req, `Upgrade to ${data.membership}`); | ||
| if (data.paper_pubs) log.description += ' and add paper pubs'; | ||
| log.subject = data.id; | ||
| return tx.none(`INSERT INTO Log ${log.sqlValues}`, log); | ||
|
|
||
| case 3: | ||
| case 4: |
There was a problem hiding this comment.
More magic numbers, could we name these or leave a comment about them?
What happens when i doensn't fit between 0 and 4?
|
Do we need to make changes to the importer too? |
|
Yeah, the importer will need to be updated as well. Should also add tests for member data updates, voting, and a bunch of other things. |
Replaces the
MembershipStatusenum with a tablemembership_typesin Postgres, and uses its data rather than hard-coded values for related processing.TODO: Improve documentation, adding database update instructions