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

[#138] Fix request history visibility and add status field #142

Merged
merged 3 commits into from
Apr 20, 2024

Conversation

delano
Copy link
Contributor

@delano delano commented Apr 20, 2024

This pull request fixes the visibility of the request history and adds a new 'status' field to track the status of requests. It improves the requests list component to pre-filter requests client-side, avoiding unnecessary server round trips. Additionally, it updates the request serializers and components to surface the new 'status' field. This PR addresses issue #138.

delano added 2 commits April 19, 2024 16:38
Updates request serializers and components to surface the new 'status'
field on food requests. This allows filtering requests by open/closed
status and displaying the current status directly in request lists for
clearer visibility of request lifecycles.
This change updates the requests list component to pre-filter requests
client-side rather than server-side. It initializes an in-memory
filtered requests store and recomputes visibility on mount instead of
refetching on every status change. This avoids unnecessary round trips
to the server while still keeping the UI updated reactively. It also
adds documentation to explain the authentication flow.
Copy link
Contributor

PR Review

⏱️ Estimated effort to review [1-5]

3, because the PR involves changes across multiple files including serializers, Vue components, and the package lock file. The complexity is moderate, involving backend and frontend changes, and requires understanding of both Django serializers and Vue.js components.

🧪 Relevant tests

No

🔍 Possible issues

Possible Bug: The addition of the "status" field to read_only_fields in FoodRequestUpdateSerializer might be unintentional. If "status" is meant to be updated, it should not be read-only.

Performance Concern: The client-side filtering logic in List.vue might lead to performance issues for a large number of requests. Consider server-side filtering for scalability.

🔒 Security concerns

No

@@ -29,6 +29,7 @@ class Meta:
"date_requested",
"safe_drop_agree",
"safe_drop_instructions",
"status",
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider removing "status" from read_only_fields in FoodRequestUpdateSerializer if the status is intended to be updated by the user or through the API. This change allows the status field to be updated when necessary. [important]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not meant to be updatable via the API. Only by logic in our code.

@@ -9,7 +9,16 @@ const props = defineProps<{
cta?: Boolean
}>()

// A list of all the users in your account including their name, title, email and role.
const visibleRequests = ref(props.requests || [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Implement server-side filtering for the requests based on their status to improve performance and scalability. This approach reduces the client-side workload and ensures the application can handle a large number of requests efficiently. [important]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're using this approach for quicker prototyping while we gather the initial feedback from users.

apps/ui/components/requests/List.vue Outdated Show resolved Hide resolved
@@ -78,7 +78,7 @@ dependencies:
specifier: ^3.2.5
version: 3.2.5
vue:
specifier: ^3.4.21
specifier: ^3.4.0
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 the downgrade of Vue from ^3.4.21 to ^3.4.0 is intentional and tested for compatibility with the project's dependencies. Unintended downgrades can introduce bugs or compatibility issues. [important]

Copy link
Contributor

qodo-merge-pro bot commented Apr 20, 2024

PR Code Suggestions

CategorySuggestions                                                                                                                                                       
Enhancement
Add validation or documentation for the new "status" field.

Ensure that the status field is appropriately validated or documented to prevent any
unexpected issues related to data integrity or application logic. Consider adding
validation rules or a help_text attribute to clarify its usage and expected values.

apps/api/afbcore/serializers/food_request_serializer.py [32]

-"status",
+"status",  # Add validation or documentation here
 
Simplify reactivity with direct computed usage.

Use the Vue computed property directly in the template to simplify the component and
improve reactivity, instead of assigning its value to a ref during onMounted.

apps/ui/components/requests/List.vue [17-18]

-let computedValue = computed(() => visibleRequests.value.some(request => request.status !== 'delivered'))
-hasOpenRequests.value = computedValue.value
+const hasOpenRequests = computed(() => visibleRequests.value.some(request => request.status !== 'delivered'))
 
Check authentication status before fetching requests.

Consider checking the authStatus before fetching requests to ensure the user is
authenticated, preventing unnecessary API calls and potential errors.

apps/ui/pages/dashboard/index.vue [36]

 const fetchRequests = async () => {
+  if (authStatus.value !== 'authenticated') return;
 
Maintainability
Remove or replace the console.log statement.

Replace the console.log statement with a more production-friendly approach to logging or
remove it if it was used for debugging purposes during development.

apps/ui/components/requests/List.vue [16]

-console.log('requests', visibleRequests.value)
+// Consider removing this console.log if it was for debugging purposes
 
Possible issue
Verify compatibility of the updated Vue version with project dependencies.

Verify the updated Vue version (3.4.0) for compatibility with your project dependencies,
especially if you're using plugins or third-party libraries that depend on a specific Vue
version range.

pnpm-lock.yaml [81]

-specifier: ^3.4.0
+specifier: ^3.4.0  # Ensure compatibility with project dependencies
 

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

  • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

Copy link
Contributor

qodo-merge-pro bot commented Apr 20, 2024

CI Failure Feedback

(Checks updated until commit 39bf1cf)

Action: install-and-cache (ubuntu-latest, 3.11, 5)

Failed stage: Install django 5 [❌]

Failure summary:

The action failed due to the inability to find a matching version of the package Django. This
occurred during the process of recreating a virtual environment, indicating that the specified
version of Django might not be available in the package repository being used or there is a
misconfiguration in the package version specified.

Relevant error logs:
1:  ##[group]Operating System
2:  Ubuntu
...

623:  Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.9/x64
624:  LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.9/x64/lib
625:  VENV: .venv/bin/activate
626:  VIRTUAL_ENV: .venv
627:  ##[endgroup]
628:  The virtual environment found in .venv seems to be broken.
629:  Recreating virtualenv afb requests in /home/runner/work/afb-requests/afb-requests/.venv
630:  Could not find a matching version of package Django
631:  ##[error]Process completed with exit code 1.

✨ CI feedback usage guide:

The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
The tool analyzes the failed checks and provides several feedbacks:

  • Failed stage
  • Failed test name
  • Failure summary
  • Relevant error logs

In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

/checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

Configuration options

  • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
  • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
  • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
  • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
  • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

See more information about the checks tool in the docs.

@delano delano changed the title Fix request history visibility and add status field [#138] Fix request history visibility and add status field Apr 20, 2024
Signed-off-by: Delano <1206+delano@users.noreply.github.com>
@delano delano merged commit f69b8c2 into 134-updates-from-privacy-review Apr 20, 2024
0 of 2 checks passed
@delano delano deleted the 138-request-history branch April 20, 2024 16:40
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.

1 participant