Skip to content
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

Track Task-related activities #1810

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Track Task-related activities #1810

wants to merge 9 commits into from

Conversation

jerivas
Copy link
Member

@jerivas jerivas commented Dec 14, 2021

Steps to test/reproduce

  • Perform actions on a Task (assignee changes, scratch org creation, reviews, etc)
  • Visit /api/task-activities/. This endpoint can be filtered by ?task=:id
  • Notice a TaskActivity instance is created for each action being performed
  • Each time an activity is logged a TASK_ACTIVITY_LOGGED event will be dispatched in the Task's websocket channel

Note: you will need to configure GitHub webhooks as explained in CONTRIBUTING.rst for the "Submit pull request" and "Merge pull request" events to be tracked propertly.

Show me

image

@jerivas
Copy link
Member Author

jerivas commented Dec 14, 2021

Copy link
Member Author

@jerivas jerivas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgerigmeyer I'm working on tests and the API endpoints now, but the feature is functionally ready for review (Task history is viewable via the admin)

Comment on lines +104 to +110
pr = self.validated_data["pull_request"]
if self._is_opened():
instance.finalize_pr_opened(pr_number, originating_user_id=None)
instance.finalize_pr_opened(pr, originating_user_id=None)
elif self._is_merged():
instance.finalize_status_completed(pr_number, originating_user_id=None)
instance.finalize_status_completed(pr, originating_user_id=None)
else:
instance.finalize_pr_closed(pr_number, originating_user_id=None)
instance.finalize_pr_closed(pr, originating_user_id=None)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now pass a full pr object to the finalize_* methods to have access to both the title and number (and other things if we ever need them in the future).

Comment on lines +95 to +98
TESTER_ACCEPTED = "Tester accepted"
TESTER_ASSIGNED = "Tester assigned"
TESTER_UNASSIGNED = "Tester unassigned"
DEV_ACCEPTED = "Developer accepted"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TESTER_ACCEPTED and DEV_ACCEPTED are not used yet but they are in place for the future invitation system

Comment on lines +772 to +790
@property
def branch_url(self) -> Optional[str]:
if self.branch_name:
return f"{self.root_project.repo_url}/tree/{self.branch_name}"
return None

@property
def branch_diff_url(self) -> Optional[str]:
base_branch = self.get_base()
branch = self.branch_name
if base_branch and branch:
return f"{self.root_project.repo_url}/compare/{base_branch}...{branch}"
return None

@property
def pr_url(self) -> Optional[str]:
if self.pr_number:
return f"{self.root_project.repo_url}/pull/{self.pr_number}"
return None
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These properties were previously defined in the serializer but we need them to log Task activity so they were moved to the model

@jerivas
Copy link
Member Author

jerivas commented Dec 16, 2021

@jgerigmeyer I'm undecided on what's the best way to expose the TaskActivities to the frontend. We can add an activities field to TaskSerializer but it seems excessive to fetch the activities on the Task list view. Perhaps we should split the list and detail Task serializers? This would fetch us fresh activities any time we emit a websocket event for a Task update.

Another option is to give the activities their own endpoint and filter by Task. That seems cleaner but we would then need a mechanism to handle list-based WS subscriptions (drf-live, which we haven't installed on Metecho yet).

@jgerigmeyer
Copy link
Member

@jgerigmeyer I'm undecided on what's the best way to expose the TaskActivities to the frontend. We can add an activities field to TaskSerializer but it seems excessive to fetch the activities on the Task list view. Perhaps we should split the list and detail Task serializers? This would fetch us fresh activities any time we emit a websocket event for a Task update.

Another option is to give the activities their own endpoint and filter by Task. That seems cleaner but we would then need a mechanism to handle list-based WS subscriptions (drf-live, which we haven't installed on Metecho yet).

@jerivas I think I'd lean toward a separate endpoint. With our current setup, those WS events could be fired on the task-detail channel anyway, and if/when we switch to drf-live we could use their list channel.

But I don't think we'll get to the front-end for this story until February at the earliest (given other priorities and limited budget), so this is probably good enough for now and we can resolve it then?

@jerivas
Copy link
Member Author

jerivas commented Feb 15, 2022

@jgerigmeyer it looks like we're clear to start working on this again. I still need to fix merge conflicts and add endpoints for the new TaskActivity model before JS work can start. From your last response it looks like they should have their own endpoint, be filterable by task, and receive WS messages on their associated Task's channel. Is that still correct?

@jerivas jerivas removed the on hold label Feb 15, 2022
@jgerigmeyer
Copy link
Member

@jgerigmeyer it looks like we're clear to start working on this again. I still need to fix merge conflicts and add endpoints for the new TaskActivity model before JS work can start. From your last response it looks like they should have their own endpoint, be filterable by task, and receive WS messages on their associated Task's channel. Is that still correct?

@jerivas Yes, that sounds right to me!

@jerivas jerivas marked this pull request as ready for review February 18, 2022 18:13
@jerivas
Copy link
Member Author

jerivas commented Feb 18, 2022

@jgerigmeyer I added the endpoints and WS events for TaskActivity instances. The instructions on the PR description have been updated accordingly

@jgerigmeyer jgerigmeyer marked this pull request as draft July 1, 2022 15:51
@jgerigmeyer jgerigmeyer marked this pull request as draft July 1, 2022 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants