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

Forward Verkada LPR Webhooks to ParkPow Visits #197

Merged
merged 7 commits into from
Apr 12, 2024

Conversation

danleyb2
Copy link
Collaborator

@danleyb2 danleyb2 commented Apr 6, 2024

https://trello.com/c/7FsGpiYS/1624-integration-verkada

  • Test receiving webhook payload and Image download
  • ParkPow Visit Creation
  • Update Readme.md

Copy link

github-actions bot commented Apr 6, 2024

Risk Level 3 - /home/runner/work/deep-license-plate-recognition/deep-license-plate-recognition/parkpow/webhook-verkada/main.py

The code changes introduce several new features and classes, which increases the complexity of the system. There are a few areas that could be improved for better error handling, code readability, and maintainability:

  1. Exception Handling: The except blocks catch general exceptions and log them, but it might be more useful to handle different types of exceptions differently. For example, network-related exceptions could trigger a retry, while others might not.

  2. Magic Numbers: The code uses the number 5 as a magic number in the while loop for retries. It would be better to define this as a constant at the top of the file to make it clear what it represents and to make it easier to change if needed.

  3. Hardcoded URLs: The URLs for the API endpoints are hardcoded. It would be better to define these as constants or configuration settings to make the code more flexible and easier to maintain.

  4. Error Handling in log_vehicle: The method log_vehicle raises a generic Exception with the message "Error logging vehicle". It would be more informative to include the actual error message from the response in the exception.

  5. Base64 Encoding: The method download_image returns the base64 encoded image, which is not immediately clear from the method name. Consider renaming the method to reflect this or changing the behavior to return the raw image data.

  6. Thread Safety: The WebhookQueue class uses a Queue, which is thread-safe, but it's not clear if other parts of the class are thread-safe. Ensure that all shared resources are properly synchronized.

  7. Logging Level: The logging level is set based on an environment variable, which is good for configurability. However, ensure that the default level is appropriate for production use if the environment variable is not set.

  8. Content-Type Validation: The do_POST method in RequestHandler checks the content type but writes b\"OK\" and continues processing even if the content type is not application/json. This could lead to unexpected behavior. It should return an error response instead.

Here's an example of how to define a constant for retries:

MAX_RETRIES = 5

# ... later in the code ...

while tries < MAX_RETRIES:
    # ... retry logic ...

And an example of improved exception handling in log_vehicle:

except requests.RequestException as e:
    error_message = f\"Error logging vehicle: {response.text}\"
    lgr.error(error_message, exc_info=e)
    raise Exception(error_message)

🔍🛠️🚦


Powered by Code Review GPT

@marcbelmont marcbelmont marked this pull request as draft April 8, 2024 08:16
@marcbelmont
Copy link
Collaborator

@danleyb2
FYI, I've converted the PR to "Draft pull request".

@danleyb2 danleyb2 marked this pull request as ready for review April 9, 2024 14:30
parkpow/webhook-verkada/main.py Outdated Show resolved Hide resolved
parkpow/webhook-verkada/main.py Show resolved Hide resolved
Copy link
Contributor

@dibonjohnseron dibonjohnseron left a comment

Choose a reason for hiding this comment

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

This looks good to me, @adolfoarmas recommendations are also nice to have.

Copy link
Collaborator

@adolfoarmas adolfoarmas left a comment

Choose a reason for hiding this comment

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

LGTM

@marcbelmont marcbelmont merged commit 698c773 into master Apr 12, 2024
1 check passed
@marcbelmont marcbelmont deleted the verkada-parkpow-visits branch April 12, 2024 06:55
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.

4 participants