leofuturer / eDrops3

The 3rd generation of the eDrops web application.
MIT License
0 stars 0 forks source link

API endpoints #93

Closed Kenny477 closed 1 year ago

Kenny477 commented 1 year ago

Need a minor revamp to API endpoints

  1. Rename API endpoints to standardize API requests to CRUD actions For example, /users should be an API endpoint that handles multiple actions (POST, GET, etc.). And we want only one /users config entry in /api/lib/serverConfig.ts Additionally API endpoint names should be intuitive and all lowercase with words separated by dashes (e.g. reset-password not resetPassword)
  2. Server config API endpoint should be exported as a single object so we can do something like endpoints.user.signup and that object entry will either be a string if static or a function if some path parameter like id needs to be passed in.
  3. Backend API needs to be cleaned up. There are a lot of endpoints we don't use as of now and some endpoints can be implemented with simpler logic.
Kenny477 commented 1 year ago

Since this encapsulates #74 I will close that one in favor of this.

Kenny477 commented 1 year ago

Another thing we need to probably do is move all Shopify API calls to our backend so we don't have to pass API keys to the client. We currently expose all the key info essentially for our Shopify API (and Pusher API). An alternative would be to change our paradigm to SSR (server side rendering) so all rendering is done on the server. Not too sure and this is probably something for consideration for later.

Kenny477 commented 1 year ago

Goals:

  1. Revamp server config We want server config to correspond to HTTP endpoints, and not have separate configs for the same endpoint but for different HTTP verbs. And also for endpoints that have dynamic paths (e.g. need to input user ID), use a function, so we don't have to write string replaces when we actually use it. For example, not having
    export const userSignUp = `${ApiRootUrl}/users`;
    export const userBaseFind = `${ApiRootUrl}/users`;
    export const userBaseDeleteById = `${ApiRootUrl}/users/id`;
    export const updateUserBaseProfile = `${ApiRootUrl}/users/id`;

    and just having

    export const user = `${ApiRootUrl}/users`;
    export const userById = (id) => ${ApiRootUrl}/users/{id}`;
  2. Create API library. Add methods for all API calls that hide core logic (e.g. getting res.data, handling errors, etc.). Either do this by using the revamped server config, or we can skip the server config step altogether, remove it, and just use the API endpoint paths directly.

eg.

function getCustomer(customerId: string) {
 // ... make api requests here and return the appropriate data
}
  1. Remove unnecessary and/or redundant API endpoints. We should try to be minimalist and only use the raw HTTP verbs. Also remove any API endpoints corresponding to HTTP verbs that we will never use.
export const updateWorkerProfile = `${ApiRootUrl}/foundryWorkers/id`;
export const getWorkerId = `${ApiRootUrl}/foundryWorkers/getWorkerID`; // this API endpoint on the server can be removed (just use /foundryWorkers/id, with some query parameters)

@cmchoi220 This is a general overview of things we can work on. Start with combing through front-end code for all request calls then creating methods for common requests in the /client/src/api folder. We will also need to work on documenting all our wrapper functions, so you can also opt to not really work on code for now and just go through and document the types of requests we make and potential functions for them.

e.g.

Kenny477 commented 1 year ago

@cmchoi220 The new PR adds a lot of random stuff and changes a lot of code/structure so there might be difficulties merging. I also implemented something that partially addresses this issue by creating default methods for each resource so methods like customer.get(), customer.add() are automatically created. There needs to be some work on unconventional methods that need complex logic or validation but for now we are in a good spot. Please leave down below what branch you are working on (and push the branch to remote/GitHub) so we can come back to this as needed.

I would suggest you either look into the cart issues or even better work on adding testing to our application (this gives you one thing to focus on). See issue #115. First look into some libraries (e.g. Cypress which we already have some stuff for, or Vitest or Jest, etc.) to test webpages. Let me know when you think you've decided on a library you want to proceed with and we can confirm. Then work on automating tests for every page (e.g. going to /login and inputting the test credentials should log you in and send you to the home page). Basically just go through all of the common workflows. This will take a while but should teach you a new library and get you familiarized with every workflow and section of the app. The other benefit is that you likely won't have to worry too much about code changes others make as testing is decoupled from features/bug fixing for the most part unless you find that a test fails inherently due to a bug (which you should then open an issue for).

Let me know if you have questions or need some more kickstart help.

cmchoi220 commented 1 year ago

The branch is api-endpoint-revamp. I'll look into the testing.

Kenny477 commented 1 year ago

The branch is api-endpoint-revamp. I'll look into the testing.

@cmchoi220 Thanks. It's up to you if you want to try rebasing off stage (which is standard, but in this case there will likely be too many conflicts to easily resolve). For now let's not worry too much about this though. I'd like to get some automatic testing setup so we don't have to manually go through everything to make sure the app is working.

P.S. I was looking through some of your code changes on your branch. For the functions you don't need to construct a "new Promise" object each time. You can just return the result of the request call as the request call returns a Promise.

e.g. instead of

return new Promise((resolve, reject) => request(...).then(() => resolve()).catch(() => reject())

just do

return request(...) 

Some docs about Promises and also async/await: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise

Kenny477 commented 1 year ago

Closing for now (to be reassessed in the future)