-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add Checkin API #389
base: development
Are you sure you want to change the base?
Add Checkin API #389
Conversation
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.py
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refactor as sourcery-ai suggested.
|
||
op_candidates = list(queryset.filter(q)) #filtering queryset with q to fetch orderposition | ||
|
||
print('\n',op_candidates,'\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use logging, don't use print.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refactor as sourcery-ai suggested.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just
return (self.order....)
This PR adds CheckinAPI to the eventyay-tickets component that can be used by open-event-checkin to perform checkin
Example Request
Request body
Response
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: