Auto-approve trippees

[ziruihao]

To test, reseed the database, then login as userId = 1 and open tripId = 5, called "POCO x NAD Sugaring Trip!🍁🥞".

• Send "Leader 1" back to the waitlist
• This should auto-approve "Trippee 1" and "Tripped 2"
• Send another back to the waitlist
• This should auto-approve "Trippee 3"

Resolves #46

[alexpetros]

I can't think of any reason why this function should return a function instead of just being:

export function autoApprove(db, tripId)

Am I missing something?

[ziruihao]

It was an overkill here, but the idea is that tripId and db are very different kinds of input. They're different in where those values are available. So tripId is readily available in any business logic, but db is provided only through the Express middleware. The idea is a function that only takes tripId may be useful in other parts of the stack that doesn't necessarily have access to db.

[alexpetros]

I know what you're thinking, but this function in particular is never going to work that way, because as autoApprove, it's always going to need DB access to do its job.

[alexpetros]

Don't think we need this—the order they're in the table based on the ROWID is going to be the order that they were added at. To create new ROWIDs, SQLite basically does SELECT max(id) + 1.

trip_members does not have an explicit INTEGER PRIMARY KEY, but it still has a hidden one, essentially, because INTEGER PRIMARY KEY is just an alias for ROWID, which SQLite will always include unless you explicitly ask it not to.

[ziruihao]

In this case trip_members table has a PRIMARY KEY (trip, user) and so it ends up sorting by the trip ID then user ID, instead of order added. It definitely would've been nice to just rely on insertion order.

[alexpetros]

Normally you would be right, but SQLite works a little differently in this regard (namely, you always have ROWID unless you explicitly say WITHOUT ROWID). See: https://www.sqlite.org/rowidtable.html

[alexpetros]

Also add: "Resolves: #46" at the end of the description so that it links to that issue and auto-resolves it when the PR gets merged.

[alexpetros]

I had a substantial comment that got lost somehow, sorry to add this after.

A while loop is not the way to go here, for two reasons:

1. If there's any possibility of the while loop not terminating, it will crash the program by blocking the event loop. The other two while loops in this code have much simpler terminating conditions, and writing this makes me think I should re-evaluate them, honestly.
2. Generally speaking, it is much simpler (and more efficient) to let SQL handle operations over multiple entries, in this case meaning "all the people who are pending admittance". Think functionally (operations over arrays) not procedurally (loops).

A better way to do it is the following:

1. If the time is less than 24 hours from now, return
2. Get the total number of members currently in the trip
3. Get the number of auto approved members
4. Let X be auto_approved_members - total_members. If X >= 0, get the pending trip members, limit X, ordered by ROWID
5. Admit them to the trip
6. Send them emails

No loops required (though a map or forEach is the simplest way to do the emails). I would do (4) and (5) as virtually identical statements, with (5) as an update; this is much easier than trying to do an array input to SQL. (2) and (3) can be combined into one statement pretty easily, if you want to test your SQL.

[alexpetros]

Total style nitpick, but I think you're over-explaining yourself here. Consider:

  // Remove then re-add member so that they're at the back of the auto-approval queue
  req.db.run('DELETE FROM trip_members WHERE trip = ? and user = ?', tripId, userId)
  req.db.run('INSERT INTO trip_members (trip, user, leader, pending) VALUES (?, ?, ?, ?)', tripId, userId, 0, 1)

  autoApprove(tripId)(req.db)

[alexpetros]

Now that I think about it, the UX is going to be pretty bad here if you're sending people to the waitlist but the auto-approval is on and it adds them right back. What do you think is the right solution to that? I think maybe we should disable "send to waitlist" entirely if the auto-approval is on.

[alexpetros]

I have thought about this more and let's do it this way: can't send anyone to the waitlist if you're auto-approving. Makes everything cleaner, less likely to have unexpected behavior.