LearnersGuild / echo

learning management system
MIT License
3 stars 31 forks source link

Changes userCreate to hard-fail if already exists #1092

Closed lumodon closed 6 years ago

lumodon commented 6 years ago

Fixes #1090

Overview

In echo's userCreate worker we were upserting idm users to echo members. Now we have added check that idm user isn't already a member of echo upon insertion. We remove a unit test that checked userCreate was not failing when updating, and replaced it with a unit test that ensures it does fail when adding a member who already exists (i.e. remove update functionality)

Data Model / DB Schema Changes

None - isolated to worker functionality

Environment / Configuration Changes

None

Notes

  1. Remaining concerns: Should we only fail-hard when the member who's being added already has chapter information and not just always fail? Without end-to-end testing, was anything dependent up on the upsert functionality - updating echo members using a create? (hopefully not that feels unintuitive)
  2. According to https://www.rethinkdb.com/api/javascript/insert/ we shouldn't have to make a resource expensive data call to Members to check if member already exists, and insert default functionality has {conflict: "error"} as it's default setting - therefore inserting should error out on it's own. It doesn't though. Is this because we don't have primary keys that indicate to Thinky that we're creating a conflicting id? Console logging indicates that it simply inserts twice without error even when not using upsert.
  3. Why does await Member.filter({id: idmUser.id}) work but await Member.get(idmUser.id) not work?
heyheyjp commented 6 years ago

@lumodon: because of significant changes in #1081 (recently merged to master) -- one of which is a fix for this very same issue -- I'm going to close this PR as it's no longer necessary. Thank you for the submission! Very much appreciated.