1. BIG server clean & API response schema standardization #266

mgreenw · 2019-02-03T23:27:57Z

Ok so I did this in a really dumb order and never committed and it was bad. I'm really sorry. I will not do that again. Here's what changed:

Removed "splash photo" and removed restriction on deleting the last photo
Standardized API response: every endpoint that returns data now returns it in a "data" field.
Centralized status code location to match response body status.
Standardized profile schema:

{
  fields: {
    displayName: string,
    bio: string,
    birthday: string
  },
  photoIds: Array<number>
}

mgreenw · 2019-02-04T02:53:08Z

src/server/api/photos/confirm-upload.js

+            FROM inserted
+          )
+        ) AS "photoIds"
+    `, [userId, photoCount + 1, uuid, userId]);


Return the new photoIds!

mgreenw · 2019-02-04T02:53:45Z

src/server/api/photos/delete-photo.js

-    return apiUtils.status(409).json({
-      status: codes.DELETE_PHOTO__CANNOT_DELETE_LAST_PHOTO,
-    });
+    return apiUtils.status(codes.DELETE_PHOTO__NOT_FOUND).noData();
  }


This removes the server check that it is a user's last photo.

mgreenw · 2019-02-04T02:53:57Z

src/server/api/photos/delete-photo.js

-        SET splash_photo_id = $1
-        WHERE user_id = $2
-      `, [photos[0].id, userId]);
-    }


Delete "splash photo id"

mgreenw · 2019-02-04T02:54:32Z

src/server/api/photos/delete-photo.js

+            ${updatedPhotos.join(',')}
+          ) AS updated_photos (id, index)
+        WHERE photos.id = updated_photos.id
+      `);


No need to reorder the photos if there are no photos! This just wraps the old query in an if statement

mgreenw · 2019-02-04T02:54:43Z

src/server/api/photos/delete-photo.js

+
+    return apiUtils.status(codes.DELETE_PHOTO__SUCCESS).data(
+      _.map(photos, photo => photo.id),
+    );


Return the new photo order

mgreenw · 2019-02-04T02:55:20Z

src/server/api/photos/get-photo.js

-      status: codes.GET_PHOTO__NOT_FOUND,
-    });
+    // $FlowFixMe I'm really not sure what's going on here...it's only HERE
+    return apiUtils.status(codes.GET_PHOTO__NOT_FOUND).noData();


This is weird...I'm not worried about it and I'm pretty sure it's flow getting confused. Will look into it more

Spent a little time debugging this. I think the root of this is the implicit types returned by functions; feels like it should work, perhaps related to:
facebook/flow#5294

I tested out a solution there by explicitly typing the return type (which is safe, as flow does make sure it's a valid return type).

const foo: { body: { status: string }, statusCode: number } = apiUtils .status(codes.GET_PHOTO__NOT_FOUND) .noData(); return foo;

Worth looking into later the deeper cause of this, but how about for now at least keeping this with a TODO comment so that the export here is well typed

mgreenw · 2019-02-04T02:55:56Z

src/server/api/photos/get-photo.js

+      // If the photo was succesfully retrieved, redirect to it!
+      // NOTE: This is "abnormal" by the standards of the API
+      if (photoRes.body.status === codes.GET_PHOTO__SUCCESS.status) {
+        return res.redirect(photoRes.body.data.url);


This handles the redirect. Internally, we still have a GET_PHOTO__SUCCCES response

mgreenw · 2019-02-04T02:56:26Z

src/server/api/photos/reorder-photos.js

-  return apiUtils.status(200).json({
-    status: codes.REORDER_PHOTOS__SUCCESS,
-    photos: newOrder,
-  });


Delete stuff about splash photo

mgreenw · 2019-02-04T02:57:15Z

src/server/api/relationships/get-matches.js

@@ -4,6 +4,7 @@ import type { $Request } from 'express';

 const db = require('../../db');
 const apiUtils = require('../utils');
+const { profileSelectQuery } = require('../users/utils');


This allows for a standard profile select query. This is nice and standardized and I use it in a few places.

mgreenw · 2019-02-04T02:57:58Z

src/server/api/relationships/get-scene-candidates.js

-    candidates: result.rows,
-  });
+
+  `, [userId, excludedUsers]);


I realized you can reuse paramaters...duh. Also it is better practice to USE paramaters

mgreenw · 2019-02-04T02:58:23Z

