Setup attendee role and Configure video settings for talks #285
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 SourceryThe PR implements attendee role functionality by modifying the world creation/update logic to include trait grants, which define role-based permissions. The implementation adds a new attendee role with configurable trait grants alongside existing admin and scheduleuser roles. Sequence diagram for world creation/update with attendee rolesequenceDiagram
participant User
participant API as API Endpoint
participant World
User->>API: POST request with world data
API->>World: Check if world exists
alt World exists
World->>World: Update domain, locale, timezone, trait_grants
else World does not exist
World->>World: Create new world with domain, locale, timezone, config, trait_grants
end
World-->>API: Response with world details
API-->>User: Return response
Class diagram for World model with trait grantsclassDiagram
class World {
String domain
String locale
String timezone
String config
Map trait_grants
}
note for World "The trait_grants attribute is added to manage role-based permissions."
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 @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the trait_grants dictionary into a separate function or constant to avoid duplication between the update and create paths
- Add validation for attendee_trait_grants input to ensure security of permission assignments
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.
hongquan
requested changes
Dec 5, 2024
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the trait_grants initialization to avoid duplication in both the update and create paths
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
odkhang
force-pushed
the
setup-attendee-role
branch
from
1aa1179 to
4278dee
Compare
odkhang
changed the title
hongquan
requested changes
Dec 17, 2024
server/venueless/api/task.py
Outdated
| world.config["pretalx"] = { | ||
| "event": event_slug, | ||
| "domain": "{}".format(settings.EVENTYAY_TALK_BASE_PATH), | ||
| "pushed": datetime.datetime.now().isoformat(), |
There was a problem hiding this comment.
Please always use timezone-aware datetime. Calling now without tz will create naive datetime.
server/venueless/api/task.py
Outdated
| } | ||
| world.save() | ||
| except requests.exceptions.ConnectionError as e: | ||
| logger.error("Connection error: %s", str(e)) |
There was a problem hiding this comment.
You don't need to wrap e with str. When the log message is to be built, the % formatter will tell logging to "stringify" the passed arguments.
Wrapping str here is even undesired, it makes "stringify" operation run before we need to build the log message.
server/venueless/api/task.py
Outdated
| logger.error("Connection error: %s", str(e)) | ||
| self.retry(exc=e) | ||
| except requests.exceptions.HTTPError as e: | ||
| if e.response.status_code in (401, 403, 404): |
There was a problem hiding this comment.
Use enum from http module, don't use hardcode 401, etc...
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 pull request resolves fossasia/eventyay-tickets#465.