ChicoState / PantryNode

MIT License
2 stars 34 forks source link

Migrate Routes from mongodb to PostgreSQL #61

Open Jooms opened 1 year ago

Jooms commented 1 year ago

Disabled as part of #57, we need to re-enable all of the database models on the new Database technology.

briswells commented 1 year ago

Disabled as part of #57, we need to re-enable all of the database models on the new Database technology.

Updated name as the models have been implemented using relational DB methodology. Routes now need to be updated to use the new tech stack. specifically Sequelize.

briswells commented 1 year ago

Linking issue to branch #route-conversion

briswells commented 1 year ago

Looks like some of the routes I thought were updated are still having some issues. Here is the list of ones I know need some work but there may be more.

/home <- I commented out some code for expiration date that needs to be converted /donor <- Throws an error. /stock <- appears to be working but a 2nd set of eyes would be great. /add_stock <- appears to be working but a 2nd set of eyes would be great. /checkout <- Needs to be retrofitted. /checkout <- Needs to be retrofitted. /charts <- Needs to be retrofitted.

AbhinavReddy-Dev commented 1 year ago

I would like help out anyone new to backend development trying to work on the routes, please let me know.

MMurtey commented 1 year ago

I'll start with /donor.

briswells commented 1 year ago

I'll start with /donor.

I'm pretty sure one of the issues with that route is "undefined" no used in postgressql. Besides that the findall should be good. idk about the rest of the code though.

briswells commented 1 year ago

I was just looking at the /charts route and from what I can see all the math is handled by the front end super inefficiently. I added a comment to the React discussion thread asking if we can shift this to the backend during the rewrites, so we should probably hold off and rewriting it until we've come to a consensus.

hamsaraj7106 commented 1 year ago

I'll work on the commented part of the code in /home

MMurtey commented 1 year ago

/donor query is now pulling all "person"s with a donation transaction. I also added a simple view that will place the returned data in in a table (this view was missing, never created, or was meant to be shared with add_donor).

I do need to go back and ensure the query is only returning distinct values. Right now it'd return duplicate values if a user had multiple donation transactions.

UPDATE: /donors now returning only distinct rows. Tested by manually adding transactions to the database, varying number of transactions by user and transaction types. Results are ordered by primary key (by default). We'll let the front-end team decide what they want the returned order to be and update.

briswells commented 1 year ago

Looks like the front end team wants us to bring the math into the backend. I know at least the /charts has a bunch of math in the front end, but be sure to check on any others while converting them.

MMurtey commented 1 year ago

Looking through history, it doesn't appear the add_donor route has been converted - I'll do that one real quick. I think logic will be useful one way or another, but the the donor/add_donor/(delete_donor?) route thing seems a bit inefficient. It came up today, so I think we should discuss how the front-end teams wants to communicate with the back-end (I'm betting JSON in/out).

briswells commented 1 year ago

Looking through history, it doesn't appear the add_donor route has been converted - I'll do that one real quick. I think logic will be useful one way or another, but the the donor/add_donor/(delete_donor?) route thing seems a bit inefficient. It came up today, so I think we should discuss how the front-end teams wants to communicate with the back-end (I'm betting JSON in/out).

Not sure when it happened but add_donor was converted, but if you wanted to update the logic go for it. I don't think we'd want to completely delete a person out of the database but we could probably strip out their contact information etc.

We should definitely move to all JSON, luckily for most of the routes all we need to do is update a single line. Maybe while the front end team gets their stuff working as the new frontend container we can put together a Swagger with the current routes? After that's done we can convert what we have to JSON, that way the website still works with the current frontend?

MMurtey commented 1 year ago

I just looked at add_donor again- in any case, a donor is only defined by transaction, so it's just creating another user which presents an interesting, if unlikely, scenario: a donor needs to create an account to "work" the pantry. The user already exists, maybe with a dummy password, but without some sort of flag to inform the user signing up what has happened, the best they can do is shrug their shoulders and hit "Forgot Password."

briswells commented 1 year ago

I noticed that the other day, a long with another minor issue with how transactions are processed, I just haven't gotten around to sitting down and fixing it. I don't think it'll be to hard to sort out one way or another.

MMurtey commented 1 year ago

Since we're moving the API direction, should we close this issue and associated branch and open a new one? We discussed a bit in person: I think it'd be a good idea to keep the Swagger source in the repo so we can team up on it. We can come up with a way to publish the generated Swagger docs.