-
Notifications
You must be signed in to change notification settings - Fork 23
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
Configure video settings for talks #288
Configure video settings for talks #288
Conversation
…eo into setup-attendee-role
Reviewer's Guide by SourceryThis PR implements video settings configuration for talks by introducing a new task system that handles token generation and integration with Eventyay. The implementation includes JWT token generation for video access, trait grants configuration, and an asynchronous task to configure video settings. Class diagram for video settings configurationclassDiagram
class World {
+String domain
+String locale
+String timezone
+Map config
+Map trait_grants
+save()
}
class ShortToken {
+World world
+String long_token
+DateTime expires
+String short_token
}
class Task {
+configure_video_settings_for_talks(world_id, days, number, traits, long)
+generate_video_token(world, days, number, traits, long)
+generate_talk_token(video_settings, video_tokens, event_slug)
}
World --> ShortToken : generates
Task --> World : uses
Task --> ShortToken : creates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…configure-video-settings-for-talks
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 @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- The generate_video_token function should check if JWT_secrets[0] exists before accessing it to avoid potential IndexError when JWT_secrets is an empty list
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 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.
logger = logging.getLogger(__name__) | ||
|
||
|
||
def generate_video_token(world, days, number, traits, long=False): |
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 introducing a TokenService class to encapsulate token generation and management functionality.
The token generation logic can be simplified by introducing a dedicated TokenService class to handle token creation and reduce parameter passing. This separates concerns and makes the code more maintainable. Here's a suggested refactor:
class TokenService:
def __init__(self, jwt_config):
self.secret = jwt_config['secret']
self.audience = jwt_config['audience']
self.issuer = jwt_config['issuer']
self.now = datetime.datetime.utcnow()
def create_video_token(self, traits, expiry_days):
return {
"iss": self.issuer,
"aud": self.audience,
"exp": self.now + datetime.timedelta(days=expiry_days),
"iat": self.now,
"uid": str(uuid.uuid4()),
"traits": traits,
}
def create_talk_token(self, video_tokens, event_slug):
return {
"exp": self.now + datetime.timedelta(days=30),
"iat": self.now,
"video_tokens": video_tokens,
"slug": event_slug,
}
def encode_token(self, payload):
return jwt.encode(payload, self.secret, algorithm="HS256")
Usage in the task:
@shared_task(bind=True, max_retries=5, default_retry_delay=60)
def configure_video_settings_for_talks(self, world_id, days, number, traits, long=False):
world = World.objects.get(id=world_id)
jwt_config = world.config.get("JWT_secrets", [])[0]
token_service = TokenService(jwt_config)
video_tokens = []
bulk_tokens = []
for _ in range(number):
payload = token_service.create_video_token(traits, days)
token = token_service.encode_token(payload)
if long:
video_tokens.append(token)
else:
st = ShortToken(world=world, long_token=token,
expires=payload['exp'])
video_tokens.append(st.short_token)
bulk_tokens.append(st)
if bulk_tokens:
ShortToken.objects.bulk_create(bulk_tokens)
talk_payload = token_service.create_talk_token(video_tokens, world_id)
talk_token = token_service.encode_token(talk_payload)
# ... rest of the API call code
This refactor:
- Consolidates token generation logic in one place
- Eliminates duplicate datetime handling
- Reduces parameter passing
- Makes the code more testable
- Keeps the important error handling intact
This PR resolves fossasia/eventyay-tickets#465