-
Notifications
You must be signed in to change notification settings - Fork 153
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
checks: add check tab #3004
base: master
Are you sure you want to change the base?
checks: add check tab #3004
Conversation
@@ -179,9 +217,12 @@ def user_dashboard_request_view(request, **kwargs): | |||
|
|||
if has_record_topic: | |||
topic = _resolve_topic_record(request) | |||
record_ui = topic["record_ui"] # None when draft | |||
record = topic["record"] # None when draft |
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.
The 4 # None when draft
comments were not true, so they are removed.
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.
are the checks also added to community view? this addition will display only user dashboard ones if I am not mistaken
I saw it later below
# TODO communities can have mulitiple configs which we should collate | ||
# TODO check for parent community too |
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.
These 2 TODOs will be implemented before going to production.
return None | ||
|
||
checks = ( | ||
CheckRun.query.filter( |
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.
I think this part should be moved to a service, since it does not follow our usual layered architecture
In addition, if I understand this correctly you are adding db request for every request - in case of community invitation and other requests types
is_draft = record_ui["is_draft"] if record_ui else False | ||
community_id = request["receiver"]["community"] |
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.
will all requests will record topic have community as a receiver? In this line you expect it, but I think it might break for some requests, or some other ones added in the future that receiver won't be a community
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.
If I remember correctly, guest access request to a record has the user as receiver
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.
Higher up in the same file we do:
request_type = request["type"]
is_record_inclusion = request_type == CommunityInclusion.type_id
try:
if is_record_inclusion:
community = request["receiver"]["community"]
I will do the same: check the type of the request before getting the community.
❤️ Thank you for your contribution!
Description
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: