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

Fix issues #22

Closed

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Sep 15, 2024

Resolves #21

Below is the activity logged after lastCheck.

old results from 110

[
  {
    event: 'review_requested',
    created_at: '2024-09-15T00:21:13Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'review_requested',
    created_at: '2024-09-14T05:58:45Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'review_requested',
    created_at: '2024-09-14T05:58:44Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'review_requested',
    created_at: '2024-09-14T05:58:44Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'ready_for_review',
    created_at: '2024-09-14T05:57:21Z',
    actor: 'gentlementlegen'
  }
]

new results:

[
  {
    event: 'review_requested',
    created_at: '2024-09-15T00:21:13Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-15T00:16:33Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'review_requested',
    created_at: '2024-09-14T05:58:45Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'review_requested',
    created_at: '2024-09-14T05:58:44Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'review_requested',
    created_at: '2024-09-14T05:58:44Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'ready_for_review',
    created_at: '2024-09-14T05:57:21Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-14T05:55:14Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-14T05:50:43Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-14T05:20:33Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-13T20:57:37Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-13T20:48:44Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-13T19:04:53Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-13T16:35:06Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-13T16:12:54Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-13T16:11:02Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-13T16:06:00Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-13T16:00:00Z',
    actor: 'gentlementlegen'
  },
  {
    event: 'committed',
    created_at: '2024-09-13T15:53:56Z',
    actor: 'gentlementlegen'
  }
]

Copy link
Contributor

github-actions bot commented Sep 15, 2024

Unused types (1)

Filename types
src/types/github-types.ts ListCommentsForIssue

src/helpers/get-assignee-activity.ts Outdated Show resolved Hide resolved
src/helpers/task-deadline.ts Outdated Show resolved Hide resolved
Copy link
Member

@gentlementlegen gentlementlegen left a comment

Choose a reason for hiding this comment

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

Could you please update README.md since I think the configuration changed?

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 19, 2024

@gentlementlegen tests added and issue resolved

@gentlementlegen
Copy link
Member

The configuration is fixed, but I am a bit lost with the current behavior of that plugin after testing it. I do not know if that is introduced by the changes of this pull-request, but look at this run:

  • I just started a task by assigning myself
  • the deadline according to the logs is in the past
  • I get unassigned right away

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 19, 2024

So because you used the /start command last month, then just now assigned via UI self-assign, it has taken the deadline from the what I assume is the comment as opposed to your self-assignment event.

{
  "now": "Sep 19, 2024, 3:09 PM",
  "reminder": "Aug 22, 2024, 9:37 AM",
  "deadline": "Aug 22, 2024, 9:38 AM",
  "caller": "updateTaskReminder"
}

So what's the solution? Fix the .sort() usage.

Do we as an org permit contributors to self-assign? It bypasses checks in other plugins.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 19, 2024

Likely myself who introduced it @gentlementlegen. Realized sort was backwards, but your example highlighted the fact we should use the timeline over comments if the timeline is later than the lastCheck so I implemented that and updated tests

image

@gentlementlegen
Copy link
Member

gentlementlegen commented Sep 20, 2024

@Keyrxng I pulled your latest changes and ran again, the behavior was similar

Do we as an org permit contributors to self-assign? It bypasses checks in other plugins.

This applies only to members but we could potentially be un-assigned for no reason the way it is happening to me right now. Can't we only based the checks to use the timeline events and check the last time a user was assigned?

Related run: https://github.com/Meniole/user-activity-watcher/actions/runs/10952491105/job/30411260264#step:5:31

@0x4007
Copy link
Member

0x4007 commented Sep 21, 2024

Ignore the deadline.

Unassignment only should happen after 7 days of no activity (configurable)

@@ -43,4 +43,9 @@ yarn test
optOut:
- "repoName"
- "repoName2"
eventWhitelist: # these are the tail of the webhook event i.e pull_request.review_requested
Copy link
Member

Choose a reason for hiding this comment

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

Should just use the full identifier. Shortening just will lead to confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the config would take the full pull_request.review_requested and I'll handle string splitting, is that what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like this was ever updated in the readme.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this was never fixed. Should use full webhook names.

it("Should correctly transform the eventWhitelist", () => {

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 24, 2024

QA: https://github.com/ubq-testing/user-activity-watcher/actions/runs/11016118749/job/30590832730

Can't we only based the checks to use the timeline events and check the last time a user was assigned?

I got rid of the lastCheck and any comment related logic. So we just use the start event and timeline activity.

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

It's not clear to me if you're ignoring the deadline as I requested, and if you are actually checking the assign event on the timeline, instead of checking the metadata from the /start response.

src/helpers/task-update.ts Show resolved Hide resolved
@DeFi-crypto
Copy link

This looks really interesting. I'm trying to try the plugin currently and it seems to be working fine.

Comment on lines +110 to +114
["review_requested", "pull_request.review_requested"],
["ready_for_review", "pull_request.ready_for_review"],
["commented", "pull_request_review_comment.created"],
["commented", "issue_comment.created"],
["committed", "push"],
Copy link
Member

Choose a reason for hiding this comment

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

Are these defaults?

@@ -43,4 +43,9 @@ yarn test
optOut:
- "repoName"
- "repoName2"
eventWhitelist: # these are the tail of the webhook event i.e pull_request.review_requested
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like this was ever updated in the readme.

metadata.startPlusLabelDuration =
DateTime.fromISO(mostRecentAssignmentEvent?.created_at || issue.created_at)
.plus({ milliseconds: durationInMs })
.toISO() || "";
Copy link
Member

Choose a reason for hiding this comment

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

Setting the duration to an empty string looks wrong.

@0x4007 0x4007 mentioned this pull request Oct 10, 2024
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Can address remaining problems later. The plugin is quite dysfunctional in production and I think this will get most of the essential functionality back up.

Unfortunately due to merge conflicts this won't be able to be merged.

@0x4007 0x4007 mentioned this pull request Oct 10, 2024
@0x4007 0x4007 closed this in #30 Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not unassign users that have activity on a pull-request
4 participants