Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructured the backend files. Wrote toggle-favorite #105

Merged
merged 21 commits into from Nov 17, 2021

Conversation

DDVD233
Copy link
Member

@DDVD233 DDVD233 commented Oct 25, 2021

I restructured backend files according to the official documentation here. I think we need to use multiple files in our backend, and it's better to start early. The doc is a bit long. Feel free to ask me if there's any part that is not clear.

I also moved the tests to the tests folder and divided tests based on their modules.

Notice that the URLs for friends APIs are now prefixed with /friends/. For example, /new-friends/ becomes /friends/new-friends/.
Similarly, the new one has the URL /favorites/toggle-favorite/.

Also, notice that the new API I wrote uses JSON as a means to pass parameters (instead of the query string). Documentations on how to use it can be found here. This is the preferred way as it automatically checks for any incompatible/missing parameters, so that we don't need to write these checks by hand.

@DDVD233 DDVD233 changed the title Restructured the backend files as per recommendation. Wrote tests Restructured the backend files. Wrote toggle-favorite Oct 25, 2021
@DDVD233 DDVD233 linked an issue Oct 25, 2021 that may be closed by this pull request
7 tasks
@DDVD233 DDVD233 added the review ready This issue is ready for review! label Oct 25, 2021
Copy link
Member

@RafaelPiloto10 RafaelPiloto10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very important work here! Thanks David! I love this new structure! Just a minor thing to keep things consistent with your changes

app_backend/app/routers/friends.py Show resolved Hide resolved
Copy link
Member

@RafaelPiloto10 RafaelPiloto10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks again for your work on this!

app_backend/app/routers/favorites.py Show resolved Hide resolved
Copy link
Contributor

@brendacano brendacano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it all looks good. I like the changes. Im just worried about the other PRs if it will affect them too much if this one gets merged first

app_backend/app/routers/favorites.py Show resolved Hide resolved
@DDVD233
Copy link
Member Author

DDVD233 commented Nov 8, 2021

Now rebased to current dev.

@RafaelPiloto10
Copy link
Member

holy guacamole, David, 93 files

Copy link
Member

@RafaelPiloto10 RafaelPiloto10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this PR is a good idea -- 93 files changed sounds like an error. Can you please split this PR up into the following:

  • backend restructure
  • toggle favorites backend
  • toggle favorites frontend

I know that is tedious, but it's going to be impossible to properly review the 93 files.
When you make these PR's think about how you want to chain them -- which PR depends on which and which should be merged first. Please note that in the PR comments

I am hesitant to approve to merge this given how many files changed in just 1 PR

@RafaelPiloto10 RafaelPiloto10 removed the review ready This issue is ready for review! label Nov 9, 2021
@DDVD233
Copy link
Member Author

DDVD233 commented Nov 11, 2021

It is probably because I merged the development branch into my branch. I was hoping rebase will reduce the commit amount, but apparently that didn't do it.

@RafaelPiloto10
Copy link
Member

It is probably because I merged the development branch into my branch. I was hoping rebase will reduce the commit amount, but apparently that didn't do it.

Try reverting the rebase and do git pull origin dev. This happened to Ore and doing that fixed it

…/toggle_favorite

� Conflicts:
�	app_backend/app/routers/friends.py
�	app_backend/tests/server_test.py
@DDVD233
Copy link
Member Author

DDVD233 commented Nov 11, 2021

OK I guess now it's better :)
Will fix the failing test later.

@brendacano
Copy link
Contributor

OK I guess now it's better :) Will fix the failing test later.

If you are ever worried that the merging or anything isn't good, you could always just start fresh in a new branch from dev and edit in everything you were doing. I know its a bit more work but at least you can be sure that its correct

@DDVD233 DDVD233 mentioned this pull request Nov 15, 2021
@DDVD233 DDVD233 removed a link to an issue Nov 15, 2021
7 tasks
@Foxworth22 Foxworth22 linked an issue Nov 15, 2021 that may be closed by this pull request
2 tasks
Copy link
Member

@RafaelPiloto10 RafaelPiloto10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top priority, let's merge this asap

Copy link
Member

@RafaelPiloto10 RafaelPiloto10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed the failing tests - lets fix the tests first and then merge this asap lol

@RafaelPiloto10 RafaelPiloto10 mentioned this pull request Nov 17, 2021
@DDVD233
Copy link
Member Author

DDVD233 commented Nov 17, 2021

I'm not sure why, but it looks like the register test creates users in the production database (when it shouldn't).

@DDVD233
Copy link
Member Author

DDVD233 commented Nov 17, 2021

I think I found the cause

@RafaelPiloto10
Copy link
Member

If you're not seeing the Dry run enabled, not updating print message then its not using the TestDatabase class in server_test.py

@DDVD233
Copy link
Member Author

DDVD233 commented Nov 17, 2021

I am removing TestDatabase class and using an environment variable to identify whether the server is running in test mode.



load_dotenv()
db = Database(os.getenv("DB_USERNAME"), os.getenv("DB_PASSWORD"), test_mode="pytest" in sys.modules)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever! Looks good to me!

@DDVD233 DDVD233 merged commit 1d9f845 into dev Nov 17, 2021
@DDVD233 DDVD233 deleted the feature/toggle_favorite branch December 4, 2021 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3pts] Refactor App Backend
4 participants