src/server/api/relationships/get-scene-candidates.js

-      to_char(profile.birthday, 'YYYY-MM-DD') AS birthday,
-      profile.bio
+      profile.user_id AS "userId",
+      ${profileSelectQuery('profile.user_id', { tableAlias: 'profile', buildJSON: true })} AS profile


This returns it in the format we discussed with profile, fields, and photoIds

mgreenw · 2019-02-04T02:58:34Z

src/server/api/relationships/judge.js

        last_swipe_timestamp = now()
-    `, [userId, candidateUserId, liked, liked]);
+    `, [userId, candidateUserId, liked]);


you can reuse params

mgreenw · 2019-02-04T02:58:56Z

src/server/api/status-codes.js

@@ -1,173 +1,221 @@
 // @flow


This whole file is changed and just has nicer abstraction over the 'code'. Much cleaner now

mgreenw · 2019-02-04T02:59:19Z

src/server/api/users/finalize-profile-setup.js

@@ -3,7 +3,7 @@
 import type { $Request } from 'express';

 const apiUtils = require('../utils');
-const utils = require('./utils');
+const { validateProfile, profileSelectQuery } = require('./utils');


Better to do imports like this

mgreenw · 2019-02-04T03:00:21Z

src/server/api/users/finalize-profile-setup.js

+      SET display_name = EXCLUDED.display_name
+    RETURNING
+      ${profileSelectQuery('$5')},
+      (xmax::text::int > 0) as existed


This is cool. 'existed' checks to see if it hit a conflict or not. If existed is true, then there was a conflict.

JacobJaffe

some comments during my 15 break in class haha

JacobJaffe · 2019-02-05T00:14:03Z

src/server/api/auth/send-verification-email.js

-    return apiUtils.status(400).json({
-      status: codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_FOUND,
-    });
+    return apiUtils.status(codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_FOUND).noData();
  }



nit: lots of reuse of .classyear and such, in general on these I think it would be a good idea to use destructure {classyear} = memberinfo in these

I totally agree...I think that's a separate PR. I started doing that for some util functions too and I'd like to do a full sweep

JacobJaffe · 2019-02-05T00:16:13Z

src/server/api/auth/verify.js

