Max assignments #9

jordan-ae · 2024-07-14T21:59:49Z

Resolves #8

This functionality prevents new or less experienced contributors from overcommitting and blocking progress by self-assigning too many tasks. If the user is a core team member there's no max task limit.

QA: ubq-test-jordan

jordan-ae · 2024-07-14T22:08:16Z

@Keyrxng & @gentlementlegen Not sure which Issue to open this PR against. Should I open it against this command-start-stop repo. The changes that I made are very few

gentlementlegen · 2024-07-15T03:11:15Z

@jordan-ae Please rebase, I just merged #1 in.

jordan-ae · 2024-07-15T04:53:37Z

Thanks @gentlementlegen all good now

Keyrxng · 2024-07-15T13:31:38Z

I will review this today @jordan-ae

src/handlers/shared/check-org-member.ts

src/handlers/shared/start.ts

gentlementlegen · 2024-07-15T14:01:04Z

It would be nice to have related unit tests regarding the configuration settings. Also, the max value should be configurable based on the user's association level (collaborator, contributor, member etc.).

Keyrxng · 2024-07-15T14:36:07Z

+1 to the above comments.

I'd also keep in mind that custom roles can be defined within an org so it should be able to cover those too

jordan-ae · 2024-07-15T16:25:26Z

+1 to the above comments.

I'd also keep in mind that custom roles can be defined within an org so it should be able to cover those too

Hummm interesting do we have like standard user roles that Ubiquity uses, or we do I have to account for all the possible custom roles that can possibly be

Keyrxng · 2024-07-15T16:30:04Z

+1 to the above comments.
I'd also keep in mind that custom roles can be defined within an org so it should be able to cover those too

Hummm interesting do we have like standard user roles that Ubiquity uses

Ubiquity tends to use the pre-defined roles.

do I have to account for all the possible custom roles that can possibly be

Yeah just I'd just pass in an iterable type and parse whatever it defines, you don't need to know the roles/limits as you write the code if the config does the heavy lifting

It's best to add defaults into plugin-input.ts though and I'm sure @0x4007 can help in defining those

github-actions · 2024-07-15T20:38:03Z

Unused files (2)

src/main.ts, src/plugin.ts

Unused dependencies (2)

Filename	dependencies
package.json	`@actions/core` `@actions/github`

Unused exports (1)

Filename	exports
src/types/plugin-input.ts	`startStopSettingsValidator`

Unused types (1)

Filename	types
src/types/plugin-input.ts	`PluginInputs`

src/plugin.ts

src/handlers/shared/start.ts

src/handlers/shared/get-user-role.ts

Keyrxng · 2024-07-15T22:08:09Z

@gentlementlegen re: #9 (comment)

Are we sticking with Knip in CI when it's error prone like this? I know rndquu has mentioned it before also

If we are then are we just manually ignoring the erroneous problem files?

src/handlers/shared/get-user-role.ts

src/plugin.ts

Keyrxng · 2024-07-15T22:34:24Z

do we have like standard user roles that Ubiquity uses

@jordan-ae one thing I had trouble with building plugins at first is the mental model. While Ubiquity is currently the primary user of the bot and it's plugins, we are not the intended demographic for either.

Plugins are built with the "Partner" and their needs in mind which is why configurability needs to be handled in such a way that it's either real easy (on/off or a specific value(s)) or we handle it elegantly under-the-hood.

0x4007 plans to have complete configurability right down to the /<cmd> string without affecting plugin execution. So when it's things that can be arbitrary like labels or roles you need to think "how can I handle all possibilities?" otherwise we impose standards onto partners which could make the plugin less attractive.

gentlementlegen · 2024-07-16T02:53:43Z

@gentlementlegen re: #9 (comment)

Are we sticking with Knip in CI when it's error prone like this? I know rndquu has mentioned it before also

If we are then are we just manually ignoring the erroneous problem files?

I think the only valid file to ignore is the entry point. I actually solve that by mentioning the entry point within the package.json so Knip doesn't complain and evaluates everything correctly.

jordan-ae · 2024-07-22T09:13:42Z

I've been dreading sending in these changes because I'm unsure if what I've done makes sense. Essentially, I've modified the maxConcurrentTask configuration to accept an array of roles and limits. Then, I check the configuration against the user's role to determine the appropriate maximum number of tasks.

jordan-ae · 2024-07-22T09:16:32Z

do we have like standard user roles that Ubiquity uses

