voteamerica / backend

MIT License
7 stars 17 forks source link

operator table not aligned with submit_new_user function #206

Closed dmilet closed 5 years ago

dmilet commented 5 years ago

-- -- Name: operator; Type: TABLE; Schema: carpoolvote; Owner: carpool_admins

CREATE TABLE operator ( "UUID" character varying(50) DEFAULT gen_random_uuid() NOT NULL, "email" character varying(250) NOT NULL, "username" character varying(250) NOT NULL, "password" character varying(250) NOT NULL, "admin" boolean NOT NULL, "UUID_organization" character varying(50) );

But in submit_new_user, the table columns are different: BEGIN
out_uuid := carpoolvote.gen_random_uuid();

    INSERT INTO carpoolvote.operator(
    "UUID", "email", "username", "userpassword", "userIsAdmin")
    VALUES (
    out_uuid, email, username, userpassword, userIsAdmin
    );

    RETURN;
EXCEPTION WHEN OTHERS
THEN
    out_uuid := '';
    out_error_code := carpoolvote.f_EXECUTION_ERROR();
    out_error_text := 'Unexpected exception in submit_new_user, ' || v_step || ' (' || SQLSTATE || ')' || SQLERRM;
    RETURN;
END;
dmilet commented 5 years ago

@jkbits1 Jon, I think we should rename the function submit_new_user to submit_new_operator and align with the table. What values do you pass from the front end ?

jkbits1 commented 5 years ago

@dmilet Dave, thanks for taking a look at this đź‘Ť yes, there is a bit of misalignment here. Arash noticed this, too, so this is a bit of a rehash of my reply.

In a quick summary of all my comments below, if we do rename something, my preference is to rename the table to user (or something similar that’s not a reserved word). It's also the smaller number of changes to make, which is in part because that reflects my overall thoughts on the naming choices.

I have a feeling that I tried to name the table user but that was a reserved word in Postgres and I wasn’t so definite on the name to override that. That might be why the primary key is named user_pk.

From a while back, with my thinking about this facility, there seems a case to be made for the name of either user or operator. My general preference is for user, as the facility is for operator now, but maybe in future we’ll want other user types, and it would then be more awkward to be using a specific term (operator) rather than a general one (user).

Even though the table is named operator, the node app is completely consistent on the name user, and the db function has the name it does to align with the node app naming. In the node app, there are constants defined for table names and the constant for the table is operator.

On the front-end, the new page is an operator page but all the node app routes (as in my above comment for testing) are based around the word user.

dmilet commented 5 years ago

would tb_user be ok ? It will be transparent to you as I'll update the submit_new_user function as well

LIVE=# \d+ carpoolvote.tb_user; Table "carpoolvote.tb_user" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description -------------------+------------------------+-----------+----------+-------------------+----------+--------------+------------- UUID | character varying(50) | | not null | gen_random_uuid() | extended | | email | character varying(250) | | not null | | extended | | username | character varying(250) | | not null | | extended | | password | character varying(250) | | not null | | extended | | is_admin | boolean | | not null | false | plain | | UUID_organization | character varying(50) | | | | extended | | Indexes: "user_pk" PRIMARY KEY, btree ("UUID") Foreign-key constraints: "user_uuid_organization_fkey" FOREIGN KEY ("UUID_organization") REFERENCES organization("UUID") ON DELETE CASCADE

jkbits1 commented 5 years ago

@dmilet that sounds great, thanks đź‘Ť if you create a PR for that, I'll either merge it when I see it or you can simply merge it straight away

jkbits1 commented 5 years ago

@dmilet looks like we tidied up everything we needed to, so closing this