Add Checkin API #389
Open
Add Checkin API #389
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
Reviewer's Guide by SourceryThis pull request adds a new CheckinAPI to the eventyay-tickets component, which can be used by open-event-checkin to perform check-ins. The implementation includes new views, serializers, and modifications to existing models to support the check-in functionality. Sequence diagram for Checkin API Redeem ProcesssequenceDiagram
actor User
participant CheckinRPCRedeemView
participant _redeem_process
participant CheckinListOrderPositionSerializer
participant perform_checkin
User->>CheckinRPCRedeemView: POST /checkinrpc/redeem
CheckinRPCRedeemView->>CheckinRPCRedeemInputSerializer: Validate request data
CheckinRPCRedeemInputSerializer-->>CheckinRPCRedeemView: Validated data
CheckinRPCRedeemView->>_redeem_process: Call with validated data
_redeem_process->>CheckinListOrderPositionSerializer: Serialize order position
CheckinListOrderPositionSerializer-->>_redeem_process: Serialized data
_redeem_process->>perform_checkin: Perform check-in
perform_checkin-->>_redeem_process: Check-in result
_redeem_process-->>CheckinRPCRedeemView: Response data
CheckinRPCRedeemView-->>User: Response
Class diagram for Checkin API ComponentsclassDiagram
class CheckinRPCRedeemView {
+post(request, *args, **kwargs)
}
class CheckinRPCSearchView {
+get_queryset(ignore_status, ignore_products)
}
class CheckinRPCRedeemInputSerializer {
+lists
+secret
+force
+source_type
+type
+ignore_unpaid
+questions_supported
+nonce
+datetime
+answers
}
class MiniCheckinListSerializer {
+event
+subevent
}
class BaseMediaType {
+identifier
+verbose_name
+generate_identifier(organizer)
+is_active(organizer)
}
class BarcodePlainMediaType {
+generate_identifier(organizer)
}
class NfcUidMediaType {
+handle_unknown(organizer, identifier, user, auth)
}
class NfcMf0aesMediaType {
+handle_new(organizer, medium, user, auth)
}
CheckinRPCRedeemView --> CheckinRPCRedeemInputSerializer
CheckinRPCSearchView --> MiniCheckinListSerializer
BaseMediaType <|-- BarcodePlainMediaType
BaseMediaType <|-- NfcUidMediaType
BaseMediaType <|-- NfcMf0aesMediaType
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Sak1012 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider refactoring some of the longer functions, particularly
_redeem_process, to improve readability and maintainability. - The new media system in
media.pyis a great addition. Consider adding some documentation or comments explaining how to extend it with new media types in the future.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| checkin_type=serializer.validated_data['type'], | ||
| ignore_unpaid=serializer.validated_data['ignore_unpaid'], | ||
| nonce=serializer.validated_data.get('nonce'), | ||
| untrusted_input=True, |
There was a problem hiding this comment.
🚨 issue (security): Review security implications of untrusted_input parameter
The untrusted_input parameter in CheckinRPCRedeemView could have security implications. Ensure that proper input validation and sanitization are in place to mitigate potential risks.
| @@ -2047,6 +2047,12 @@ class Meta: | |||
| def sort_key(self): | |||
| return self.addon_to.positionid if self.addon_to else self.positionid, self.addon_to_id or 0 | |||
|
|
|||
| @cached_property | |||
There was a problem hiding this comment.
suggestion (performance): Review usage of cached_property
While cached_property can improve performance, be cautious of overuse. Consider the implications on memory usage, especially for properties that might change frequently or consume significant memory.
Suggested change
| @cached_property | |
| @property |
|
|
||
| return cf.file | ||
|
|
||
| def _redeem_process(*, checkinlists, raw_barcode, answers_data, datetime, force, checkin_type, ignore_unpaid, nonce, |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the _redeem_process function to improve maintainability and readability.
The _redeem_process function is overly complex and violates the Single Responsibility Principle. To improve its maintainability and readability, consider the following refactoring suggestions:
- Extract file upload handling into a separate function:
def handle_file_upload(answers_data, user, auth):
uploaded_files = {}
for q_id, file_data in answers_data.items():
if file_data.startswith("file:"):
uploaded_files[q_id] = _handle_file_upload(file_data, user, auth)
return uploaded_files- Extract answer validation into a separate function:
def validate_answers(op, answers_data, uploaded_files):
given_answers = {}
for q in op.item.questions.filter(ask_during_checkin=True):
if str(q.pk) in answers_data:
try:
if q.type == Question.TYPE_FILE:
given_answers[q] = uploaded_files.get(str(q.pk))
else:
given_answers[q] = q.clean_answer(answers_data[str(q.pk)])
except BaseValidationError:
pass
return given_answers- Use a custom error handling decorator to reduce boilerplate:
def handle_checkin_errors(func):
@wraps(func)
def wrapper(*args, **kwargs):
try:
return func(*args, **kwargs)
except RequiredQuestionsError as e:
return Response({
'status': 'incomplete',
'require_attention': op.require_checkin_attention,
'position': CheckinListOrderPositionSerializer(op, context=_make_context(context, op.order.event)).data,
'questions': [QuestionSerializer(q).data for q in e.questions],
'list': MiniCheckinListSerializer(list_by_event[op.order.event_id]).data,
}, status=400)
except CheckInError as e:
if not simulate:
# Log action and create Checkin object (existing code)
return Response({
'status': 'error',
'reason': e.code,
'reason_explanation': e.reason,
'require_attention': op.require_checkin_attention,
'position': CheckinListOrderPositionSerializer(op, context=_make_context(context, op.order.event)).data,
'list': MiniCheckinListSerializer(list_by_event[op.order.event_id]).data,
}, status=400)
return wrapper- Refactor the main function to use these new helpers:
@handle_checkin_errors
def _redeem_process(*args, **kwargs):
# ... (existing parameter unpacking)
uploaded_files = handle_file_upload(answers_data, user, auth)
op = _get_order_position(raw_barcode, checkinlists)
given_answers = validate_answers(op, answers_data, uploaded_files)
perform_checkin(
op=op,
clist=list_by_event[op.order.event_id],
given_answers=given_answers,
force=force,
ignore_unpaid=ignore_unpaid,
nonce=nonce,
datetime=datetime,
questions_supported=questions_supported,
canceled_supported=canceled_supported,
user=user,
auth=auth,
type=checkin_type,
)
return Response({
'status': 'ok',
'require_attention': op.require_checkin_attention,
'position': CheckinListOrderPositionSerializer(op, context=_make_context(context, op.order.event)).data,
'list': MiniCheckinListSerializer(list_by_event[op.order.event_id]).data,
}, status=201)These changes will significantly reduce the complexity of the _redeem_process function, making it more maintainable and easier to understand. Each extracted function has a single responsibility, improving the overall structure of the code.
There was a problem hiding this comment.
Please refactor as sourcery-ai suggested.
hongquan
requested changes
Oct 11, 2024
|
|
||
| op_candidates = list(queryset.filter(q)) #filtering queryset with q to fetch orderposition | ||
|
|
||
| print('\n',op_candidates,'\n') |
| q |= Q(pk=raw_barcode) | ||
|
|
||
|
|
||
| op_candidates = list(queryset.filter(q)) #filtering queryset with q to fetch orderposition |
There was a problem hiding this comment.
Don't know why you have to load many rows and then, use only the first item
op = op_candidates[0]You should use first or earliest method on queryset.
|
|
||
| return cf.file | ||
|
|
||
| def _redeem_process(*, checkinlists, raw_barcode, answers_data, datetime, force, checkin_type, ignore_unpaid, nonce, |
There was a problem hiding this comment.
Don't use parameter name which looks like a type in standard lib (datetime). It is confusing.
|
|
||
| return cf.file | ||
|
|
||
| def _redeem_process(*, checkinlists, raw_barcode, answers_data, datetime, force, checkin_type, ignore_unpaid, nonce, |
There was a problem hiding this comment.
Please refactor as sourcery-ai suggested.
hongquan
reviewed
Oct 11, 2024
| @@ -2047,6 +2047,12 @@ class Meta: | |||
| def sort_key(self): | |||
| return self.addon_to.positionid if self.addon_to else self.positionid, self.addon_to_id or 0 | |||
|
|
|||
| @cached_property | |||
| def require_checkin_attention(self): | |||
| if self.order.checkin_attention or self.item.checkin_attention or (self.variation_id and self.variation.checkin_attention): | |||
There was a problem hiding this comment.
Why not just
return (self.order....)
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 PR adds CheckinAPI to the eventyay-tickets component that can be used by open-event-checkin to perform checkin
Example Request
Request body
{ "secret": "r7zxu7b65p38929a4da25hr6u39e29uu", "source_type": "barcode", "lists":["1","2"], "force": false, "ignore_unpaid": false, "nonce": "Pvrk50vUzQd0DhdpNRL4I4OcXsvg70uA", "datetime": null, "questions_supported": false }Response
{ "status": "ok", "require_attention": false, "position": { "id": 19, "order": "H0ZCF", "positionid": 1, "item": 3, "variation": null, "price": "0.00", "attendee_name": "Tony Stark", "attendee_name_parts": { "_scheme": "given_family", "given_name": "Tony", "family_name": "Stark" }, "company": null, "street": null, "zipcode": null, "city": null, "country": null, "state": null, "attendee_email": null, "voucher": null, "tax_rate": "0.00", "tax_value": "0.00", "secret": "r7zxu7b65p38929a4da25hr6u39e29uu", "addon_to": null, "subevent": null, "checkins": [ { "id": 1, "datetime": "2024-10-09T13:35:50.705298Z", "list": 2, "auto_checked_in": false, "type": "entry" } ], "downloads": [ { "output": "pdf", "url": "http://localhost:8000/api/v1/organizers/admin/events/bc102/orderpositions/19/download/pdf/" } ], "answers": [], "tax_rule": null, "pseudonymization_id": "P73AWLUZKN", "seat": null, "require_attention": false, "order__status": "p" }, "list": { "id": 2, "name": "Default", "event": "bc102", "subevent": null, "include_pending": false } }Summary by Sourcery
Add a new Checkin API to facilitate check-in operations for events, supporting various media types and enhancing the check-in process with new serializers and validation mechanisms.
New Features:
Enhancements: