Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive webhook notification support to the subscription tracking system, expanding notification options beyond email. Users can now configure multiple webhook services like Gotify, Teams, Discord, and Slack for receiving subscription expiry notifications.
- Adds webhook notifications as an alternative or complement to email notifications
- Implements database normalization for PostgreSQL connections using psycopg3
- Enhances the notification settings UI to support webhook configuration and management
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| config.py | Adds PostgreSQL URL normalization and psycopg3 configuration optimization |
| app/templates/notification_settings.html | Expands UI to include webhook management and notification method selection |
| app/templates/edit_webhook.html | New template for editing webhook configurations with dynamic help text |
| app/templates/add_webhook.html | New template for adding webhooks with service-specific setup examples |
| app/routes.py | Adds webhook CRUD routes and test webhook functionality |
| app/models.py | Defines new Webhook model and adds webhook_notifications to UserSettings |
| app/forms.py | Adds WebhookForm with validation for webhook configurations |
| app/email.py | Updates notification logic to support both email and webhook methods |
| app/init.py | Implements automatic database migration for webhook support |
| Dockerfile | Adds PostgreSQL client libraries for psycopg3 support |
Comments suppressed due to low confidence (1)
app/email.py:1
- [nitpick] Corrected the emoji from 🔔 to 📧 to be consistent with email notifications, or use a more appropriate notification emoji.
import smtplib
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if database_url.startswith('postgresql://'): | ||
| database_url = database_url.replace('postgresql://', 'postgresql+psycopg://', 1) | ||
| elif database_url.startswith('postgres://'): | ||
| database_url = database_url.replace('postgres://', 'postgresql+psycopg://', 1) |
There was a problem hiding this comment.
The conditional logic is redundant since both branches perform the same replacement pattern. This could be simplified to a single conditional that checks for either prefix.
| if database_url.startswith('postgresql://'): | |
| database_url = database_url.replace('postgresql://', 'postgresql+psycopg://', 1) | |
| elif database_url.startswith('postgres://'): | |
| database_url = database_url.replace('postgres://', 'postgresql+psycopg://', 1) | |
| for prefix in ('postgresql://', 'postgres://'): | |
| if database_url.startswith(prefix): | |
| database_url = database_url.replace(prefix, 'postgresql+psycopg://', 1) | |
| break |
| <li><i class="fas fa-check text-success me-2"></i>Only active subscriptions trigger notifications</li> | ||
| <li><i class="fas fa-check text-success me-2"></i>You won't receive duplicate notifications on the same day</li> | ||
| <li><i class="fas fa-paper-plane text-info me-2"></i>Use the "Send Test Email" button above to verify your email configuration</li> | ||
| <li><i class="fas fa-clock text-primary me-2"></i>Notifications are checked every hour at your preferred time</li> |
There was a problem hiding this comment.
The text suggests notifications are checked 'every hour at your preferred time' which is confusing. It should clarify that notifications are checked hourly but sent at the user's preferred time.
| <li><i class="fas fa-clock text-primary me-2"></i>Notifications are checked every hour at your preferred time</li> | |
| <li><i class="fas fa-clock text-primary me-2"></i>Notifications are checked every hour, but are only sent at your preferred time</li> |
| elif self.auth_username and self.auth_password: | ||
| import base64 | ||
| credentials = base64.b64encode(f'{self.auth_username}:{self.auth_password}'.encode()).decode() | ||
| headers['Authorization'] = f'Basic {credentials}' |
There was a problem hiding this comment.
The password is being used directly without any indication it was stored securely. If passwords are stored as plain text, this represents a security vulnerability as sensitive authentication data should be encrypted.
| url VARCHAR(500) NOT NULL, | ||
| auth_header VARCHAR(200), | ||
| auth_username VARCHAR(100), | ||
| auth_password VARCHAR(200), |
There was a problem hiding this comment.
The auth_password field is defined as VARCHAR without encryption, suggesting passwords will be stored as plain text. Authentication passwords should be hashed or encrypted for security.
1 participant
No description provided.