Refactor store and add RoomState #34
Open
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
…modation-api into 21-room-creation-frontend
21 tasks
WalkthroughThe updates introduce enhancements across the client and backend, focusing on property and room management. Notable changes include adding image configuration in Next.js, improving room display and addition functionalities, introducing components for image input with preview, room details form, and delete confirmation dialog. Backend adjustments refine serialization and view handling, streamlining data representation and operations like creation and updating. These changes aim to enrich user experience and data interaction within the application. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (9)
- client/next.config.js (1 hunks)
- client/src/app/(dashboard)/profile/[id]/landlords/properties/[propertyId]/page.tsx (1 hunks)
- client/src/app/(dashboard)/profile/[id]/landlords/properties/[propertyId]/rooms-add/page.tsx (1 hunks)
- client/src/components/shared/imagesInputWithPreview.tsx (1 hunks)
- client/src/components/shared/properties/roomCard.tsx (1 hunks)
- client/src/components/shared/properties/roomForm.tsx (1 hunks)
- client/src/components/ui/textarea.tsx (1 hunks)
- client/src/store/store.ts (2 hunks)
- client/src/types/index.ts (4 hunks)
Check Runs (2)
Analyze (javascript-typescript) completed (1)
Check Formating completed (2)
Additional comments: 10
client/src/app/(dashboard)/profile/[id]/landlords/properties/[propertyId]/rooms-add/page.tsx (1)
- 1-15: The implementation of the
AddRoomPagecomponent looks good. It correctly imports and utilizes theRoomFormcomponent, passing thepropertyIdand action type as props. Ensure that theRoomFormcomponent is adequately designed to handle the 'Add' action and that thepropertyIdis utilized as intended within the form.client/next.config.js (1)
- 8-17: The addition of
remotePatternsin the Next.js configuration for handling images from specific domains is correctly implemented. Ensure that the protocol and hostname (httpandlocalhost) are appropriate for your application's environments. For production, consider usinghttpsto enhance security.client/src/components/ui/textarea.tsx (1)
- 1-24: The implementation of the
Textareacomponent is well-structured and makes good use of React'sforwardRefand props spreading. Ensure that thecnutility function used for className concatenation is thoroughly tested and reliable for various use cases.client/src/components/shared/imagesInputWithPreview.tsx (1)
- 1-50: The
ImageFileInputcomponent is well-implemented, supporting multiple file selections and providing a clear button for input reset. Ensure that the component behaves as expected in edge cases, such as when a user selects files but then cancels the operation.client/src/app/(dashboard)/profile/[id]/landlords/properties/[propertyId]/page.tsx (1)
- 38-49: The modifications to the
PropertyDetailscomponent enhance the room management section by providing a structured and interactive way to add new rooms. Ensure that the link's href is correctly formed and that theButtoncomponent is accessible to all users.client/src/components/shared/properties/roomCard.tsx (1)
- 14-14: The update to use curly braces for the
srcattribute value assignment in theImagecomponent is a good practice in JSX, enhancing code readability and consistency.client/src/types/index.ts (1)
- 153-170: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [90-170]
The additions and modifications to type definitions, including the
RoomTypeenum, changes to theRoomtype, and the introduction ofRoomFormPropsandRoomState, enhance type safety and clarity for room management features. Ensure that these new types are consistently used throughout the application and cover all necessary use cases.client/src/store/store.ts (1)
- 92-125: The addition of
RoomStateusing Zustand for room state management is well-implemented, providing a structured way to manage room-related data globally. Ensure that the initial state and state management functions align with the application's requirements and handle edge cases appropriately.client/src/components/shared/properties/roomForm.tsx (2)
- 1-1: Using
"use client";is a good practice for Next.js 13+ components that are intended to run only on the client side. This ensures that the component will not be accidentally rendered on the server, improving performance and avoiding potential issues.- 2-21: The imports are well-organized and follow a logical structure, starting with React-related imports, followed by component imports, type imports, and finally utility and external library imports. This organization enhances readability and maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- client/src/components/shared/deleteConfirmition.tsx (1 hunks)
- client/src/components/shared/properties/roomCard.tsx (1 hunks)
- client/src/components/shared/properties/roomForm.tsx (1 hunks)
- client/src/components/ui/textarea.tsx (1 hunks)
- client/src/types/index.ts (5 hunks)
Files skipped from review as they are similar to previous changes (4)
- client/src/components/shared/properties/roomCard.tsx
- client/src/components/shared/properties/roomForm.tsx
- client/src/components/ui/textarea.tsx
- client/src/types/index.ts
Additional comments: 4
client/src/components/shared/deleteConfirmition.tsx (4)
- 1-1: Using the
"use client";directive is a good practice for ensuring this module runs only in the client-side environment. This can help avoid unintended server-side rendering issues.- 18-26: The component's props are well-defined, ensuring clarity in what is expected to be passed to
DeleteConfirmition. However, consider adding a brief comment describing the component's purpose and its props for better maintainability and readability.- 43-61: The structure of the
AlertDialogcomponent is clear and follows a logical flow. However, consider adding accessibility attributes such asaria-labeloraria-labelledbyto improve accessibility for screen reader users.- 64-64: Correctly exporting the
DeleteConfirmitioncomponent as the default export. This follows common practice for React components.
Comment on lines
27
to
40
| async function action() { | ||
| try { | ||
| await axios.delete(actionUrl, { | ||
| withCredentials: true, | ||
| }); | ||
| toast.success("Successfully deleted"); | ||
| window.location.reload(); | ||
| } catch (error) { | ||
| alert(JSON.stringify(error)); | ||
| const message = | ||
| (error as AxiosErrorWithDetails)?.response?.data?.detail ?? | ||
| "Cannot delete, something went wrong"; | ||
| toast(`Failed to delete: ${message}`); | ||
| } |
There was a problem hiding this comment.
The action function correctly handles the delete operation and provides feedback to the user. However, there are a few areas for improvement:
- Directly using
window.location.reload()for updating the UI after a successful delete operation can be inefficient and may lead to a poor user experience. Consider using state management or context to reflect changes without reloading the page. - The error handling could be improved by using
toast.errorinstead oftoastfor displaying error messages, to make it consistent with the success message which usestoast.success. - Using
alert(JSON.stringify(error));for error debugging is not user-friendly. It's better to remove or replace it with a more subtle logging mechanism.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- client/src/components/properties/updateRoomDialog.tsx (1 hunks)
- client/src/components/shared/properties/roomCard.tsx (1 hunks)
- client/src/components/shared/properties/roomForm.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- client/src/components/shared/properties/roomCard.tsx
- client/src/components/shared/properties/roomForm.tsx
Additional comments: 1
client/src/components/properties/updateRoomDialog.tsx (1)
- 1-3: The import statements are clear and follow best practices by separating external and internal imports. The usage of type import for
Roomis also a good practice for performance.
Comment on lines
+5
to
+28
| const UpdateRoomDialog = ({ | ||
| trigger, | ||
| roomData, | ||
| property, | ||
| }: { | ||
| trigger: React.ReactNode; | ||
| roomData: Room; | ||
| property: string; | ||
| }) => { | ||
| return ( | ||
| <Dialog> | ||
| <DialogTrigger asChild>{trigger}</DialogTrigger> | ||
| <DialogContent> | ||
| <RoomForm | ||
| action="Update" | ||
| roomData={roomData} | ||
| property={property.split("/").at(-1) ?? ""} | ||
| /> | ||
| </DialogContent> | ||
| </Dialog> | ||
| ); | ||
| }; | ||
|
|
||
| export default UpdateRoomDialog; |
There was a problem hiding this comment.
The UpdateRoomDialog component is well-structured and utilizes destructuring for props, which enhances readability. However, there are a few areas for improvement:
- Prop Types: It's a good practice to define prop types outside of the component function signature for better readability and maintainability. Consider using
interfaceortypefor prop definitions. - Property Splitting: The operation
property.split("/").at(-1) ?? ""directly within the JSX can be considered a bit complex for inline expressions. It might be more readable to perform this operation outside the return statement and store the result in a variable.
Consider defining prop types using interface or type and refactor the property splitting logic for better readability.
+ interface UpdateRoomDialogProps {
+ trigger: React.ReactNode;
+ roomData: Room;
+ property: string;
+ }
+
- const UpdateRoomDialog = ({
+ const UpdateRoomDialog: React.FC<UpdateRoomDialogProps> = ({
trigger,
roomData,
property,
}) => {
+ const propertyName = property.split("/").at(-1) ?? "";
return (
<Dialog>
<DialogTrigger asChild>{trigger}</DialogTrigger>
<DialogContent>
<RoomForm
action="Update"
roomData={roomData}
- property={property.split("/").at(-1) ?? ""}
+ property={propertyName}
/>
</DialogContent>
</Dialog>
);
};Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| const UpdateRoomDialog = ({ | |
| trigger, | |
| roomData, | |
| property, | |
| }: { | |
| trigger: React.ReactNode; | |
| roomData: Room; | |
| property: string; | |
| }) => { | |
| return ( | |
| <Dialog> | |
| <DialogTrigger asChild>{trigger}</DialogTrigger> | |
| <DialogContent> | |
| <RoomForm | |
| action="Update" | |
| roomData={roomData} | |
| property={property.split("/").at(-1) ?? ""} | |
| /> | |
| </DialogContent> | |
| </Dialog> | |
| ); | |
| }; | |
| export default UpdateRoomDialog; | |
| interface UpdateRoomDialogProps { | |
| trigger: React.ReactNode; | |
| roomData: Room; | |
| property: string; | |
| } | |
| const UpdateRoomDialog: React.FC<UpdateRoomDialogProps> = ({ | |
| trigger, | |
| roomData, | |
| property, | |
| }) => { | |
| const propertyName = property.split("/").at(-1) ?? ""; | |
| return ( | |
| <Dialog> | |
| <DialogTrigger asChild>{trigger}</DialogTrigger> | |
| <DialogContent> | |
| <RoomForm | |
| action="Update" | |
| roomData={roomData} | |
| property={propertyName} | |
| /> | |
| </DialogContent> | |
| </Dialog> | |
| ); | |
| }; | |
| export default UpdateRoomDialog; |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/src/components/shared/deleteConfirmition.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- client/src/components/shared/deleteConfirmition.tsx
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- client/src/store/store.ts (2 hunks)
- client/src/types/index.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- client/src/store/store.ts
- client/src/types/index.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| else: | ||
| error_detail = str(e) | ||
| return Response( | ||
| {"detail": error_detail}, status=status.HTTP_400_BAD_REQUEST |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
| ) | ||
| except ValidationError as e: | ||
| return Response( | ||
| {"detail": str(e).strip("['").strip("']")}, |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- backend/core/serializers.py (11 hunks)
- backend/core/views.py (11 hunks)
Check Runs (1)
CodeQL success (2)
- backend/core/views.py: 131-131: Stack trace information flows to this location and may be exposed to an external user.
- backend/core/views.py: 689-689: Stack trace information flows to this location and may be exposed to an external user.
Additional comments: 12
backend/core/serializers.py (8)
- 6-6: The import of
UserSerializerfromaccounts.serializersis a good addition, ensuring that user-related data can be serialized within other serializers where needed.- 44-44: Adding
depth = 1to theMetaclass of serializers is beneficial for fetching related objects up to one level deep automatically. This enhances the API response with more detailed information without additional queries, improving the user experience.- 58-61: The
to_representationmethod inStudentSerializeris well-implemented. It customizes the representation of theStudentmodel to include serialized user data, which is crucial for front-end applications to display comprehensive student information.- 112-115: Similarly, the
to_representationmethod inLandlordSerializeris correctly implemented, ensuring that user data is included in the serialized landlord data. This consistency across serializers is commendable.- 163-166: The addition of the
to_representationmethod inBookingSerializerto include owner data is a thoughtful enhancement. It ensures that booking information is accompanied by detailed owner information, which is essential for managing bookings effectively.- 236-241: The
to_representationmethod inRoomSerializeris correctly implemented to include property data. This is particularly useful for scenarios where room details need to be displayed along with property information, enhancing data richness.- 272-282: The
to_representationmethod inPropertySerializeris comprehensive, including owner, reviews, amenities, and city data. This method significantly enriches the property data presented to the end-user, facilitating a better understanding of property details.- 315-318: The
to_representationmethod inReviewSerializerincludes owner data, which is crucial for displaying reviews with information about the reviewer. This addition enhances the transparency and credibility of reviews.backend/core/views.py (4)
- 7-8: The addition of imports for
DjangoFilterBackendandSearchFilteris a positive change, indicating an enhancement in filtering capabilities for the API endpoints. These tools provide powerful filtering options, improving the API's usability.- 37-37: Including
RoomImagein the list of models is necessary for the changes made in theRoomViewSet, specifically for handling image data associated with rooms. This ensures that all relevant models are accessible within the viewsets.- 213-214: The update to
filter_backendsandfilterset_fieldsinPropertyViewSetenhances the filtering capabilities, allowing users to filter properties by city name, owner ID, and amenities name. This is a significant improvement in the API's functionality.- 586-593: Similarly, the update to
filter_backendsandfilterset_fieldsinRoomViewSetimproves the API's usability by enabling more granular filtering options for rooms. This allows users to find rooms that match their specific criteria more efficiently.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (1)
- backend/core/views.py (11 hunks)
Check Runs (1)
CodeQL success (2)
- backend/core/views.py: 131-131: Stack trace information flows to this location and may be exposed to an external user.
- backend/core/views.py: 690-690: Stack trace information flows to this location and may be exposed to an external user.
Additional comments: 5
backend/core/views.py (5)
- 7-8: The addition of
DjangoFilterBackendandSearchFilterimports is a good practice for enhancing filtering capabilities in viewsets. Ensure that these filters are properly configured and tested for each applicable viewset.- 37-37: Adding
RoomImageto the list of models is crucial for supporting image management in rooms. Ensure that theRoomImagemodel is properly integrated with theRoommodel and that image handling (upload, storage, retrieval) is secure and efficient.- 213-214: The configuration of
filter_backendsandfilterset_fieldsinPropertyViewSetenhances the filtering capabilities, making it easier for users to find properties based on specific criteria. Ensure that these filters are thoroughly tested for accuracy and performance.- 404-409: The
get_querysetmethod inBookingViewSetis a good addition for filtering bookings based on the user's role (student or landlord). This enhances the user experience by showing relevant bookings. Ensure that this method is properly tested, especially for edge cases where a user might have both roles.- 586-601: The configuration of
filter_backendsandfilterset_fieldsinRoomViewSetis a positive change for improving room search functionality. It's important to ensure that these filters work as expected and do not impact the performance negatively.
Comment on lines
+52
to
+86
| def create(self, request, *args, **kwargs): | ||
| if request.user.is_student: | ||
| return Response( | ||
| {"detail": "You can not create a student account."}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| if request.user.is_landlord: | ||
| return Response( | ||
| {"detail": "You can not create a student account as a landlord."}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| try: | ||
| with transaction.atomic(): | ||
| institution = Institution.objects.get( # pylint: disable=no-member | ||
| pk=request.data.pop("institution") | ||
| ) | ||
| student = Student.objects.create( # pylint: disable=no-member | ||
| user=request.user, institution=institution, **request.data | ||
| ) | ||
| request.user.is_student = True | ||
| request.user.save() | ||
| return Response( | ||
| StudentSerializer(student, context={"request": request}).data, | ||
| status=status.HTTP_201_CREATED, | ||
| ) | ||
| except ValidationError: | ||
| return Response( | ||
| {"detail": "Institution does not exist."}, | ||
| status=status.HTTP_404_NOT_FOUND, | ||
| ) | ||
| except Exception as e: # pylint: disable=broad-except | ||
| return Response( | ||
| e.args[0].detail if hasattr(e.args[0], "detail") else e.args[0], | ||
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| ) |
There was a problem hiding this comment.
The create method in StudentViewSet has been significantly modified to include checks for user roles and to handle student creation with atomic transactions. This approach ensures data integrity and provides clear responses for unauthorized actions. However, consider handling the broad exception more gracefully to avoid exposing stack trace information to the end user.
- e.args[0].detail if hasattr(e.args[0], "detail") else e.args[0],
+ {"detail": "An error occurred during student creation."}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| def create(self, request, *args, **kwargs): | |
| if request.user.is_student: | |
| return Response( | |
| {"detail": "You can not create a student account."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| if request.user.is_landlord: | |
| return Response( | |
| {"detail": "You can not create a student account as a landlord."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| try: | |
| with transaction.atomic(): | |
| institution = Institution.objects.get( # pylint: disable=no-member | |
| pk=request.data.pop("institution") | |
| ) | |
| student = Student.objects.create( # pylint: disable=no-member | |
| user=request.user, institution=institution, **request.data | |
| ) | |
| request.user.is_student = True | |
| request.user.save() | |
| return Response( | |
| StudentSerializer(student, context={"request": request}).data, | |
| status=status.HTTP_201_CREATED, | |
| ) | |
| except ValidationError: | |
| return Response( | |
| {"detail": "Institution does not exist."}, | |
| status=status.HTTP_404_NOT_FOUND, | |
| ) | |
| except Exception as e: # pylint: disable=broad-except | |
| return Response( | |
| e.args[0].detail if hasattr(e.args[0], "detail") else e.args[0], | |
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| ) | |
| def create(self, request, *args, **kwargs): | |
| if request.user.is_student: | |
| return Response( | |
| {"detail": "You can not create a student account."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| if request.user.is_landlord: | |
| return Response( | |
| {"detail": "You can not create a student account as a landlord."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| try: | |
| with transaction.atomic(): | |
| institution = Institution.objects.get( # pylint: disable=no-member | |
| pk=request.data.pop("institution") | |
| ) | |
| student = Student.objects.create( # pylint: disable=no-member | |
| user=request.user, institution=institution, **request.data | |
| ) | |
| request.user.is_student = True | |
| request.user.save() | |
| return Response( | |
| StudentSerializer(student, context={"request": request}).data, | |
| status=status.HTTP_201_CREATED, | |
| ) | |
| except ValidationError: | |
| return Response( | |
| {"detail": "Institution does not exist."}, | |
| status=status.HTTP_404_NOT_FOUND, | |
| ) | |
| except Exception as e: # pylint: disable=broad-except | |
| return Response( | |
| {"detail": "An error occurred during student creation."} | |
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| ) |
Comment on lines
+98
to
+132
| def create(self, request, *args, **kwargs): | ||
| if request.user.is_student: | ||
| return Response( | ||
| {"detail": "You can not create a landlord account as a student."}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| if request.user.is_landlord: | ||
| return Response( | ||
| {"detail": "You can not create another landlord account."}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| try: | ||
| with transaction.atomic(): | ||
| city = City.objects.get( # pylint: disable=no-member | ||
| pk=request.data.pop("city") | ||
| ) | ||
| landlord = Landlord.objects.create( # pylint: disable=no-member | ||
| user=request.user, city=city, **request.data | ||
| ) | ||
| request.user.is_landlord = True | ||
| request.user.save() | ||
| return Response( | ||
| LandlordSerializer(landlord, context={"request": request}).data, | ||
| status=status.HTTP_201_CREATED, | ||
| ) | ||
| except ValidationError as e: | ||
| if isinstance(e.error_list, list) and e.error_list: | ||
| error_detail = { | ||
| field: errors[0] for field, errors in e.error_list[0].params.items() | ||
| } | ||
| else: | ||
| error_detail = str(e) | ||
| return Response( | ||
| {"detail": error_detail}, status=status.HTTP_400_BAD_REQUEST | ||
| ) |
There was a problem hiding this comment.
The create method in LandlordViewSet includes role checks and atomic transactions for landlord creation, similar to the StudentViewSet. The detailed error handling for ValidationError is commendable. However, ensure that the broad exception handling is refined to prevent potential information exposure.
- {"detail": error_detail}, status=status.HTTP_400_BAD_REQUEST
+ {"detail": "An error occurred during landlord creation."}, status=status.HTTP_500_INTERNAL_SERVER_ERRORCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| def create(self, request, *args, **kwargs): | |
| if request.user.is_student: | |
| return Response( | |
| {"detail": "You can not create a landlord account as a student."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| if request.user.is_landlord: | |
| return Response( | |
| {"detail": "You can not create another landlord account."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| try: | |
| with transaction.atomic(): | |
| city = City.objects.get( # pylint: disable=no-member | |
| pk=request.data.pop("city") | |
| ) | |
| landlord = Landlord.objects.create( # pylint: disable=no-member | |
| user=request.user, city=city, **request.data | |
| ) | |
| request.user.is_landlord = True | |
| request.user.save() | |
| return Response( | |
| LandlordSerializer(landlord, context={"request": request}).data, | |
| status=status.HTTP_201_CREATED, | |
| ) | |
| except ValidationError as e: | |
| if isinstance(e.error_list, list) and e.error_list: | |
| error_detail = { | |
| field: errors[0] for field, errors in e.error_list[0].params.items() | |
| } | |
| else: | |
| error_detail = str(e) | |
| return Response( | |
| {"detail": error_detail}, status=status.HTTP_400_BAD_REQUEST | |
| ) | |
| def create(self, request, *args, **kwargs): | |
| if request.user.is_student: | |
| return Response( | |
| {"detail": "You can not create a landlord account as a student."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| if request.user.is_landlord: | |
| return Response( | |
| {"detail": "You can not create another landlord account."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| try: | |
| with transaction.atomic(): | |
| city = City.objects.get( # pylint: disable=no-member | |
| pk=request.data.pop("city") | |
| ) | |
| landlord = Landlord.objects.create( # pylint: disable=no-member | |
| user=request.user, city=city, **request.data | |
| ) | |
| request.user.is_landlord = True | |
| request.user.save() | |
| return Response( | |
| LandlordSerializer(landlord, context={"request": request}).data, | |
| status=status.HTTP_201_CREATED, | |
| ) | |
| except ValidationError as e: | |
| if isinstance(e.error_list, list) and e.error_list: | |
| error_detail = { | |
| field: errors[0] for field, errors in e.error_list[0].params.items() | |
| } | |
| else: | |
| error_detail = str(e) | |
| return Response( | |
| {"detail": "An error occurred during landlord creation."}, status=status.HTTP_500_INTERNAL_SERVER_ERROR | |
| ) |
Comment on lines
+242
to
+255
| with transaction.atomic(): | ||
| city = City.objects.get( # pylint: disable=no-member | ||
| pk=request.data.pop("city") | ||
| ) | ||
| amenities = request.data.pop("amenities") | ||
| property_ = Property.objects.create( # pylint: disable=no-member | ||
| owner=landlord, city=city, **request.data | ||
| ) | ||
| property_.amenities.set(amenities) | ||
| property_.save() | ||
| return Response( | ||
| PropertySerializer(property_, context={"request": request}).data, | ||
| status=status.HTTP_201_CREATED, | ||
| ) |
There was a problem hiding this comment.
The create method in PropertyViewSet uses atomic transactions for property creation, which is a good practice for ensuring data integrity. However, consider validating the existence of city and amenities before attempting to create the property to avoid unnecessary database transactions in case of invalid data.
Comment on lines
+657
to
+697
| @transaction.atomic | ||
| def update(self, request, *args, **kwargs): | ||
| instance = self.get_object() | ||
| if request.user != instance.property.owner: | ||
| return Response( | ||
| {"detail": "You can only update rooms that you own."}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| try: | ||
| instance.name = request.data.get("name", instance.name) | ||
| instance.num_beds = request.data.get("num_beds", instance.num_beds) | ||
| instance.price = request.data.get("price", instance.price) | ||
| instance.room_type = request.data.get("room_type", instance.room_type) | ||
| instance.available_beds = request.data.get( | ||
| "available_beds", instance.available_beds | ||
| ) | ||
| instance.is_available = request.data.get( | ||
| "is_available", instance.is_available | ||
| ) | ||
| instance.occupied_beds = request.data.get( | ||
| "occupied_beds", instance.occupied_beds | ||
| ) | ||
| instance.display_image = request.data.get( | ||
| "display_image", instance.display_image | ||
| ) | ||
| instance.description = request.data.get("description", instance.description) | ||
| instance.save() | ||
| return Response( | ||
| RoomSerializer(instance, context={"request": request}).data, | ||
| status=status.HTTP_200_OK, | ||
| ) | ||
| except ValidationError as e: | ||
| return Response( | ||
| {"detail": str(e).strip("['").strip("']")}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
| except Exception as e: # pylint: disable=broad-except | ||
| return Response( | ||
| e.args[0].detail if hasattr(e.args[0], "detail") else e.args[0], | ||
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| ) |
There was a problem hiding this comment.
The update method in RoomViewSet has been enhanced to allow room updates only by the property owner, which is a crucial security measure. However, the broad exception handling could potentially expose stack trace information to the end user. It's recommended to refine the error handling to provide a more generic error message.
- e.args[0].detail if hasattr(e.args[0], "detail") else e.args[0],
+ {"detail": "An error occurred during room update."}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| @transaction.atomic | |
| def update(self, request, *args, **kwargs): | |
| instance = self.get_object() | |
| if request.user != instance.property.owner: | |
| return Response( | |
| {"detail": "You can only update rooms that you own."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| try: | |
| instance.name = request.data.get("name", instance.name) | |
| instance.num_beds = request.data.get("num_beds", instance.num_beds) | |
| instance.price = request.data.get("price", instance.price) | |
| instance.room_type = request.data.get("room_type", instance.room_type) | |
| instance.available_beds = request.data.get( | |
| "available_beds", instance.available_beds | |
| ) | |
| instance.is_available = request.data.get( | |
| "is_available", instance.is_available | |
| ) | |
| instance.occupied_beds = request.data.get( | |
| "occupied_beds", instance.occupied_beds | |
| ) | |
| instance.display_image = request.data.get( | |
| "display_image", instance.display_image | |
| ) | |
| instance.description = request.data.get("description", instance.description) | |
| instance.save() | |
| return Response( | |
| RoomSerializer(instance, context={"request": request}).data, | |
| status=status.HTTP_200_OK, | |
| ) | |
| except ValidationError as e: | |
| return Response( | |
| {"detail": str(e).strip("['").strip("']")}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| except Exception as e: # pylint: disable=broad-except | |
| return Response( | |
| e.args[0].detail if hasattr(e.args[0], "detail") else e.args[0], | |
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| ) | |
| @transaction.atomic | |
| def update(self, request, *args, **kwargs): | |
| instance = self.get_object() | |
| if request.user != instance.property.owner: | |
| return Response( | |
| {"detail": "You can only update rooms that you own."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| try: | |
| instance.name = request.data.get("name", instance.name) | |
| instance.num_beds = request.data.get("num_beds", instance.num_beds) | |
| instance.price = request.data.get("price", instance.price) | |
| instance.room_type = request.data.get("room_type", instance.room_type) | |
| instance.available_beds = request.data.get( | |
| "available_beds", instance.available_beds | |
| ) | |
| instance.is_available = request.data.get( | |
| "is_available", instance.is_available | |
| ) | |
| instance.occupied_beds = request.data.get( | |
| "occupied_beds", instance.occupied_beds | |
| ) | |
| instance.display_image = request.data.get( | |
| "display_image", instance.display_image | |
| ) | |
| instance.description = request.data.get("description", instance.description) | |
| instance.save() | |
| return Response( | |
| RoomSerializer(instance, context={"request": request}).data, | |
| status=status.HTTP_200_OK, | |
| ) | |
| except ValidationError as e: | |
| return Response( | |
| {"detail": str(e).strip("['").strip("']")}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| except Exception as e: # pylint: disable=broad-except | |
| return Response( | |
| {"detail": "An error occurred during room update."}, | |
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| ) |
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.
This pull request refactors the store and adds a new RoomState to manage the state of the room form. It also includes various file changes related to the refactoring and addition of the RoomState.
Summary by CodeRabbit
RoomStatein the store for better state management of room information.