website: updates to settings and events model #4033
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request enhance logging functionality across multiple classes and viewsets in the application. Modifications include the addition of logging to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #4033 +/- ##
========================================
Coverage 11.73% 11.73%
========================================
Files 114 114
Lines 15331 15331
Branches 319 319
========================================
Hits 1799 1799
Misses 13532 13532 |
|
website changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/website/apps/event/views.py (1)
53-57: Enhance list method logging for better observabilityConsider adding the category filter information to the list method's log message for better traceability.
def list(self, request, *args, **kwargs): logger.debug("Handling Event list request") response = super().list(request, *args, **kwargs) - logger.info(f"Listed Events, returned {len(response.data)} records") + category = self.request.query_params.get('category', 'all') + logger.info(f"Listed Events with category={category}, returned {len(response.data)} records") return responsesrc/website/core/settings.py (2)
Line range hint
24-47: Enhance environment variable helper functions with type hints and examplesThe helper functions would benefit from more detailed documentation and type hints.
-def parse_env_list(env_var: str, default: str = "") -> list: +def parse_env_list(env_var: str, default: str = "") -> list[str]: """ Convert a comma-separated list in an env var to a Python list. + + Example: + >>> os.environ['HOSTS'] = 'localhost,127.0.0.1' + >>> parse_env_list('HOSTS') + ['localhost', '127.0.0.1'] """ -def get_env_bool(env_var: str, default: bool = False) -> bool: +def get_env_bool(env_var: str, default: bool = False) -> bool: """ Convert an environment variable to boolean. + + Accepts: 'true', '1', 't', 'yes' (case-insensitive) + + Example: + >>> os.environ['DEBUG'] = 'yes' + >>> get_env_bool('DEBUG') + True """
155-162: Make database SSL requirement configurableConsider making the SSL requirement configurable via environment variable for development environments.
if DATABASE_URL: + ssl_require = get_env_bool('DATABASE_SSL_REQUIRE', default=True) DATABASES = { 'default': dj_database_url.parse( DATABASE_URL, conn_max_age=600, - ssl_require=True + ssl_require=ssl_require ) }src/website/apps/event/models.py (1)
Line range hint
101-121: Refactor Cloudinary operations into utility functionConsider extracting the Cloudinary file deletion logic into a utility function to reduce code duplication.
def delete_cloudinary_file(public_id: str, file_type: str) -> bool: """Delete a file from Cloudinary and log the operation. Args: public_id: The public ID of the file in Cloudinary file_type: The type of file (e.g., 'event_image', 'partner_logo') Returns: bool: True if deletion was successful, False otherwise """ try: destroy(public_id) logger.info(f"Deleted {file_type} from Cloudinary: {public_id}") return True except Exception as e: logger.error(f"Error deleting {file_type} from Cloudinary: {e}") return False # Usage in models: def delete(self, *args, **kwargs): logger.debug(f"Attempting to delete {self.__class__.__name__}: ID={self.pk}") if self.event_image: delete_cloudinary_file(self.event_image.public_id, 'event_image') if self.background_image: delete_cloudinary_file(self.background_image.public_id, 'background_image') super().delete(*args, **kwargs) logger.info(f"Deleted {self.__class__.__name__}: ID={self.pk}")Also applies to: 268-280, 318-329
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/website/apps/event/models.py(9 hunks)src/website/apps/event/views.py(2 hunks)src/website/core/settings.py(7 hunks)
| def list(self, request, *args, **kwargs): | ||
| logger.debug("Handling Inquiry list request") | ||
| response = super().list(request, *args, **kwargs) | ||
| logger.info(f"Listed Inquiries, returned {len(response.data)} records") | ||
| return response | ||
|
|
||
| def retrieve(self, request, *args, **kwargs): | ||
| logger.debug( | ||
| f"Handling Inquiry retrieve request, id={kwargs.get('id')}") | ||
| response = super().retrieve(request, *args, **kwargs) | ||
| logger.info(f"Retrieved Inquiry detail for ID={kwargs.get('id')}") | ||
| return response |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in viewset logging
The logging implementation is duplicated across all viewsets. Consider creating a LoggingMixin to encapsulate this common functionality.
class LoggingMixin:
def list(self, request, *args, **kwargs):
logger.debug(f"Handling {self.__class__.__name__} list request")
response = super().list(request, *args, **kwargs)
logger.info(f"Listed {self.__class__.__name__}s, returned {len(response.data)} records")
return response
def retrieve(self, request, *args, **kwargs):
logger.debug(f"Handling {self.__class__.__name__} retrieve request, id={kwargs.get('id')}")
response = super().retrieve(request, *args, **kwargs)
logger.info(f"Retrieved {self.__class__.__name__} detail for ID={kwargs.get('id')}")
return response
# Usage example:
class EventViewSet(LoggingMixin, viewsets.ReadOnlyModelViewSet):
...Also applies to: 91-102, 110-121, 129-141, 149-160
| def save(self, *args, **kwargs): | ||
| is_new = self.pk is None | ||
| super().save(*args, **kwargs) | ||
| if is_new: | ||
| logger.info(f"Created new Event: ID={self.pk}, Title={self.title}") | ||
| else: | ||
| logger.info(f"Updated Event: ID={self.pk}, Title={self.title}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction handling to model save methods
Consider wrapping the save operations in transactions to ensure data consistency with logging.
+from django.db import transaction
def save(self, *args, **kwargs):
is_new = self.pk is None
- super().save(*args, **kwargs)
- if is_new:
- logger.info(f"Created new {self.__class__.__name__}: ID={self.pk}, Title={self.title}")
- else:
- logger.info(f"Updated {self.__class__.__name__}: ID={self.pk}, Title={self.title}")
+ try:
+ with transaction.atomic():
+ super().save(*args, **kwargs)
+ if is_new:
+ logger.info(f"Created new {self.__class__.__name__}: ID={self.pk}, Title={self.title}")
+ else:
+ logger.info(f"Updated {self.__class__.__name__}: ID={self.pk}, Title={self.title}")
+ except Exception as e:
+ logger.error(f"Error saving {self.__class__.__name__}: {e}")
+ raiseAlso applies to: 145-154, 180-187, 216-225, 259-267, 309-317
Description
Summary by CodeRabbit
New Features
Bug Fixes
DEBUGandALLOWED_HOSTSsettings.Documentation