@jordan-ae one thing I had trouble with building plugins at first is the mental model. While Ubiquity is currently the primary user of the bot and it's plugins, we are not the intended demographic for either.

Plugins are built with the "Partner" and their needs in mind which is why configurability needs to be handled in such a way that it's either real easy (on/off or a specific value(s)) or we handle it elegantly under-the-hood.

0x4007 plans to have complete configurability right down to the /<cmd> string without affecting plugin execution. So when it's things that can be arbitrary like labels or roles you need to think "how can I handle all possibilities?" otherwise we impose standards onto partners which could make the plugin less attractive.

Thanks @Keyrxng this helped a lot

src/handlers/shared/start.ts

src/types/plugin-input.ts

Keyrxng · 2024-07-22T12:40:15Z

what I've done makes sense

It does make sense yeah you have the right approach now. The code is starting to look acceptable, it would be good to see some QA of the changes in action but if not definitely some unit tests covering the added configurables

QA might be difficult here without an enterprise trial for custom roles. Also I have an alt GH account I use for this sort of live QA because if your the only member of your test org it's hard to test various roles etc

src/handlers/shared/get-user-task-limit-and-role.ts

Keyrxng · 2024-09-07T20:33:27Z

Removed items from my review because #35 addresses the double comment posting (due to the catch-all), logReturn etc.

Not sure why the error has changed in the tests in this PR for when the APP_ID is not set vs development but I'll correct anything in #35

src/utils/issue.ts

gentlementlegen · 2024-09-11T03:18:37Z

@jordan-ae You should enable Prettier on save because many files are not properly formatted. Run yarn format:prettier to fix them all at once.

gentlementlegen

Works for me, here is my QA: Meniole#20

jordan-ae · 2024-09-12T08:56:29Z

Works for me, here is my QA: Meniole#20

Yaaayyyyy 🎉

0x4007 · 2024-09-12T09:14:46Z

README.md

@@ -38,7 +38,10 @@ To configure your Ubiquibot to run this plugin, add the following to the `.ubiqu
  with:
    reviewDelayTolerance: "3 Days"
    taskStaleTimeoutDuration: "30 Days"
-    maxConcurrentTasks: 3
+    maxConcurrentTasks: # Default concurrent task limits per role.
+      admin: 10


I feel like admins shouldn't have limits but to be honest I feel this feature has no teeth because we can just use the GitHub UI.

It would be more interesting if it unassigns when collaborators self assign and they hit the limit.

@0x4007 we can change UI assignments to follow the same logic as /start so user would get unassigned if they are doing too many tasks, even when forcing it through the UI.

0x4007 · 2024-09-12T09:17:51Z

According to the original conversation on the issue, admins should not have limits. Please remove from the default.

jordan-ae · 2024-09-12T09:25:29Z

@0x4007 i tried using infinity for admins in the yaml config. But it didn't seem to be too reliable. And to the best of my knowledge that's the easiest way to define unlimited task to admins. Other than that I think it will just make sense and be much easier to give admins a larger task limit. Like 100 maybe? To represent infinity

0x4007 · 2024-09-12T09:27:57Z

Just delete the admin related config. Remove any logic related to admins. It makes no sense to limit admins, that's the purpose of an admin

jordan-ae · 2024-09-12T09:46:01Z

Okay got it I've set admin to Infinity by default. So admins will no longer have any limits

gentlementlegen · 2024-09-12T09:54:18Z

src/types/plugin-input.ts

@@ -16,7 +16,7 @@ export const startStopSchema = T.Object(
    reviewDelayTolerance: T.String({ default: "1 Day" }),
    taskStaleTimeoutDuration: T.String({ default: "30 Days" }),
    startRequiresWallet: T.Boolean({ default: true }),
-    maxConcurrentTasks: T.Record(T.String(), T.Integer(), { default: { admin: 20, member: 10, contributor: 2 } }),
+    maxConcurrentTasks: T.Record(T.String(), T.Integer(), { default: { admin: Infinity, member: 10, contributor: 2 } }),


Could you totally remove it?

0x4007

I'm assuming that Infinity works

gentlementlegen requested review from gentlementlegen and Keyrxng July 15, 2024 13:52

gentlementlegen reviewed Jul 15, 2024

View reviewed changes

src/handlers/shared/check-org-member.ts Outdated Show resolved Hide resolved

gentlementlegen requested changes Jul 15, 2024

View reviewed changes

src/handlers/shared/start.ts Outdated Show resolved Hide resolved