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

feat: properly handle task limits #67

Conversation

jordan-ae
Copy link
Contributor

@jordan-ae jordan-ae commented Oct 25, 2024

Resolves #28

This PR updates the conditions for handling task limits

@0x4007
Copy link
Member

0x4007 commented Oct 26, 2024

@zugdev perhaps you can help test and review this since you seem the most affected

@zugdev
Copy link

zugdev commented Oct 26, 2024

@zugdev perhaps you can help test and review this since you seem the most affected

sure! @jordan-ae do you have a bot running this fix?

@jordan-ae
Copy link
Contributor Author

sure! @jordan-ae do you have a bot running this fix?

I'll add a QA link to the issue preview soon. But to test this locally you'll have to set the ubiquity-os-kernel up, clone and setup the command-start-stop repo. Then you can test this locally

Copy link

@jordan-ae, this task has been idle for a while. Please provide an update.

@jordan-ae
Copy link
Contributor Author

@0x4007 I've added a QA. Looks like everyone is really busy but the changes here are really minimal should be pretty straight forward to review hopefully

@0x4007
Copy link
Member

0x4007 commented Oct 31, 2024

@whilefoo @gentlementlegen @zugdev can you test this

@zugdev
Copy link

zugdev commented Nov 1, 2024

With your QA you should've demonstrated that if a PR is waiting for review the active issue count of the user is aliviated. That's so he can start more issues while his PRs are pending review. I'll try to QA it myself tomorrow or the day after that.

@gentlementlegen
Copy link
Member

What should also be tested is the +1 task available after the threshold for reviews is due.

@jordan-ae
Copy link
Contributor Author

jordan-ae commented Nov 1, 2024

@zugdev & @gentlementlegen not sure if I linked it poorly but that's what the QA demonstrates. The user's initial task limit is 2. He self assigns the two task and can't asign himself to task 3 until an approved pr is opened against one of his current task. Once a pr is opened and has an approval +1 task is added to his task limit.

@gentlementlegen
Copy link
Member

I tested with your changes and here is what I get:

Finally the assigment gets denied with the following values

        ⚠ You have reached your max task limit
        ⚠ {
            "assignedIssues": 1,
            "openedPullRequests": 0,
            "limit": 1,
            "caller": "handleTaskLimitChecks"
          }

It seems the openedPullRequest is wrong.

Copy link

@jordan-ae, this task has been idle for a while. Please provide an update.

@jordan-ae
Copy link
Contributor Author

Screenshot from 2024-11-03 17-11-02

@gentlementlegen I've been facing this error on /start when testing. It's trying to check pull request that do not exist I've cleared all existing changes and still get the same error. Do you have any idea what might be causing the error?

@gentlementlegen
Copy link
Member

@jordan-ae never happened to me before. Have you changed your code for testing and forgot to revert it? It should be fetching the pull-requests from GitHub API so there cannot be items that do not exist there.

@jordan-ae
Copy link
Contributor Author

Thought so too. But I've reverted all the changes, same Issue. So I recloned the repo, same issue still.

@gentlementlegen
Copy link
Member

Does this reference a private repository? Otherwise there could be a bug then you should investigate why this repository gets listed if it doesn't exist.

Copy link
Member

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

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

I don't think fully implements the specification in which I stated that after the review changes are resolved or the reviewer was requested again, it should be counted as completed

@jordan-ae
Copy link
Contributor Author

I don't think fully implements the specification in which I stated that after the review changes are resolved or the reviewer was requested again, it should be counted as completed

Missed that when reading the spec. Will add Asap

@jordan-ae jordan-ae requested a review from whilefoo November 5, 2024 12:36
src/utils/issue.ts Outdated Show resolved Hide resolved
Comment on lines 163 to 165
const adjustedAssignedIssues = assignedIssues.length - approved.length + changes.length;

// check for max and enforce max
if (Math.abs(assignedIssues.length - openedPullRequests.length) >= limit) {
if (Math.abs(adjustedAssignedIssues) >= limit) {
Copy link
Member

Choose a reason for hiding this comment

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

if I have 1 issue and 1 PR with requested changes, it will count as 2 here but it should only count 1

src/utils/issue.ts Show resolved Hide resolved
src/utils/issue.ts Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Nov 6, 2024

Unused types (1)

Filename types
src/types/payload.ts Review

@jordan-ae jordan-ae requested a review from whilefoo November 6, 2024 12:33
@jordan-ae jordan-ae force-pushed the task-limit-adjustments branch from 39577e4 to fadfb4e Compare November 6, 2024 17:31
src/utils/issue.ts Outdated Show resolved Hide resolved
src/utils/issue.ts Outdated Show resolved Hide resolved
@jordan-ae
Copy link
Contributor Author

Feature works now but I'm having a hard time writing a handler for this function to make the test pass

@gentlementlegen
Copy link
Member

The first issue in the tests is

HttpError: API rate limit exceeded for 52.238.24.178. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.) - https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting

make sure a token is provided during the tests for these.

Copy link

@jordan-ae, this task has been idle for a while. Please provide an update.

@ubiquity-os-beta ubiquity-os-beta bot closed this Nov 11, 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.

Properly handle task limit
5 participants