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

backend refactor #682

Merged
merged 3 commits into from
Jun 11, 2020
Merged

backend refactor #682

merged 3 commits into from
Jun 11, 2020

Conversation

jmensch1
Copy link
Contributor

@jmensch1 jmensch1 commented Jun 9, 2020

This is pretty much a complete refactor of the backend, plus some new features. Here are the highlights:

A clearer file structure in the /server directory

file-structure

At the top level we've got /api, which is the containerized python app. /jmeter contains performance tests, and /redis is the containerized redis service with configuration.

Within the /api folder, executables are separated from source code, and the source code itself is more modular and intuitive.

All environment settings in a single .env file.

All settings now live in one place -- the .env file -- which is also the only .gitignored file in the server. The docker-compose.yml file no longer contains any secrets, so it's in version control.

If we want to combine the frontend and backend environment variables, it will be easy to move them to the root of the project (although we'll also have to move the docker-compose.yml file there as well.)

Separation of route declarations from their implementation

# from /server/api/src/app.py 

routes = {
    '/': (
        ['GET'], R.index),

    '/status/api': (
        ['GET', 'HEAD'], R.status.api),

    '/status/sys': (
        ['GET', 'HEAD'], R.status.sys),

    '/status/db': (
        ['GET', 'HEAD'], R.status.db),

    '/servicerequest/<srnumber>': (
        ['GET'], R.request_detail),

    '/map/clusters': (
        ['POST'], R.map.clusters),

    '/map/heat': (
        ['POST'], R.map.heat),

    '/visualizations': (
        ['POST'], R.visualizations),

    '/comparison/frequency': (
        ['POST'], R.comparison.frequency),

    '/comparison/timetoclose': (
        ['POST'], R.comparison.timetoclose),

    '/comparison/counts': (
        ['POST'], R.comparison.counts),

    '/feedback': (
        ['POST'], R.feedback)}

Error handling (implementing #506) with Slack notifications

The server now returns 400 errors with helpful messages to the client if input is bad (e.g., required params are missing, or wrong datatypes are sent). Any 500 errors can be assumed to be a problem with the server.

If a slack webhook url is provided in the .env file, errors are sent to Slack with a stack trace and a description of the api call that generated the error.

A streamlined install process and a README

The onboarding process is much easier now, and there's a README that describes it. Basically it requires this (from the README):

  • install docker and docker compose on your machine
  • run chmod +x install.sh && ./install.sh from this directory
  • put a Socrata api token in your .env file
  • run docker-compose run api python bin/db_seed.py --years 2019,2020

Informative, colorful logging

logging

An AWS deploy workflow

There's a new workflow for deploying to AWS whenever there's a push to dev.

  • Up to date with dev branch
  • Branch name follows guidelines
  • All PR Status checks are successful
  • Peer reviewed and approved

Any questions? See the getting started guide

@jmensch1 jmensch1 requested a review from adamkendis June 9, 2020 18:49
@jmensch1 jmensch1 linked an issue Jun 9, 2020 that may be closed by this pull request
1 task
Copy link
Member

@adamkendis adamkendis left a comment

Choose a reason for hiding this comment

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

This was a killer PR. The new structure is awesome - small files, pretty clear separation of concerns. The error handler and logging are great. Thanks for remembering to update the frontend to match updated api endpoints. Just had a few small questions but nothing blocking this from being merged in.

@@ -0,0 +1,102 @@
## Getting Started
Copy link
Member

Choose a reason for hiding this comment

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

This is so good. I'll add front-end stuff to the README this week.

ERROR_CODES = Slack.ERROR_CODES


class ErrorHandler(EH):
Copy link
Member

Choose a reason for hiding this comment

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

Awesome.

@@ -0,0 +1,4 @@


def migrate():
Copy link
Member

Choose a reason for hiding this comment

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

Is this (and v_2) for future functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I stubbed out a migration concept, we'll see if it's actually useful. For now it doesn't do anything.

INSERT INTO requests SELECT * FROM stage
""")
log('\tInserted rows: {}'.format(inserted.rowcount))


def add_years(years, rows_per_year=math.inf, batch_size=BATCH_SIZE):
def add_years(years,
rows_per_year=-1,
Copy link
Member

Choose a reason for hiding this comment

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

What does -1 for rows_per_year do? No limit unless a positive int is passed for that arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah -1 just means get all the requests that Socrata has for the year

Comment on lines +29 to +39
# validate input
if not isinstance(fields, list) or len(fields) == 0:
raise Exception('fields must be provided')

if (
startDate is None or
endDate is None or
not isinstance(requestTypes, list) or
not isinstance(ncList, list)
):
raise Exception('invalid filters')
Copy link
Member

Choose a reason for hiding this comment

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

Nice, is this new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might have been checking that before, but it wasn't set up to return a 400 error. It probably just resulted in a 500 error if you didn't include startDate/endDate.

@adamkendis adamkendis merged commit e73ad6e into dev Jun 11, 2020
@adamkendis adamkendis deleted the BACK-refactor branch June 11, 2020 20:06
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.

[FEAT] Handling client errors
2 participants