@@ -42,26 +42,20 @@ const verify = async (utln: string, code: number) => {

  // No email has been sent for this utln
  if (result.rowCount === 0) {


This is more of a syntac standardization idea, but I was thinking we could do
result.rowCount === 0 && return apiUtils...
for these kinda things? It would really clean up lots of code that's lots of conditional chaining

Is that cleaner in general? I guess I don't see that as more readable

JacobJaffe

More incremental comments

JacobJaffe · 2019-02-05T15:49:09Z

src/server/api/photos/delete-photo.js

@@ -92,29 +77,21 @@ const deletePhoto = async (photoId: number, userId: number, userHasProfile: bool

    // 4. Commit the transaction!
    await client.query('COMMIT');
+
+    return apiUtils.status(codes.DELETE_PHOTO__SUCCESS).data(


Can this happen in the finally? It's dangerous to put anything after the s3Delete, because theoretically any additional code could trigger the rollback, but s3 would have been changed.
Actually, for this reason I think step 3 and 4 should also be switched.

JacobJaffe · 2019-02-05T15:52:02Z

src/server/api/photos/delete-photo.js

 };

 const handler = [
  apiUtils.asyncHandler(async (req: $Request) => {
-    return deletePhoto(Number.parseInt(req.params.photoId, 10), req.user.id, req.user.hasProfile);
+    return deletePhoto(Number.parseInt(req.params.photoId, 10), req.user.id);


I think that the Number.parseInt should be abstracted to a photoUtils function if we pass the same constant base 10 to it every time.

Params.photoId comes in as a string. I am about to refactor this endpoint to be MUCH simpler using a new way of ordering photos using a trigger. This will remove the need for the photoId to come in as a number and instead allow it to come in as a string. Therefore, I'd prefer not to add a utility here as it will soon be removed

JacobJaffe · 2019-02-05T16:07:22Z

src/server/api/photos/get-photo.js

-      status: codes.GET_PHOTO__NOT_FOUND,
-    });
+    // $FlowFixMe I'm really not sure what's going on here...it's only HERE
+    return apiUtils.status(codes.GET_PHOTO__NOT_FOUND).noData();


Spent a little time debugging this. I think the root of this is the implicit types returned by functions; feels like it should work, perhaps related to:
facebook/flow#5294

I tested out a solution there by explicitly typing the return type (which is safe, as flow does make sure it's a valid return type).

const foo: { body: { status: string }, statusCode: number } = apiUtils .status(codes.GET_PHOTO__NOT_FOUND) .noData(); return foo;

Worth looking into later the deeper cause of this, but how about for now at least keeping this with a TODO comment so that the export here is well typed

JacobJaffe

see comments

JacobJaffe · 2019-02-08T02:04:20Z

src/server/api/photos/reorder-photos.js

-    return apiUtils.status(400).json({
-      status: codes.REORDER_PHOTOS__MISMATCHED_IDS,
-    });
+    return apiUtils.status(codes.REORDER_PHOTOS__MISMATCHED_IDS).noData();


can we return or log the mismatches?
edit: made a task

JacobJaffe · 2019-02-08T02:27:01Z

src/server/api/users/utils.js

@@ -75,8 +75,74 @@ function getFieldTemplates(definedFields: Array<[string, any]>) {
  };
 }

+const DefaultProfileOptions = {


Comments please

JacobJaffe · 2019-02-08T02:27:47Z

src/server/api/utils/async-handler.js

@@ -11,7 +11,8 @@ const asyncHandler = (fn : Middleware) => {
  return (req: $Request, res: $Response, next: NextFunction) => {
    Promise.resolve(fn(req, res, next))
      .then((response) => {
-        res.status(response.status).json(response.body);
+        // This is where the magic happens


please elaborate

mgreenw added 6 commits February 2, 2019 14:52

Update API response schema to always include 'status' and 'data'

a4df509

Lots of schema changes and db migration to remove splash phot

ad8cc48

Profile schema standardize select

9b3bcca

Remove console log

cf84701

Add ability to delete last photo

85e8682

Remove last photo deletion status code

fef8416

mgreenw requested a review from JacobJaffe February 3, 2019 23:27

Fix delete photo changes to tests

64d11c6

mgreenw commented Feb 4, 2019

View reviewed changes

JacobJaffe reviewed Feb 5, 2019

View reviewed changes

mgreenw added 3 commits February 5, 2019 08:29

Splash photo id nullable true on down migration

724393c

rollback last commit

cb8a20c

splash photo nullable on down migration

2a72a7c

mgreenw changed the title ~~BIG server clean & API response schema standardization~~ 1. BIG server clean & API response schema standardization Feb 5, 2019

JacobJaffe requested changes Feb 5, 2019

View reviewed changes

fix weird flowtype issue in get-photo

ca75126

mgreenw added 5 commits February 5, 2019 18:22

add flowtype error comment

c34fab6

Update status with return flowtypes

573404e

Add comment about flowfixme issue

b52a64b

Merge branch 'master' into max.server.api-return-schema-profiles

b8d0d9a

Update comments in api/users utils for select profile and settings

8065887

JacobJaffe requested changes Feb 8, 2019

View reviewed changes

update async-handler comments

e723a5a

JacobJaffe approved these changes Feb 8, 2019

View reviewed changes

JacobJaffe merged commit a9083ed into master Feb 8, 2019

mgreenw deleted the max.server.api-return-schema-profiles branch May 5, 2019 00:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1. BIG server clean & API response schema standardization #266

1. BIG server clean & API response schema standardization #266

mgreenw commented Feb 3, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

JacobJaffe Feb 5, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

mgreenw Feb 4, 2019

JacobJaffe left a comment

JacobJaffe Feb 5, 2019

mgreenw Feb 5, 2019

JacobJaffe Feb 5, 2019

mgreenw Feb 5, 2019

JacobJaffe left a comment

JacobJaffe Feb 5, 2019

JacobJaffe Feb 5, 2019

mgreenw Feb 5, 2019

JacobJaffe Feb 5, 2019

JacobJaffe left a comment

JacobJaffe Feb 8, 2019

JacobJaffe Feb 8, 2019

JacobJaffe Feb 8, 2019

		@@ -42,26 +42,20 @@ const verify = async (utln: string, code: number) => {

		// No email has been sent for this utln
		if (result.rowCount === 0) {

1. BIG server clean & API response schema standardization #266

1. BIG server clean & API response schema standardization #266

Conversation

mgreenw commented Feb 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobJaffe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobJaffe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobJaffe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment