Enhance profile and pet management features (and add comprehensive test cases) #174
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ALso consistent nav toolbar
model - Introduce `latitude`, `longitude`, `delivery_radius` fields to Branch model - Migrate database to accommodate new fields - Update branch fixtures with default delivery radius - Extend PhysicalLocationSerializerMixin with `instructions` field - Refine field length constraints for address serializer fields This change enables tracking branch location coordinates and delivery range. It supports upcoming functionality for location-based branch discovery and delivery request mapping. The serializer updates align field lengths with typical use cases.
This change enhances the food request form by introducing a new optional textarea field for users to provide delivery instructions. Key benefits include: - Allowing users to specify additional details like buzz codes, suite numbers, etc. to facilitate smoother deliveries - Improving the overall user experience by accommodating diverse address formats and delivery scenarios - Potentially reducing delivery delays or failed attempts due to missing information The new field is rendered alongside the existing building type field, maintaining a clean and intuitive layout. Corresponding changes have been made to the data model and TypeScript type definitions to support this new attribute.
API - Enable CRUD operations on Branch model through Django admin interface - Implement BranchViewSet to expose Branch data through API endpoints - Add BranchSerializer to handle Branch model serialization/deserialization - Configure filtering, searching, and ordering for Branch API endpoints - Enhance admin UI with previews, filtering, and form customizations This change equips the application with comprehensive tooling to manage Branch data, allowing administrators to create, update, and maintain branch information through the admin interface. It also exposes this data to client applications via a RESTful API, enabling efficient retrieval, filtering, and manipulation of branch details. Additionally, the admin UI has been enhanced with previews, filters, and form customizations for an improved user experience when managing branches.
Signed-off-by: delano <[email protected]>
From nuxt start-time warnings Signed-off-by: delano <[email protected]>
This github issue explains the problem and the solution in high fidelity, particularly this comment from ayalon: nuxt/nuxt#27558 (comment) The comment from danielroe a few messages down indicates that the next release of nuxi-cli will have the fix (current 3.12.0). https://vitejs.dev/config/server-options#server-hmr
birth year range and general notes. In this commit, several improvements are made to the pet details section of the food request form: - The birth year field now dynamically generates a range of years from 1980 up to the current year, instead of a hardcoded list - A new "General Notes" field is added to allow users to provide additional information about their pets - The "Usual Brands" field is removed, as it was deemed unnecessary These changes aim to enhance the user experience and gather more relevant information about the pets, facilitating better service delivery. Considerations: - The birth year range may need adjusting if pets older than 1980 need to be accommodated - Character limits are imposed on the general notes field to prevent excessive input
- Utilize model utilities like AutoCreatedField and AutoLastModifiedField - Add "details" JSONField to store additional data for models - Add "is_removed" flag to soft-delete models instead of hard deletion - Replace hard-coded UUID default with UUIDField for Delivery model - Enhance model mixins with HasDetailsMixin for uniform details handling This change introduces several improvements to the data models: 1. Leverages Django model utilities like `AutoCreatedField` and `AutoLastModifiedField` to automatically track creation and modification timestamps. 2. Adds a `details` JSONField to multiple models, allowing storage of arbitrary metadata without schema changes. 3. Introduces an `is_removed` flag for soft-deletion of records instead of permanent removal. 4. Replaces hard-coded UUID defaults with Django's `UUIDField` for the `Delivery` model. 5. Enhances model mixins with `HasDetailsMixin` for consistent handling of details across models. These changes enhance data integrity, provide more flexibility for future requirements, and align with best practices for Django model design.
Further details: - Configures Gunicorn as the WSGI server for running the Django API - Sets up worker processes, worker class (gthread for I/O and CPU workloads), logging, and other server options - Includes hook functions for on_starting, on_reload, on_exit events - Implements custom when_ready and worker_abort handlers Rationale: - Gunicorn provides a lean, robust production-grade HTTP server for serving Django apps efficiently - Threading configuration balances performance for mixed I/O and CPU workloads typical of web apps - Hooks allow custom logic during server lifecycle events - Proper configuration is essential for optimized performance and resource utilization in production
- Default branch filter shows only operational branches - Branch selection updates Google Maps autocomplete bounds - Minor code cleanup and refactoring This change enhances the user experience when creating a new food request by: 1. Hiding non-operational branches from the branch selection dropdown by default. This prevents confusion and ensures users only select from available options. 2. Updating the Google Maps autocomplete bounds when a branch is selected from the dropdown. This focuses the address search area around the chosen branch, providing more relevant address suggestions. 3. Removing unused code and comments, improving code readability and maintainability. Overall, these changes streamline the food request creation process, guide users to relevant choices, and improve the quality of the codebase.
And store the place_id and Place object details.
For storing google PlaceResult object
- Allow entering address, phone number, and delivery branch - Prefill user details and sync changes to backend - Add link to view delivery area based on selected branch - Improve geocoding and autocomplete functionality This commit enhances the delivery information form to provide a better user experience and streamline the process of updating delivery-related information. Key changes include: - Enabling users to enter their address, phone number, and preferred delivery branch directly in the form - Prefilling user details (name, email) from the backend and syncing changes back to the user profile - Adding a link to view the delivery area covered by the selected branch - Improving geocoding and address autocomplete functionality for a smoother address entry process These improvements aim to simplify the process of managing delivery preferences, ensuring users can easily update their information and understand the delivery area covered by their chosen branch.
Which would nagivate to the next step in the formset, but not actually select the autocompleted value.
- Allow editing and saving changes to pet details - Add default example pet to provide guidance - Refactor submission handling to match API requirements - Enhance validation and UI feedback on form state The client pet details form has been updated to enable editing and persisting changes to a user's pet information. Key enhancements include: - Introducing a default example pet to guide users on expected input format - Refactoring form submission to match API payload requirements for pet details - Enhancing form validation with better error handling and success feedback - Improving UI with consistent styling and better guidance for input fields
For clarity and profit.
w/ a few fixes that were discovered during the process
Implement the following changes: - Convert `pet_type` values to lowercase on save in `Pet` model - Order `Profile` objects by creation date, oldest first - Add `get_default_profile()` method to `User` model - Allow reconciling pets for a profile via `reconcile_pets` action - Refine `PetsForm` component and client pet schema Rationale: - Ensure consistent `pet_type` values by converting to lowercase - Order profiles by creation date to determine the "main" profile easily - Provide a convenient method to retrieve the default/first profile - Enable synchronizing pet data between client and server efficiently - Improve form UI and validation for better user experience Benefits: - Data integrity and consistency - Intuitive profile ordering and retrieval - Streamlined pet data management - Enhanced form usability and validation
- Add `spay_or_neutered` boolean field to `Pet` model to track pet's spay/neuter status - Add `animal_details` JSONField to `Pet` model for storing additional animal-specific details - Order `Profile` instances by creation date - Order `DeliveryRegion` instances alphabetically by name This allows tracking important pet health information and additional animal details in a flexible JSON format. Ordering `Profile` and `DeliveryRegion` instances provides a more logical default sorting.
- Improve form layout and styling with larger inputs, tabs, and card-like sections
- Change pet type options to lowercase ("dog", "cat", etc.)
- Add creation and modification dates to PetInfo model
- Refactor spay/neuter field to boolean type
- Consolidate animal-specific details under "animal_details" object
- Enhance pets list with sorting, reordering, and easier adding/removal
- Disable save button until form is modified to prevent accidental submissions
- Handle form validation and show focused error on unsuccessful submit
…ellation - Allow adding pets in PetsForm only if user has none - Display pet details in FoodRequestForm without controls - Add cancel button in PetsForm to navigate back - Update clientPetsSchema to accept beforeText param - Add custom Tailwind color for cancel button styling - Apply 'afbcore-form' class to Vueform instances Key changes: - PetsForm: - Conditionally render pet add/remove controls based on user's existing pets - Add cancel button to navigate back - Style cancel button with custom Tailwind color - FoodRequestForm: Display existing user pets without add/remove controls - clientPetsSchema: Accept beforeText param to customize instructions - Tailwind config: Add custom 'custom-cancel' color - Vueform config: Apply 'afbcore-form' class to all Vueform instances The updates enhance user experience by tailoring pet management based on existing data, providing clearer instructions, and offering cancellation in relevant forms.
Long description: 1. The `FoodRequestForm.vue` component now stores the entire food request form data in the `details` field when submitting to the API. This change aims to facilitate debugging by providing a complete snapshot of the form data received by the backend. 2. In `food_request_serializer.py`, the `details` field has been added to the list of serialized fields for the `AbstractFoodRequestSerializer`. 3. The `contact_phone` and `contact_email` fields in the `FoodRequest` model have been made nullable by setting `null=True`. This change allows for cases where the contact information is not provided. 4. The `DeliveryContactSerializer` in `food_request_serializer.py` now includes additional fields for alternative contact information: `alt_contact_name`, `alt_contact_phone`, and `alt_contact_email`. By storing the complete form data, any issues or discrepancies between the frontend and backend can be more easily investigated and resolved, improving the overall debugging experience.
5 tasks
PR Reviewer Guide 🔍
|
![qodo-merge-pro[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| either all saved or all rolled back in case of an error. | ||
|
|
||
| """ | ||
| profile = self.get_object() |
There was a problem hiding this comment.
Suggestion: Consider validating that the profile belongs to the currently authenticated user in the reconcile_pets method to prevent users from updating pets associated with profiles that do not belong to them. This adds an extra layer of security and ensures that users can only modify pets within their own profiles. [security, importance: 7]
Suggested change
| profile = self.get_object() | |
| profile = self.get_object() | |
| if profile.user != request.user: | |
| return Response({"error": "You do not have permission to modify this profile's pets."}, status=status.HTTP_403_FORBIDDEN) |
There was a problem hiding this comment.
it's a great point. We take a declarative (and less defensive) approach by implementing UserFilterBaseViewSet which includes the current request user in the queryset itself. This way the view doesn't even know profiles owned by other users exist.
| @@ -167,7 +167,7 @@ | |||
| # https://docs.djangoproject.com/en/4.2/ref/csrf/#how-it-works | |||
| # CSRF_COOKIE_DOMAIN = "" | |||
|
|
|||
| TOKEN_EXPIRED_AFTER_WEEKS = 2 | |||
| TOKEN_EXPIRED_AFTER_WEEKS = 1 | |||
There was a problem hiding this comment.
Suggestion: For the TOKEN_EXPIRED_AFTER_WEEKS setting, consider documenting the reason for changing the token expiration from 2 weeks to 1 week. This helps maintainers understand the security considerations or business requirements driving this change. [enhancement, importance: 7]
Suggested change
| TOKEN_EXPIRED_AFTER_WEEKS = 1 | |
| # Reducing token expiration to 1 week for enhanced security posture and to ensure users re-authenticate more frequently. | |
| TOKEN_EXPIRED_AFTER_WEEKS = 1 |
Comment on lines
+136
to
+137
| serializer = PetSerializer(pet, data=pet_data, partial=True) | ||
| serializer.is_valid(raise_exception=True) |
There was a problem hiding this comment.
Suggestion: In the reconcile_pets method, consider handling the case where the pet data is invalid more gracefully. Instead of immediately returning a 400 error when encountering the first invalid pet, collect all validation errors and return them together. This approach provides the client with comprehensive feedback on all errors in their request, improving the API's usability. [enhancement, importance: 7]
Suggested change
| serializer = PetSerializer(pet, data=pet_data, partial=True) | |
| serializer.is_valid(raise_exception=True) | |
| if not serializer.is_valid(): | |
| errors.append(serializer.errors) | |
| else: | |
| serializer.save() | |
| # After processing all pets, check if there were any errors | |
| if errors: | |
| return Response({"errors": errors}, status=status.HTTP_400_BAD_REQUEST) |
| }, | ||
| "test": { | ||
| "ENGINE": os.getenv("DB_ENGINE"), | ||
| "NAME": "test_" + (os.getenv("DB_NAME") or ""), |
There was a problem hiding this comment.
Suggestion: When defining the test database configuration, ensure that the database name is not empty to avoid potential conflicts or errors during testing. This can be achieved by providing a default name for the test database if DB_NAME is not set in the environment variables. [possible issue, importance: 7]
Suggested change
| "NAME": "test_" + (os.getenv("DB_NAME") or ""), | |
| "NAME": "test_" + (os.getenv("DB_NAME") or "default_test_db"), |
Comment on lines
+28
to
+29
| self.token1 = Token.objects.create(user=self.user1) | ||
| self.token2 = Token.objects.create(user=self.user2) |
There was a problem hiding this comment.
Suggestion: In the setUp method, consider adding a teardown method (tearDown) to clean up any resources or configurations set during the tests. This is especially important for tests that modify global settings or create records in a test database. It ensures test isolation and prevents side effects between tests. [best practice, importance: 7]
Suggested change
| self.token1 = Token.objects.create(user=self.user1) | |
| self.token2 = Token.objects.create(user=self.user2) | |
| def tearDown(self): | |
| self.user1.delete() | |
| self.user2.delete() | |
| super().tearDown() |
| @@ -60,12 +61,17 @@ class FoodRequest(BaseAbstractModel): | |||
| default=BUILDING_TYPE_CHOICES.NOT_SPECIFIED, | |||
| ) | |||
| address_details = models.JSONField(default=dict) | |||
| ext_address_details = models.JSONField(default=dict) | |||
There was a problem hiding this comment.
Suggestion: Consider adding a validation for ext_address_details JSONField to ensure it contains valid JSON structure according to the expected schema. This can prevent saving malformed JSON data. [enhancement, importance: 7]
Suggested change
| ext_address_details = models.JSONField(default=dict) | |
| from django.core.validators import validate_json_schema | |
| ext_address_details_schema = { | |
| "type": "object", | |
| "properties": { | |
| "floor": {"type": "string"}, | |
| "door_code": {"type": "string"} | |
| }, | |
| "additionalProperties": False | |
| } | |
| ext_address_details = models.JSONField(default=dict, validators=[validate_json_schema(ext_address_details_schema)]) |
|
|
||
|
|
||
| class BranchViewSet(viewsets.ModelViewSet): | ||
| queryset = Branch.objects.all() |
There was a problem hiding this comment.
Suggestion: To improve security and ensure that sensitive branches are not inadvertently exposed, consider explicitly filtering out hidden branches in the default queryset unless the user has specific permissions. [security, importance: 7]
Suggested change
| queryset = Branch.objects.all() | |
| from rest_framework.permissions import IsAdminUser | |
| def get_queryset(self): | |
| if self.request.user.is_staff or self.request.user.has_perm('afbcore.view_hidden_branch'): | |
| return Branch.objects.all() | |
| return Branch.objects.filter(hidden=False) |
Comment on lines
+66
to
+70
| delivery_radius = models.IntegerField( | ||
| default=1, | ||
| help_text="Delivery radius in kilometers", | ||
| null=True, | ||
| blank=True, |
There was a problem hiding this comment.
Suggestion: Consider adding a model validation check to ensure that delivery_radius is positive when specified. This can prevent logical errors related to delivery operations. [enhancement, importance: 7]
Suggested change
| delivery_radius = models.IntegerField( | |
| default=1, | |
| help_text="Delivery radius in kilometers", | |
| null=True, | |
| blank=True, | |
| from django.core.validators import MinValueValidator | |
| delivery_radius = models.IntegerField( | |
| default=1, | |
| help_text="Delivery radius in kilometers", | |
| null=True, | |
| blank=True, | |
| validators=[MinValueValidator(1)] | |
| ) |
| # delivery_regions = serializers.ListField(required=False) | ||
| # delivery_regions = serializers.StringRelatedField(many=True) | ||
|
|
||
| pets = PetSerializer(many=True, read_only=True, required=False) |
There was a problem hiding this comment.
Suggestion: To ensure data integrity and consistency, consider adding validation to the pets field to check if the pets belong to the user associated with the profile being serialized. [enhancement, importance: 7]
Suggested change
| pets = PetSerializer(many=True, read_only=True, required=False) | |
| pets = PetSerializer(many=True, read_only=True, required=False) | |
| def validate_pets(self, value): | |
| user_pets = self.context['request'].user.pets.all() | |
| if not all(pet in user_pets for pet in value): | |
| raise serializers.ValidationError("One or more pets do not belong to this user.") | |
| return value |
Comment on lines
+16
to
+22
| field=models.EmailField(blank=True, max_length=254, null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name="foodrequest", | ||
| name="contact_phone", | ||
| field=phonenumber_field.modelfields.PhoneNumberField( | ||
| blank=True, max_length=128, null=True, region="CA" |
There was a problem hiding this comment.
Suggestion: Consider adding a database index to the contact_email and contact_phone fields to improve query performance, especially if these fields will be frequently searched or filtered upon. This can be done by setting db_index=True in the field arguments. [performance, importance: 7]
Suggested change
| field=models.EmailField(blank=True, max_length=254, null=True), | |
| ), | |
| migrations.AlterField( | |
| model_name="foodrequest", | |
| name="contact_phone", | |
| field=phonenumber_field.modelfields.PhoneNumberField( | |
| blank=True, max_length=128, null=True, region="CA" | |
| field=models.EmailField(blank=True, max_length=254, null=True, db_index=True), | |
| field=phonenumber_field.modelfields.PhoneNumberField(blank=True, max_length=128, null=True, region="CA", db_index=True) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The changes introduce comprehensive test cases for various viewsets, serializers, and models, enhancing the robustness of the application. Key enhancements include:
reconcile_pets.branch,details, andis_removedfields.