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

#154: Improve user and profile serialization #166

Merged
merged 9 commits into from
Jun 5, 2024
Merged

Conversation

delano
Copy link
Contributor

@delano delano commented Jun 4, 2024

This pull request includes several changes to improve the user and profile serialization. The changes are as follows:

  • In the user serializer, switch from a single nested ProfileSerializer to handling multiple profiles via the many=True argument. This allows for a one-to-many relationship between users and profiles.

  • In the ProfileSerializer, add a read-only user field to represent the reverse relation.

  • Change the phone_number field in the ProfileSerializer to a plain string to simplify handling of different formats.

  • In the FoodRequest model, rename the status field to request_status for clarity.

  • Update the magic login page to correctly disable the code input field when no code has been requested yet.

Fixes #154.

delano added 4 commits June 4, 2024 13:04
e.g. Profile.status (which has different choices)
In the user serializer, switch from a single nested ProfileSerializer to
handling multiple profiles via the `many=True` argument. This allows for
a one-to-many relationship between users and profiles.

Additionally, in the ProfileSerializer:
- Add a read-only user field to represent the reverse relation
- Change the phone_number field to a plain string to simplify handling
of different formats

In the FoodRequest model, rename the `status` field to `request_status`
for clarity.

Update the magic login page to correctly disable the code input field
when no code has been requested yet.
@delano delano self-assigned this Jun 4, 2024
@delano delano marked this pull request as draft June 4, 2024 20:06
@delano delano marked this pull request as ready for review June 4, 2024 20:06
Copy link
Contributor

qodo-merge-pro bot commented Jun 4, 2024

PR Review 🔍

(Review updated until commit 442579e)

⏱️ Estimated effort to review [1-5]

3, because the PR involves changes across multiple layers including the frontend, model layer, and API serializers. Understanding the implications of these changes requires a good grasp of the system's architecture and the Django framework.

🧪 Relevant tests

No

⚡ Possible issues

Field Renaming in Model: Renaming the status field to request_status in the FoodRequest model without a corresponding database migration could lead to issues. However, this concern is mitigated by the inclusion of a migration file in the PR.

User and Profile Serialization: Changing the user serialization to handle multiple profiles with many=True and adding a read-only user field in the ProfileSerializer could introduce breaking changes to existing API consumers expecting a single profile object or not expecting the user field in profile responses.

🔒 Security concerns

No

Copy link
Contributor

qodo-merge-pro bot commented Jun 4, 2024

Persistent review updated to latest commit 442579e

@@ -119,7 +119,7 @@ async function authHandler() {
<p class="text-gray-500 dark:text-gray-400 text-center">Don't have an account? <NuxtLink to="/login" class="text-primary font-medium">Sign up</NuxtLink>.</p>

<UInput v-model="email" name="email" type="hidden" label="Email" required />
<UInput v-model="code" name="code" type="text" color="primary" variant="outline" :disabled="state.code" placeholder="Code" label="Code" size="xl" class="w-32 mx-auto" maxLength="6" required />
<UInput v-model="code" name="code" type="text" color="primary" variant="outline" :disabled="state.code !== null" placeholder="Code" label="Code" size="xl" class="w-32 mx-auto" maxLength="6" required />
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a loading spinner or disabling the "Sign In" button while the authentication request is in progress. This can improve user experience by providing feedback that the system is working on their request. [important]

@@ -96,7 +96,7 @@ class FoodRequest(BaseAbstractModel):
date_requested = models.DateField(auto_now_add=True)

# Request Received, Request Approved & in Queue, Request Denied, Volunteer Assigned, Request Ready For Volunteer Pickup, Delivery Scheduled, Out For Delivery, Delivered, Undeliverable
status = models.CharField(
request_status = models.CharField(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that all existing data in the status field is correctly migrated to request_status and that any code relying on the status field is updated to use request_status. This is crucial to prevent data loss or inconsistencies during the migration. [important]

# phone = serializers.CharField(required=False, allow_blank=True)
profile = ProfileSerializer(required=False)
# A user can have multiple profiles
profiles = ProfileSerializer(many=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since users can have multiple profiles, ensure that the frontend and any consuming services are updated to handle the profiles field as an array. This may require adjustments in the UI rendering logic and data handling. [important]

@@ -18,7 +18,7 @@

# Example of a viewset with custom actions.
#
class CurrentUserAPIView(CreateAPIView):
class CurrentUserAPIView(RetrieveAPIView):
Copy link
Contributor

Choose a reason for hiding this comment

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

After changing CurrentUserAPIView from CreateAPIView to RetrieveAPIView, ensure that the frontend correctly handles the API response. This change implies that the view now supports GET requests for fetching the current user's data, which might require adjustments in the API consumption logic. [medium]

delano and others added 5 commits June 4, 2024 15:06
Co-authored-by: codiumai-pr-agent-pro[bot] <151058649+codiumai-pr-agent-pro[bot]@users.noreply.github.com>
Signed-off-by: Delano <1206+delano@users.noreply.github.com>
Co-authored-by: codiumai-pr-agent-pro[bot] <151058649+codiumai-pr-agent-pro[bot]@users.noreply.github.com>
Signed-off-by: Delano <1206+delano@users.noreply.github.com>
…uest_status.py

Co-authored-by: codiumai-pr-agent-pro[bot] <151058649+codiumai-pr-agent-pro[bot]@users.noreply.github.com>
Signed-off-by: Delano <1206+delano@users.noreply.github.com>
@delano delano merged commit 20e4941 into dev Jun 5, 2024
2 checks passed
@delano delano deleted the 154-user-profiles branch June 5, 2024 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✔️ Done
Development

Successfully merging this pull request may close these issues.

[View] Client profile details
1 participant