feat: new pickup system #246
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the The existing content is already comprehensive and aligns with the provided instructions. Therefore, no modifications are necessary. TipsChat with CodeRabbit Bot (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| </a> | ||
| {% endif %} | ||
| {% if connect_with.primary_email %} | ||
| <a href="mailto:{{ connect_with.primary_email }}" |
Check warning
Code scanning / CodeQL
Potentially unsafe external link Medium
| </a> | ||
| {% endif %} | ||
| {% if connect_with.profile.instagram %} | ||
| <a href="{{ connect_with.profile.instagram }}" |
Check warning
Code scanning / CodeQL
Potentially unsafe external link Medium
| </a> | ||
| {% endif %} | ||
| {% if connect_with.profile.facebook %} | ||
| <a href="{{ connect_with.profile.facebook }}" |
Check warning
Code scanning / CodeQL
Potentially unsafe external link Medium
![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: 22
Configuration used: CodeRabbit UI
Files selected for processing (41)
- fiesta/apps/accounts/models/user.py (1 hunks)
- fiesta/apps/buddy_system/models/configuration.py (1 hunks)
- fiesta/apps/buddy_system/urls.py (2 hunks)
- fiesta/apps/buddy_system/views/editor.py (3 hunks)
- fiesta/apps/buddy_system/views/matching.py (3 hunks)
- fiesta/apps/buddy_system/views/request.py (2 hunks)
- fiesta/apps/dashboard/templates/dashboard/index.html (1 hunks)
- fiesta/apps/fiestarequests/models/configuration.py (2 hunks)
- fiesta/apps/fiestarequests/models/request.py (1 hunks)
- fiesta/apps/fiestarequests/tables/editor.py (1 hunks)
- fiesta/apps/fiestarequests/views/editor.py (1 hunks)
- fiesta/apps/fiestarequests/views/matching.py (1 hunks)
- fiesta/apps/fiestarequests/views/request.py (1 hunks)
- fiesta/apps/pickup_system/admin.py (1 hunks)
- fiesta/apps/pickup_system/apps.py (1 hunks)
- fiesta/apps/pickup_system/forms.py (1 hunks)
- fiesta/apps/pickup_system/migrations/0001_initial.py (1 hunks)
- fiesta/apps/pickup_system/models/init.py (1 hunks)
- fiesta/apps/pickup_system/models/configuration.py (1 hunks)
- fiesta/apps/pickup_system/models/files.py (1 hunks)
- fiesta/apps/pickup_system/models/request.py (1 hunks)
- fiesta/apps/pickup_system/templates/pickup_system/dashboard_block.html (1 hunks)
- fiesta/apps/pickup_system/templates/pickup_system/index_international.html (1 hunks)
- fiesta/apps/pickup_system/templates/pickup_system/index_member.html (1 hunks)
- fiesta/apps/pickup_system/templates/pickup_system/matching_requests.html (1 hunks)
- fiesta/apps/pickup_system/templates/pickup_system/my_pickups.html (1 hunks)
- fiesta/apps/pickup_system/templates/pickup_system/parts/pickup_request_note_help.html (1 hunks)
- fiesta/apps/pickup_system/templates/pickup_system/parts/request_match_card.html (1 hunks)
- fiesta/apps/pickup_system/templates/pickup_system/parts/requests_editor_match_btn.html (1 hunks)
- fiesta/apps/pickup_system/templatetags/pickup_system.py (1 hunks)
- fiesta/apps/pickup_system/urls.py (1 hunks)
- fiesta/apps/pickup_system/views/init.py (1 hunks)
- fiesta/apps/pickup_system/views/editor.py (1 hunks)
- fiesta/apps/pickup_system/views/index.py (1 hunks)
- fiesta/apps/pickup_system/views/matches.py (1 hunks)
- fiesta/apps/pickup_system/views/matching.py (1 hunks)
- fiesta/apps/pickup_system/views/request.py (1 hunks)
- fiesta/apps/plugins/migrations/0017_alter_plugin_app_label.py (1 hunks)
- fiesta/apps/sections/models/section.py (1 hunks)
- fiesta/apps/sections/views/membership.py (5 hunks)
- fiesta/fiesta/settings/project.py (1 hunks)
Files skipped from review due to trivial changes (6)
- fiesta/apps/buddy_system/urls.py
- fiesta/apps/dashboard/templates/dashboard/index.html
- fiesta/apps/pickup_system/models/init.py
- fiesta/apps/pickup_system/templates/pickup_system/index_international.html
- fiesta/apps/pickup_system/templates/pickup_system/parts/pickup_request_note_help.html
- fiesta/apps/pickup_system/views/init.py
Additional comments: 76
fiesta/apps/pickup_system/models/configuration.py (1)
- 1-16: The new
PickupSystemConfigurationclass is correctly inheriting fromBaseRequestSystemConfigurationand the verbose names are set appropriately. The class is also correctly exported in the__all__list. Ensure that the...on line 9 is replaced with the actual implementation of the class.fiesta/fiesta/settings/project.py (1)
- 63-67: The new app "apps.pickup_system.apps.PickupSystemConfig" has been added to the list of installed apps. Ensure that the app is correctly configured and all necessary migrations have been created and applied.
fiesta/apps/pickup_system/templates/pickup_system/parts/requests_editor_match_btn.html (1)
- 1-11: The code looks good. It uses Django's template language to dynamically generate the anchor tag's href attribute and button text based on the
record.matchvalue. It also uses Alpine.js for modal handling. However, ensure that themodalfunction andbindobject are defined in your Alpine.js context.fiesta/apps/fiestarequests/models/configuration.py (2)
3-8: The import of
MatchingPoliciesRegisteris removed. Ensure that this does not affect other parts of the code that might be using it. Also, verify that the removal of this import does not break any functionality related to matching policies.16-25: The
matching_policyfield and its associated propertymatching_policy_instanceare removed. This is a significant change and could potentially break the application if not handled properly. Ensure that all references to these removed elements are updated accordingly throughout the codebase. Also, verify that the removal of these elements does not affect the functionality of the application, especially in terms of matching policies.fiesta/apps/pickup_system/apps.py (1)
- 16-58: The
PickupSystemConfigclass seems to be well defined and follows Django's app configuration conventions. Theas_navigation_itemmethod correctly checks for user membership status before adding navigation items. However, ensure that the URLs "my-pickups" and "requests" are correctly defined in your URL configuration.fiesta/apps/pickup_system/admin.py (1)
- 1-22: The code looks good and follows Django's best practices for registering models in the admin interface. The use of inheritance for the admin classes is a good practice as it promotes code reusability and maintainability. However, ensure that the base admin classes (
BaseChildConfigurationAdmin,BaseRequestAdmin,BaseRequestMatchAdmin) provide the necessary fields and functionality for the new models.fiesta/apps/pickup_system/templates/pickup_system/index_member.html (1)
- 1-17: The template seems to be correctly extending the base template and loading the necessary Django template tags. The main block contains the necessary HTML and Django template language to display the pickup system interface. The use of the
{% trans %}tag for text strings is a good practice for internationalization. The link to the "matching-requests" URL appears to be correctly formed.fiesta/apps/plugins/migrations/0017_alter_plugin_app_label.py (1)
- 1-18: The migration script looks good. It alters the
app_labelfield in thePluginmodel, adding achoicesparameter and updating themax_lengthparameter to 256. This change will affect how theapp_labelfield is displayed and validated in the admin interface. Ensure that the new choices and max_length are compatible with the existing data in theapp_labelfield to avoid data loss or integrity issues.fiesta/apps/fiestarequests/models/request.py (1)
- 24-27: The
matchfield is added to theBaseRequestProtocolclass. This field is a foreign key to amodels.Model. Ensure that theBaseRequestMatchProtocolmodel is correctly defined and that the relationship betweenBaseRequestProtocolandBaseRequestMatchProtocolis correctly set up. Also, ensure that thematchfield is correctly handled in the views and templates whereBaseRequestProtocolis used.fiesta/apps/pickup_system/views/matches.py (2)
1-1: The
__future__import for annotations is a good practice for Python 3.7 and below. However, if you're using Python 3.8 or above, this import is not necessary as the future behavior is the default.14-17: The
get_querysetmethod is correctly overridden to customize the queryset. The use ofprefetch_relatedandselect_relatedshould help optimize database queries.fiesta/apps/pickup_system/migrations/0001_initial.py (1)
- 1-83: The migration file seems to be correctly generated by Django and creates the necessary models for the pickup system. The models
PickupRequest,PickupSystemConfiguration, andPickupRequestMatchare created with appropriate fields and options. The foreign key relationships and on_delete policies seem to be correctly set up. The verbose names and other options are also appropriately defined.However, please ensure that the dependencies listed in the migration file are correct and that the migration has been tested to work correctly with your database.
Also, consider adding help_text to the fields in your models to provide more context about what each field is used for. This can be especially helpful for other developers who might work on this code in the future.
fiesta/apps/pickup_system/models/request.py (3)
1-10: The use of
base_request_model_factoryto generate base classes for the new models is a good practice as it promotes code reusability and maintainability. However, ensure that the base classes provide all the necessary fields and methods required by the new models.13-22: The
PickupRequestmodel is well defined. However, there's a TODO comment on line 14 indicating that date/time/place fields might need to be added. Ensure to address this before merging the changes.24-30: The
PickupRequestMatchmodel is also well defined. The__str__method provides a human-readable representation of the model which is a good practice.fiesta/apps/buddy_system/views/request.py (2)
3-21: The import statements are well organized and follow the PEP 8 style guide. The new import statement for
BaseNewRequestViewis correctly placed.92-106: The refactoring of
NewRequestViewintoNewBuddyRequestViewis done correctly. The class now inherits fromBaseNewRequestViewand theform_validmethod is removed as its functionality is now handled by the base class. Theget_initialmethod is overridden to use the base class'sget_initialmethod and add additional initial data. The removal of the mixinsEnsureInSectionSpaceViewMixinandEnsureInternationalUserViewMixinis not an issue as long as the functionality they provided is now handled elsewhere.fiesta/apps/fiestarequests/views/matching.py (4)
1-14: Imports and class definitions seem to be in order. Ensure that all imported modules and classes are used in the code.
16-18: The class
BaseTakeRequestViewis defined with multiple mixins and a base class. Ensure that the order of the mixins and base class is correct according to the Method Resolution Order (MRO).19-41: The
postmethod is decorated with@transaction.atomicwhich ensures that all database operations within the method are executed within a single database transaction. This is a good practice for operations that should be atomic. However, there are a couple of TODO comments in the code that need to be addressed. Also, the redirect URL at the end of the method is currently hardcoded to the root ("/"). Consider making this dynamic or configurable.35-37: The
BaseRequestProtocolobjectbris updated and saved. Ensure that thematchfield exists in theBaseRequestProtocolmodel and that theState.MATCHEDvalue is valid for thestatefield.fiesta/apps/pickup_system/templates/pickup_system/matching_requests.html (2)
- 1-140: The template seems to be well-structured and follows Django's best practices. It uses Django's template language effectively to display dynamic content and control the flow of the template. It also uses Django's internationalization features to support multiple languages, which is a good practice for accessibility and user experience.
However, there are a few points to consider:
- 135-139: Good use of Django's ORM to check if the
object_listqueryset has any objects. This is more efficient than checking the length of the queryset.fiesta/apps/pickup_system/views/editor.py (5)
1-21: The import statements look good and are well organized. They are grouped by standard library, third-party, and local application imports, which is a good practice.
23-41: The
PickupRequestsTableclass extendsBaseRequestsTableand adds a new columnmatch_request. The__init__method is overridden to add a link to theissuer_namecolumn. This looks fine, but ensure that thepickup_system:editor-detailURL exists and is correctly configured.43-52: The
PickupRequestsEditorViewclass is well defined and inherits from appropriate mixins and base classes. Theget_querysetmethod is overridden to return a queryset ofpickup_system_requestsrelated to the current section. Ensure that thepickup_system_requestsattribute exists on thein_space_of_sectionobject.59-75: The
PickupRequestEditorDetailViewclass is well defined and inherits from appropriate mixins and base classes. Theget_context_datamethod is overridden to add aform_urlto the context. Ensure that thepickup_system:editor-detailURL exists and is correctly configured.82-92: The
QuickPickupMatchViewclass is well defined and inherits fromBaseQuickRequestMatchView. Ensure that thepickup_system:requestsandpickup_system:quick-matchURLs exist and are correctly configured.fiesta/apps/pickup_system/models/files.py (1)
- 15-16: The
get_request_querysetmethod is declared but not implemented in bothBaseIssuerPictureServeViewandBaseMatcherPictureServeViewclasses. This method should be implemented in the subclasses that inherit from these base classes. If not, it will raise aNotImplementedErrorwhen called. Ensure that all subclasses have this method implemented.fiesta/apps/sections/views/membership.py (3)
12-20: The new imports look fine and are necessary for the new functionality.
57-76: The
get_context_datamethod has been updated to include the newPickupSystemConfigandPickupRequestsTable. The logic seems correct and the use of theclean_listfunction to filter outNonevalues is appropriate.89-112: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [77-137]
The
get_tablesmethod has been updated to include the newPickupSystemConfigandPickupRequestsTable. The logic seems correct and the use of theclean_listfunction to filter outNonevalues is appropriate.fiesta/apps/pickup_system/urls.py (2)
13-24: The URL patterns are well defined and follow Django's best practices. The use of
as_view()method for class-based views is correct. The names provided for each URL will be helpful for reverse URL resolution.26-27: The use of a proxy view to serve profile pictures is a good practice as it abstracts away the details of the file storage backend. However, ensure that appropriate access controls are in place in the
IssuerPictureServeViewandMatcherPictureServeViewto prevent unauthorized access to user profile pictures.fiesta/apps/buddy_system/views/matching.py (4)
1-13: The imports look clean and organized. No unused imports are present. The import order follows the PEP8 guidelines.
1-15: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [15-33]
The
MatchingRequestsViewclass is well-structured and follows Django's class-based view conventions. The use of mixins for common functionality is a good practice. Thehas_permissionandget_querysetmethods are correctly overridden.
- 34-45: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [34-50]
The
TakeBuddyRequestViewclass is also well-structured. The addition of thematch_modelattribute is a good practice as it allows for more flexibility and reusability of the class. Thehas_permissionandget_querysetmethods are correctly overridden.
- 52-61: The
IssuerPictureServeViewandMatcherPictureServeViewclasses are correctly using theServeFilesFromPickupsMixinfor serving files. This is a good example of code reuse and modularity.fiesta/apps/buddy_system/models/configuration.py (3)
10-16: The
matching_policyfield is added to theBuddySystemConfigurationmodel. It uses Django'sCharFieldwith a default value, choices, a maximum length, and a help text. This is a good practice as it provides flexibility in defining the matching policy and also provides helpful context to the user or developer.18-21: The
matching_policy_instanceproperty is added to retrieve the matching policy based on the selected ID. This is a good practice as it encapsulates the logic for retrieving the matching policy and makes it easily accessible as a property of the model. However, there is a TODO comment about passing configuration. It would be good to clarify what this means and whether it needs to be addressed.- # TODO: pass configuration? + # TODO: pass configuration? Please clarify or address this comment.
- 23-24: The
Metaclass is used to provide additional information about theBuddySystemConfigurationmodel. This is a good practice as it allows for customization of the model's behavior and presentation in Django's admin interface and other areas.fiesta/apps/pickup_system/views/matching.py (1)
- 1-55: The code looks good overall. However, there are a few points to consider:
- Ensure that the
__future__import is necessary. If you're using Python 3.7 or later, you don't need to import annotations from__future__as they are included by default.- Make sure that the
...inIssuerPictureServeViewandMatcherPictureServeVieware placeholders for actual code. If not, these classes will raise aSyntaxError.- Ensure that the
BaseIssuerPictureServeViewandBaseMatcherPictureServeViewclasses are defined somewhere in your codebase, as they are imported but not defined in this file.- Verify that the
get_querysetmethods inMatchingRequestsViewandTakePickupRequestVieware returning the expected results. They seem to be filtering onstate=CREATED, so make sure this is the intended behavior.- Check that the
get_request_querysetmethod inServeFilesFromPickupsMixinis returning the expected results. It seems to be returning all pickup system requests for the current section, so make sure this is the intended behavior.fiesta/apps/fiestarequests/tables/editor.py (4)
1-16: The imports seem to be in order and are correctly organized. However, ensure that all imported modules and classes are used in the code. Unused imports can lead to confusion and unnecessary memory usage.
18-20: The
related_facultiesfunction is correctly defined and seems to return the expected queryset. However, ensure that therequest.in_space_of_sectionattribute is always available and correctly set when this function is called.22-49: The
BaseRequestsFilterclass is well defined. The filters are correctly set up and thefilter_searchmethod seems to be correctly implemented. However, ensure that thesearchfilter correctly handles all possible input values and doesn't raise exceptions for unexpected input.54-104: The
BaseRequestsTableclass is well defined. The columns are correctly set up and theMetaclass seems to be correctly implemented. However, ensure that thesequenceattribute correctly lists all columns in the desired order and doesn't include any columns that are not defined. Also, ensure that theattrsattribute is correctly set and doesn't cause any issues with the rendering of the table.fiesta/apps/pickup_system/templates/pickup_system/dashboard_block.html (5)
1-3: Ensure that the custom template tags
pickup_system,utils, andi18nare correctly defined and imported. Also, verify that they are being used in the template.6-6: Ensure that the
get_pickup_system_configurationtemplate tag is correctly defined and returns the expected configuration object.11-53: This block handles the display of the pickup system for international users. It checks the state of the current pickup request and displays the appropriate information and actions. Ensure that the logic for determining the state of the pickup request and the corresponding display is correct.
55-69: This block handles the display of the pickup system for local users when there are waiting requests to match. It displays the number of waiting requests and provides a link to show them if they exist. Ensure that the logic for determining the number of waiting requests and the corresponding display is correct.
72-127: This block handles the display of the pickup system for local users. It checks if there are any matched pickup requests and displays the appropriate information and actions. Ensure that the logic for determining the state of the matched pickup requests and the corresponding display is correct.
fiesta/apps/fiestarequests/views/request.py (2)
1-20: The
BaseNewRequestViewclass is well-structured and inherits from several mixins and views. This is a good practice as it promotes code reusability and modularity. However, ensure that the order of inheritance is correct and that there are no conflicts between the methods of the parent classes.33-38: The
form_validmethod is overridden to ensure certain fields are set before form validation. This is a good practice as it ensures data integrity. However, it's important to ensure that theform_validmethod of the parent class is called at the end of this method, which is correctly done here.fiesta/apps/pickup_system/templates/pickup_system/parts/request_match_card.html (5)
1-5: Good use of Django's template tag system to load necessary functionalities.
11-13: The use of Django's built-in filters to convert the request state to a CSS variant is a good practice. It keeps the logic in the template minimal and improves readability.
27-37: The use of conditional rendering to display the user's picture or initials based on the availability of the picture is a good practice. It ensures that there is always some visual representation for the user.
46-158: The conditional rendering based on the state of the request (
MATCHEDorCREATED) is well implemented. It ensures that the correct information is displayed based on the state of the request.165-171: The use of a custom template tag to get the number of waiting pickup requests placed before the current one is a good practice. It abstracts away the logic from the template and improves readability.
fiesta/apps/pickup_system/templatetags/pickup_system.py (7)
30-33: This function censors sensitive information in a description. It's important to ensure that this function is used wherever necessary to prevent the accidental exposure of sensitive information.
35-45: This function retrieves the latest pickup request of the current user. It's important to ensure that the user has the necessary permissions to view the returned request.
47-59: This function retrieves all pickup requests in the "CREATED" state. It's important to ensure that the user has the necessary permissions to view the returned requests.
61-69: This function retrieves the count of pickup requests placed before a given request. It's important to ensure that the user has the necessary permissions to view the returned count.
71-80: This function retrieves all matched pickup requests of the current user. It's important to ensure that the user has the necessary permissions to view the returned requests.
82-89: This function maps the state of a pickup request to a CSS class. It's important to ensure that all possible states are covered in this mapping.
91-99: This function retrieves the configuration of the pickup system. It's important to ensure that the user has the necessary permissions to view the returned configuration.
fiesta/apps/fiestarequests/views/editor.py (3)
1-22: The
BaseQuickRequestMatchViewclass is well-structured and inherits from the necessary mixins andUpdateView. The class-level attributes are defined correctly.30-33: The
get_context_datamethod is correctly implemented. It addsform_urlto the context, which is reversed with the object's primary key. This is a good practice as it allows the URL to be dynamically generated based on the object.35-45: The
get_initialmethod is correctly implemented. It tries to get thematcherandmatcher_facultyfrom the object's match. If the match does not exist, it returns an empty dictionary. This is a good practice as it allows the form to be pre-filled with existing data if it exists.fiesta/apps/buddy_system/views/editor.py (6)
23-28: The
match_requestcolumn is added to theBuddyRequestsTableclass. Ensure that the templatebuddy_system/parts/requests_editor_match_btn.htmlexists and is correctly formatted.30-31: The
Metaclass ofBuddyRequestsTableis modified to include the newmatch_requestfield. This change seems fine as long as thematch_requestfield is intended to be included in the table.33-42: The
__init__method ofBuddyRequestsTableis modified. The changes seem to be adding a link to theissuer_namecolumn if it exists. Ensure that thebuddy_system:editor-detailroute exists and is correctly configured.69-70: The
template_nameandajax_template_nameattributes ofBuddyRequestEditorDetailVieware changed. Ensure that the new templatesfiestaforms/pages/card_page_for_ajax_form.htmlandfiestaforms/parts/ajax-form-container.htmlexist and are correctly formatted.77-80: The
get_context_datamethod is added toBuddyRequestEditorDetailView. This method adds aform_urlto the context. Ensure that thepickup_system:editor-detailroute exists and is correctly configured.83-93: The
QuickBuddyMatchViewclass is replaced withBaseQuickRequestMatchView, and theform_urlandmatch_modelattributes are added. Ensure that thebuddy_system:quick-matchroute exists and is correctly configured, and thatBuddyRequestMatchis the correct model to use for matching.fiesta/apps/pickup_system/forms.py (1)
- 1-17: The import statements and constant definitions are well-organized and follow PEP 8 style guide. The
fields_for_modelfunction is used to get a dictionary of form fields for theUserProfilemodel, which is a good use of Django's form and model utilities.
| class MyPickups(EnsureLocalUserViewMixin, ListView): | ||
| request: HttpRequest | ||
|
|
There was a problem hiding this comment.
The request attribute is not typically declared in Django views. It's automatically provided by Django during the request-response cycle. If you're declaring it for type hinting, consider moving it to the method where it's used.
|
|
||
| # only typing of related manager | ||
| buddy_system_requests: models.QuerySet | ||
| pickup_system_requests: models.QuerySet |
There was a problem hiding this comment.
The buddy_system_requests and pickup_system_requests attributes are declared as models.QuerySet but they are not initialized or defined anywhere in the Section class. This could lead to confusion and potential errors. If these are meant to be related fields from another model, they should be defined in the related model using models.ForeignKey, models.OneToOneField, or models.ManyToManyField with the related_name argument set to buddy_system_requests or pickup_system_requests respectively. If they are meant to be methods that return a QuerySet, they should be defined as such.
# If they are related fields, they should be defined in the related model like this:
class BuddySystemRequest(models.Model):
section = models.ForeignKey(Section, on_delete=models.CASCADE, related_name='buddy_system_requests')
class PickupSystemRequest(models.Model):
section = models.ForeignKey(Section, on_delete=models.CASCADE, related_name='pickup_system_requests')
# If they are methods, they should be defined like this:
class Section(BaseTimestampedModel):
# ...
def buddy_system_requests(self):
return BuddySystemRequest.objects.filter(section=self)
def pickup_system_requests(self):
return PickupSystemRequest.objects.filter(section=self)| # TODO: check matcher relation to responsible section | ||
| # TODO: reset any previous match for this BR |
There was a problem hiding this comment.
There are TODO comments indicating that there are additional checks and operations that need to be implemented. Ensure that these are addressed before merging the changes.
| <img src="{% url "pickup_system:serve-issuer-profile-picture" br.issuer.profile.picture %}" | ||
| alt="{% trans "request issuer photo" %}" | ||
| class="rounded-full w-24 h-24 m-4"> |
There was a problem hiding this comment.
Ensure that the URL "pickup_system:serve-issuer-profile-picture" is correctly defined in your Django URL configuration and that it correctly serves the profile picture. Also, consider adding a default image in case the profile picture is not available.
63:
Ensure that the censor_description filter is correctly implemented and does not inadvertently remove or alter important information.
71:
Ensure that the URL "pickup_system:take-pickup-request" is correctly defined in your Django URL configuration and that it correctly handles the POST request for taking a pickup request.
109:
The placeholder text is quite long and specific. Consider shortening it or making it more generic to accommodate different use cases.
123:
Ensure that the button disabling logic works correctly. The button should be disabled if noteEnabled is true and note is empty.
| @@ -0,0 +1,28 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
The __future__ import is not necessary in Python 3.7 and above as annotations are a part of the language standard. If you're using Python 3.7 or above, you can safely remove this line.
- from __future__ import annotationsCommitable 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.
| from __future__ import annotations |
| from __future__ import annotations | ||
|
|
||
| from django.http import HttpResponseRedirect | ||
| from django.urls import reverse | ||
| from django.views.generic import TemplateView | ||
|
|
||
| from apps.pickup_system.models import PickupRequest, PickupSystemConfiguration | ||
| from apps.plugins.views import PluginConfigurationViewMixin | ||
| from apps.sections.middleware.user_membership import HttpRequest | ||
| from apps.sections.views.mixins.section_space import EnsureInSectionSpaceViewMixin | ||
|
|
||
|
|
||
| class PickupSystemIndexView( | ||
| EnsureInSectionSpaceViewMixin, | ||
| PluginConfigurationViewMixin[PickupSystemConfiguration], | ||
| TemplateView, | ||
| ): | ||
| request: HttpRequest | ||
|
|
||
| extra_context = { | ||
| "RequestState": PickupRequest.State, | ||
| } | ||
|
|
||
| def get(self, request, *args, **kwargs): | ||
| if ( | ||
| self.request.membership.is_international | ||
| and not self.request.in_space_of_section.pickup_system_requests.filter(issuer=self.request.user).exists() | ||
| ): | ||
| return HttpResponseRedirect(reverse("pickup_system:wanna-pickup")) | ||
|
|
||
| return super().get(request, *args, **kwargs) | ||
|
|
||
| def get_context_data(self, **kwargs): | ||
| data = super().get_context_data(**kwargs) | ||
|
|
||
| data.update( | ||
| {"requests": self.request.in_space_of_section.pickup_system_requests.filter(issuer=self.request.user)} | ||
| ) | ||
| return data | ||
|
|
||
| def get_template_names(self): | ||
| return [ | ||
| ( | ||
| "pickup_system/index_international.html" | ||
| if self.request.membership.is_international | ||
| else "pickup_system/index_member.html" | ||
| ) | ||
| ] |
There was a problem hiding this comment.
The PickupSystemIndexView class seems to be well-structured and follows Django's best practices. However, there are a few points to consider:
-
The
getmethod checks if the user is an international member and if they have any pickup system requests. If they don't, it redirects them to the "wanna-pickup" page. This logic might be better placed in a separate method or middleware to keep thegetmethod clean and focused on its main task: handling GET requests. -
The
get_context_datamethod adds the user's pickup system requests to the context. This is a good practice, but it might be more efficient to use Django'sselect_relatedorprefetch_relatedmethods when retrieving the requests to reduce the number of database queries. -
The
get_template_namesmethod returns a different template name based on whether the user is an international member. This is a good use of Django's flexible template system, but it might be clearer to use an if-else statement instead of a ternary operator for better readability.
Here are the suggested changes:
def get(self, request, *args, **kwargs):
if self.should_redirect_to_wanna_pickup():
return HttpResponseRedirect(reverse("pickup_system:wanna-pickup"))
return super().get(request, *args, **kwargs)
+ def should_redirect_to_wanna_pickup(self):
+ return (
+ self.request.membership.is_international
+ and not self.request.in_space_of_section.pickup_system_requests.filter(issuer=self.request.user).exists()
+ )
data.update(
- {"requests": self.request.in_space_of_section.pickup_system_requests.filter(issuer=self.request.user)}
+ {"requests": self.request.in_space_of_section.pickup_system_requests.select_related('issuer').filter(issuer=self.request.user)}
)
if self.request.membership.is_international:
return ["pickup_system/index_international.html"]
else:
return ["pickup_system/index_member.html"]Commitable 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.
| from __future__ import annotations | |
| from django.http import HttpResponseRedirect | |
| from django.urls import reverse | |
| from django.views.generic import TemplateView | |
| from apps.pickup_system.models import PickupRequest, PickupSystemConfiguration | |
| from apps.plugins.views import PluginConfigurationViewMixin | |
| from apps.sections.middleware.user_membership import HttpRequest | |
| from apps.sections.views.mixins.section_space import EnsureInSectionSpaceViewMixin | |
| class PickupSystemIndexView( | |
| EnsureInSectionSpaceViewMixin, | |
| PluginConfigurationViewMixin[PickupSystemConfiguration], | |
| TemplateView, | |
| ): | |
| request: HttpRequest | |
| extra_context = { | |
| "RequestState": PickupRequest.State, | |
| } | |
| def get(self, request, *args, **kwargs): | |
| if ( | |
| self.request.membership.is_international | |
| and not self.request.in_space_of_section.pickup_system_requests.filter(issuer=self.request.user).exists() | |
| ): | |
| return HttpResponseRedirect(reverse("pickup_system:wanna-pickup")) | |
| return super().get(request, *args, **kwargs) | |
| def get_context_data(self, **kwargs): | |
| data = super().get_context_data(**kwargs) | |
| data.update( | |
| {"requests": self.request.in_space_of_section.pickup_system_requests.filter(issuer=self.request.user)} | |
| ) | |
| return data | |
| def get_template_names(self): | |
| return [ | |
| ( | |
| "pickup_system/index_international.html" | |
| if self.request.membership.is_international | |
| else "pickup_system/index_member.html" | |
| ) | |
| ] | |
| from __future__ import annotations | |
| from django.http import HttpResponseRedirect | |
| from django.urls import reverse | |
| from django.views.generic import TemplateView | |
| from apps.pickup_system.models import PickupRequest, PickupSystemConfiguration | |
| from apps.plugins.views import PluginConfigurationViewMixin | |
| from apps.sections.middleware.user_membership import HttpRequest | |
| from apps.sections.views.mixins.section_space import EnsureInSectionSpaceViewMixin | |
| class PickupSystemIndexView( | |
| EnsureInSectionSpaceViewMixin, | |
| PluginConfigurationViewMixin[PickupSystemConfiguration], | |
| TemplateView, | |
| ): | |
| request: HttpRequest | |
| extra_context = { | |
| "RequestState": PickupRequest.State, | |
| } | |
| def get(self, request, *args, **kwargs): | |
| if self.should_redirect_to_wanna_pickup(): | |
| return HttpResponseRedirect(reverse("pickup_system:wanna-pickup")) | |
| return super().get(request, *args, **kwargs) | |
| def should_redirect_to_wanna_pickup(self): | |
| return ( | |
| self.request.membership.is_international | |
| and not self.request.in_space_of_section.pickup_system_requests.filter(issuer=self.request.user).exists() | |
| ) | |
| def get_context_data(self, **kwargs): | |
| data = super().get_context_data(**kwargs) | |
| data.update( | |
| {"requests": self.request.in_space_of_section.pickup_system_requests.select_related('issuer').filter(issuer=self.request.user)} | |
| ) | |
| return data | |
| def get_template_names(self): | |
| if self.request.membership.is_international: | |
| return ["pickup_system/index_international.html"] | |
| else: | |
| return ["pickup_system/index_member.html"] |
| from __future__ import annotations | ||
|
|
||
| from django.http import HttpResponseRedirect | ||
| from django.urls import reverse, reverse_lazy | ||
| from django.utils.translation import gettext_lazy as _ | ||
| from django.views.generic import TemplateView | ||
|
|
||
| from apps.fiestarequests.views.request import BaseNewRequestView | ||
| from apps.pickup_system.forms import NewPickupRequestForm | ||
| from apps.pickup_system.models import PickupSystemConfiguration | ||
| from apps.plugins.views import PluginConfigurationViewMixin | ||
| from apps.sections.views.mixins.section_space import EnsureInSectionSpaceViewMixin | ||
|
|
||
|
|
||
| class PickupSystemEntrance(EnsureInSectionSpaceViewMixin, PluginConfigurationViewMixin, TemplateView): | ||
| def get(self, request, *args, **kwargs): | ||
| if self.request.membership.is_international: | ||
| return HttpResponseRedirect(reverse("pickup_system:new-request")) | ||
|
|
||
| c: PickupSystemConfiguration = self.configuration | ||
| if c.matching_policy_instance.can_member_match: | ||
| return HttpResponseRedirect(reverse("pickup_system:matching-requests")) | ||
|
|
||
| return HttpResponseRedirect(reverse("pickup_system:index")) | ||
|
|
||
|
|
||
| class NewPickupRequestView(BaseNewRequestView): | ||
| form_class = NewPickupRequestForm | ||
| success_message = _("Your pickup request has been successfully created!") | ||
|
|
||
| success_url = reverse_lazy("pickup_system:index") |
There was a problem hiding this comment.
The code looks good overall. However, there are a few points to consider:
-
Ensure that the
reverse_lazyfunction is used correctly. It is used here to define thesuccess_urlattribute of theNewPickupRequestViewclass. This function is useful when the URL is not needed immediately, but at a later point in time, such as when the view is actually accessed. If the URL is needed immediately, consider using thereversefunction instead. -
The
EnsureInSectionSpaceViewMixinandPluginConfigurationViewMixinmixins are used in thePickupSystemEntranceclass. Make sure that these mixins are providing the expected functionality and that their methods do not conflict with each other or with the methods of thePickupSystemEntranceclass. -
In the
getmethod of thePickupSystemEntranceclass, themembershipattribute of therequestobject is accessed without checking if it exists. If there is a chance that this attribute might not exist, consider adding error handling to prevent a potentialAttributeError. -
The
matching_policy_instanceattribute of thePickupSystemConfigurationinstancecis accessed without checking if it exists. If there is a chance that this attribute might not exist, consider adding error handling to prevent a potentialAttributeError. -
The
NewPickupRequestViewclass inherits fromBaseNewRequestViewand sets theform_class,success_message, andsuccess_urlattributes. Ensure that theBaseNewRequestViewclass is designed to be subclassed in this way and that it provides the necessary functionality. -
The
NewPickupRequestFormclass is used as the form class for theNewPickupRequestViewview. Ensure that this form class provides the necessary fields and validation for creating a new pickup request. -
The
success_messageattribute of theNewPickupRequestViewclass is set to a translated string. Ensure that this string is included in the translation files for all supported languages. -
The
success_urlattribute of theNewPickupRequestViewclass is set to the URL for the "pickup_system:index" view. Ensure that this view exists and provides the expected functionality.
fiesta/apps/pickup_system/forms.py
Outdated
| class NewPickupRequestForm(BaseModelForm): | ||
| submit_text = _("Send request for pickup") | ||
|
|
||
| # TODO: group field somehow and add group headings | ||
| facebook, instagram, telegram, whatsapp = USER_PROFILE_CONTACT_FIELDS.values() | ||
|
|
||
| approving_request = BooleanField(required=True, label=_("I really want a pickup")) | ||
|
|
||
| class Meta: | ||
| model = PickupRequest | ||
| fields = ( | ||
| "note", | ||
| # "interests", | ||
| "issuer", | ||
| "issuer_faculty", | ||
| ) | ||
| field_classes = { | ||
| # "interests": ChoicedArrayField, | ||
| } | ||
| widgets = { | ||
| "issuer": HiddenInput, | ||
| "issuer_faculty": FacultyWidget, | ||
| } | ||
| labels = { | ||
| "note": _("Tell us about yourself"), | ||
| # "interests": _("What are you into?"), | ||
| "issuer_faculty": _("Your faculty"), | ||
| } | ||
| help_texts = { | ||
| "note": lazy( | ||
| lambda: render_to_string("pickup_system/parts/pickup_request_note_help.html"), | ||
| str, | ||
| ) | ||
| } | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
|
|
||
| if self.initial.get("issuer_faculty"): | ||
| self.fields["issuer_faculty"].disabled = True | ||
|
|
||
|
|
There was a problem hiding this comment.
The NewPickupRequestForm class is well-defined. It inherits from BaseModelForm and sets the model attribute to PickupRequest, which is the correct way to create a Django model form. The fields attribute is set to a tuple of field names from the PickupRequest model that should be included in the form. The field_classes and widgets attributes are used to customize the form fields, and the labels and help_texts attributes are used to set the display labels and help texts for the form fields. The __init__ method is overridden to disable the "issuer_faculty" field if it has an initial value, which is a good way to prevent users from changing this field after it has been set. However, the approving_request field is set to be required, but there is no error handling for when this field is not provided. Consider adding error handling for this case.
+ approving_request = BooleanField(required=True, label=_("I really want a pickup"), error_messages={'required': _('You must approve the request.')})Commitable 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.
| class NewPickupRequestForm(BaseModelForm): | |
| submit_text = _("Send request for pickup") | |
| # TODO: group field somehow and add group headings | |
| facebook, instagram, telegram, whatsapp = USER_PROFILE_CONTACT_FIELDS.values() | |
| approving_request = BooleanField(required=True, label=_("I really want a pickup")) | |
| class Meta: | |
| model = PickupRequest | |
| fields = ( | |
| "note", | |
| # "interests", | |
| "issuer", | |
| "issuer_faculty", | |
| ) | |
| field_classes = { | |
| # "interests": ChoicedArrayField, | |
| } | |
| widgets = { | |
| "issuer": HiddenInput, | |
| "issuer_faculty": FacultyWidget, | |
| } | |
| labels = { | |
| "note": _("Tell us about yourself"), | |
| # "interests": _("What are you into?"), | |
| "issuer_faculty": _("Your faculty"), | |
| } | |
| help_texts = { | |
| "note": lazy( | |
| lambda: render_to_string("pickup_system/parts/pickup_request_note_help.html"), | |
| str, | |
| ) | |
| } | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| if self.initial.get("issuer_faculty"): | |
| self.fields["issuer_faculty"].disabled = True | |
| class NewPickupRequestForm(BaseModelForm): | |
| submit_text = _("Send request for pickup") | |
| # TODO: group field somehow and add group headings | |
| facebook, instagram, telegram, whatsapp = USER_PROFILE_CONTACT_FIELDS.values() | |
| approving_request = BooleanField(required=True, label=_("I really want a pickup"), error_messages={'required': _('You must approve the request.')}) | |
| class Meta: | |
| model = PickupRequest | |
| fields = ( | |
| "note", | |
| # "interests", | |
| "issuer", | |
| "issuer_faculty", | |
| ) | |
| field_classes = { | |
| # "interests": ChoicedArrayField, | |
| } | |
| widgets = { | |
| "issuer": HiddenInput, | |
| "issuer_faculty": FacultyWidget, | |
| } | |
| labels = { | |
| "note": _("Tell us about yourself"), | |
| # "interests": _("What are you into?"), | |
| "issuer_faculty": _("Your faculty"), | |
| } | |
| help_texts = { | |
| "note": lazy( | |
| lambda: render_to_string("pickup_system/parts/pickup_request_note_help.html"), | |
| str, | |
| ) | |
| } | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| if self.initial.get("issuer_faculty"): | |
| self.fields["issuer_faculty"].disabled = True |
fiesta/apps/pickup_system/forms.py
Outdated
| class PickupRequestEditorForm(BaseModelForm): | ||
| submit_text = _("Save") | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
|
|
||
| self.fields["issuer"].disabled = True | ||
|
|
||
| if self.instance.state != PickupRequest.State.CREATED: | ||
| # self.fields["matched_by"].disabled = True | ||
| # self.fields["matched_at"].disabled = True | ||
| self.fields["note"].disabled = True | ||
| # self.fields["interests"].disabled = True | ||
|
|
||
| class Meta: | ||
| model = PickupRequest | ||
| fields = ( | ||
| "issuer", | ||
| "state", | ||
| "note", | ||
| # "interests", | ||
| # "matched_by", | ||
| # "matched_at", | ||
| ) | ||
| field_classes = { | ||
| # "interests": ChoicedArrayField, | ||
| # "matched_at": DateTimeLocalField, | ||
| } | ||
| widgets = { | ||
| "issuer": UserWidget, | ||
| # "matched_by": ActiveLocalMembersFromSectionWidget, | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
The PickupRequestEditorForm class is also well-defined. It inherits from BaseModelForm and sets the model attribute to PickupRequest. The fields attribute is set to a tuple of field names from the PickupRequest model that should be included in the form. The __init__ method is overridden to disable certain fields based on the state of the instance, which is a good way to prevent users from changing these fields after the request has been created. However, the "issuer" field is always disabled, which may not be desirable if you want to allow users to change the issuer of a request. Consider making this field enabled by default and only disabling it under certain conditions.
- self.fields["issuer"].disabled = True
+ if self.instance.state != PickupRequest.State.CREATED:
+ self.fields["issuer"].disabled = TrueCommitable 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.
| class PickupRequestEditorForm(BaseModelForm): | |
| submit_text = _("Save") | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| self.fields["issuer"].disabled = True | |
| if self.instance.state != PickupRequest.State.CREATED: | |
| # self.fields["matched_by"].disabled = True | |
| # self.fields["matched_at"].disabled = True | |
| self.fields["note"].disabled = True | |
| # self.fields["interests"].disabled = True | |
| class Meta: | |
| model = PickupRequest | |
| fields = ( | |
| "issuer", | |
| "state", | |
| "note", | |
| # "interests", | |
| # "matched_by", | |
| # "matched_at", | |
| ) | |
| field_classes = { | |
| # "interests": ChoicedArrayField, | |
| # "matched_at": DateTimeLocalField, | |
| } | |
| widgets = { | |
| "issuer": UserWidget, | |
| # "matched_by": ActiveLocalMembersFromSectionWidget, | |
| } | |
| class PickupRequestEditorForm(BaseModelForm): | |
| submit_text = _("Save") | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| if self.instance.state != PickupRequest.State.CREATED: | |
| self.fields["issuer"].disabled = True | |
| self.fields["note"].disabled = True | |
| class Meta: | |
| model = PickupRequest | |
| fields = ( | |
| "issuer", | |
| "state", | |
| "note", | |
| ) | |
| field_classes = { | |
| } | |
| widgets = { | |
| "issuer": UserWidget, | |
| } |
fiesta/apps/pickup_system/forms.py
Outdated
| class QuickPickupMatchForm(BaseModelForm): | ||
| submit_text = _("Match") | ||
| instance: PickupRequestMatch | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
|
|
||
| class Meta: | ||
| model = PickupRequestMatch | ||
| fields = ("matcher",) | ||
| widgets = { | ||
| "matcher": ActiveLocalMembersFromSectionWidget, | ||
| } | ||
|
|
||
| def clean_matcher(self): | ||
| matcher: User = self.cleaned_data["matcher"] | ||
|
|
||
| if not matcher.profile_or_none.faculty: | ||
| raise ValidationError(_("This user has not set their faculty. Please ask them to do so or do it yourself.")) | ||
|
|
||
| return matcher |
There was a problem hiding this comment.
The QuickPickupMatchForm class is well-defined. It inherits from BaseModelForm and sets the model attribute to PickupRequestMatch. The fields attribute is set to a tuple of field names from the PickupRequestMatch model that should be included in the form. The clean_matcher method is overridden to add custom validation for the "matcher" field, which is a good use of Django's form validation system. However, the error message for when the matcher's faculty is not set could be more specific. Consider changing the error message to indicate that the matcher's faculty needs to be set before they can be matched with a pickup request.
- raise ValidationError(_("This user has not set their faculty. Please ask them to do so or do it yourself."))
+ raise ValidationError(_("The matcher's faculty must be set before they can be matched with a pickup request. Please ask the matcher to set their faculty or do it yourself."))Commitable 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.
| class QuickPickupMatchForm(BaseModelForm): | |
| submit_text = _("Match") | |
| instance: PickupRequestMatch | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| class Meta: | |
| model = PickupRequestMatch | |
| fields = ("matcher",) | |
| widgets = { | |
| "matcher": ActiveLocalMembersFromSectionWidget, | |
| } | |
| def clean_matcher(self): | |
| matcher: User = self.cleaned_data["matcher"] | |
| if not matcher.profile_or_none.faculty: | |
| raise ValidationError(_("This user has not set their faculty. Please ask them to do so or do it yourself.")) | |
| return matcher | |
| class QuickPickupMatchForm(BaseModelForm): | |
| submit_text = _("Match") | |
| instance: PickupRequestMatch | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| class Meta: | |
| model = PickupRequestMatch | |
| fields = ("matcher",) | |
| widgets = { | |
| "matcher": ActiveLocalMembersFromSectionWidget, | |
| } | |
| def clean_matcher(self): | |
| matcher: User = self.cleaned_data["matcher"] | |
| if not matcher.profile_or_none.faculty: | |
| raise ValidationError(_("The matcher's faculty must be set before they can be matched with a pickup request. Please ask the matcher to set their faculty or do it yourself.")) | |
| return matcher |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- fiesta/apps/buddy_system/templates/buddy_system/dashboard_block.html (1 hunks)
- fiesta/apps/fiestarequests/matching_policy.py (1 hunks)
- fiesta/apps/fiestarequests/models/configuration.py (2 hunks)
- fiesta/apps/pickup_system/templates/pickup_system/dashboard_block.html (1 hunks)
Files skipped from review due to trivial changes (2)
- fiesta/apps/buddy_system/templates/buddy_system/dashboard_block.html
- fiesta/apps/fiestarequests/matching_policy.py
Additional comments: 9
fiesta/apps/fiestarequests/models/configuration.py (1)
- 3-8: The removal of
MatchingPoliciesRegisterimport and thematching_policy_instanceproperty suggests a refactoring or redesign of how matching policies are handled. Ensure that this change does not affect other parts of the application that may rely on these components.fiesta/apps/pickup_system/templates/pickup_system/dashboard_block.html (8)
6-6: The template tag
{% get_pickup_system_configuration as configuration %}is used to load the pickup system configuration. Ensure that this tag is properly defined in thepickup_systemtemplate tags and that it handles errors gracefully in case the configuration cannot be retrieved.11-35: This section handles the display logic for international members. It checks if a current pickup request exists and displays the appropriate status and actions. Ensure that the
get_current_pickup_request_of_userandget_waiting_pickup_requests_placed_beforetemplate tags are well-tested, especially for edge cases where there might be no requests or an unexpected number of waiting requests.26-32: The template assumes that
br.match.matcheris a valid object with afull_nameattribute. Ensure that theget_user_picturetemplate tag and thematchobject are robust against cases where a match might not have been found or thematcherobject is not fully populated.37-51: This section is for local members to view waiting requests that they can match with. Ensure that the
get_waiting_requests_to_matchtemplate tag correctly handles cases where there are no waiting requests. The existence check{% if waiting_brs.exists %}is good practice to avoid unnecessary rendering of elements.58-107: This block handles the display of matched pickup requests for local members. It uses several template tags such as
get_matched_pickup_requestsandget_user_picture. Ensure that these tags are thoroughly tested, especially in scenarios where there might be no matches or the user pictures are not available.74-85: The template generates a link for each matched pickup request with a profile picture. Ensure that the
serve-issuer-profile-pictureURL is secured and that it only serves images for authorized requests to prevent unauthorized access to user profile pictures.
[SECURITY]88-93: When no profile picture is available, the template generates a placeholder with the user's initials. The style is dynamically set using the
get_color_by_textfilter applied to theusername. Ensure that this filter escapes any special characters to prevent CSS injection attacks.
[SECURITY]97-102: This section provides a link to view more matched requests if there are more than three. Ensure that the arithmetic operation
|add:"-3"is safe and won't cause any template rendering issues if the number of requests is exactly three.
| display_issuer_faculty = models.BooleanField(default=False) | ||
| display_request_creation_date = models.BooleanField(default=True) | ||
|
|
||
| rolling_limit = models.PositiveSmallIntegerField(default=0) | ||
|
|
||
| matching_policy = models.CharField( | ||
| default=MatchingPoliciesRegister.DEFAULT_POLICY.id, | ||
| choices=MatchingPoliciesRegister.CHOICES, | ||
| max_length=32, | ||
| help_text=MatchingPoliciesRegister.DESCRIPTION, | ||
| ) | ||
| # TODO: make it visible and working | ||
| rolling_limit = models.PositiveSmallIntegerField(default=0, editable=False) | ||
|
|
||
| enable_note_from_matcher = models.BooleanField( | ||
| default=True, | ||
| help_text=_("Allows matcher to reply with custom notes to the request issuer"), | ||
| ) | ||
|
|
||
| @property | ||
| def matching_policy_instance(self): | ||
| # TODO: pass configuration? | ||
| return MatchingPoliciesRegister.get_policy_by_id(self.matching_policy) | ||
|
|
||
| class Meta: | ||
| abstract = True |
There was a problem hiding this comment.
The rolling_limit field has been made non-editable, which implies it cannot be changed via Django admin or forms. If this field is supposed to be managed only through code or migrations, this change is appropriate. However, if administrators should be able to edit this value, setting editable=False would be a mistake. The TODO comment suggests that there might be future work planned to make this field visible and functional in the admin interface. It's important to track this TODO to ensure it gets addressed.
| <div class="Dashboard__tile__value">⌛ Waiting for match</div> | ||
| {% get_waiting_pickup_requests_placed_before br as waiting_total %} | ||
| <div class="Dashboard__tile__desc text-base-200"> | ||
| There is {{ waiting_total }} waiting request{{ waiting_total|pluralize:"s" }} before yours. |
There was a problem hiding this comment.
The use of {{ waiting_total }} directly in the template assumes that the value will always be an integer. If there's any chance that waiting_total could be None or another non-integer value, this could cause a display issue. It might be beneficial to add a default filter or handle this case in the template tag itself.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- fiesta/apps/fiestarequests/matching_policy.py (3 hunks)
Additional comments: 3
fiesta/apps/fiestarequests/matching_policy.py (3)
60-65: Renaming of
SameFacultyMatchingPolicytoManualWithSameFacultyMatchingPolicyand updating thetitleattribute is consistent with the intent to clarify the policy's operation mode. This change should be cross-checked to ensure that all references to the old class name are updated throughout the codebase.69-73: The change in the
limit_requestsmethod to filter byissuer_facultyinstead ofissuer__profile__facultysimplifies the query. However, it's important to ensure that theissuer_facultyfield exists on theBuddyRequestmodel and that it is correctly populated with the faculty of the issuer. If theissuer_facultyfield does not exist, this change will cause a runtime error.97-104: The update to the
AVAILABLE_POLICIESlist in theMatchingPoliciesRegisterclass reflects the renaming of the policy. It's important to verify that the commented-out policies (LimitedSameFacultyMatchingPolicy,AutoMatchingPolicy) are intentionally left out and that there's a plan to implement or remove them in the future. Leaving TODO comments is a good practice, but they should be accompanied by a ticket or an issue in the project's tracking system to ensure they are addressed.
thejoeejoee
force-pushed
the
feat-pickup-system
branch
from
f529c35
to
6600be6
Compare
| location <span class="text-3xl">🗺️</span> | ||
| </th> | ||
| <td> | ||
| <a class="link link-secondary" |
Check warning
Code scanning / CodeQL
Potentially unsafe external link Medium
Summary by CodeRabbit