search

ubiquity / ubiquibot

Putting the 'A' in 'DAO'
https://github.com/marketplace/ubiquibot
MIT License
17 stars 61 forks source link

feat: fix automatic unassign #759

Closed wannacfuture closed 1 year ago

wannacfuture commented 1 year ago

Resolves #685

Fixed lastActivityTime function

netlify[bot] commented 1 year ago

Deploy Preview for ubiquibot-staging ready!

Name Link
Latest commit b72d55db6dbecda5f509e92f73183ade60f35ed9
Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/651c6805f2a05a0008cb79fc
Deploy Preview https://deploy-preview-759--ubiquibot-staging.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

0x4007 commented 1 year ago
  1. The PR addresses the third requirement by refining the "last activity time" logic.
  2. However, the PR does not seem to implement any logic to prevent the bot from undoing user actions. There's no code that checks if an assignee was added by a user and then prevents the bot from unassigning that user. Similarly, there's no code related to labels and their potential removal by the bot.

In conclusion:

To fully satisfy the requirements, the PR would need additional logic to ensure the bot respects user actions like assigning users or adding labels.

Source: https://chat.openai.com/share/3fac1c44-7998-4d8a-aee8-603a300aede4

wannacfuture commented 1 year ago

ChatGPT is smarter than I expected 🤣

But updating lastActivityTime function is enough to prevent bot from unassigning hunter right after manual assignment. Mr. GPT, lol

Or if I add a specific label, perhaps the bot should be mindful to never remove that same added label.

This is already implemented, I think. E.g. when I add priority label bot removes original priority label.

@pavlovcik

0x4007 commented 1 year ago

I'm pretty sure its not implemented.

I remember asking about chaining together handlers and sharing context to ensure that it understands the user action that invoked the bot and does not undo it and you saying that's difficult to do.

Should be for everything including for now: assigns, and price label

wannacfuture commented 1 year ago

I'm pretty sure its not implemented.

I remember asking about chaining together handlers and sharing context to ensure that it understands the user action that invoked the bot and does not undo it and you saying that's difficult to do.

Should be for everything including for now: assigns, and price label

https://github.com/ubiquity/card-issuance/issues/14

On the above issue, the bot removes the price label right after you have set it. But it didn't remove the label which you have added. It removed the original label.

For example, if there was 200 USD label, and you add 300 USD label, the bot will remove 200 label and leave 300. Isn't this the right behaviour?

Or if I add a specific label, perhaps the bot should be mindful to never remove that same added label.

Not so sure what should be done exactly for this. @pavlovcik

0x4007 commented 1 year ago

But it didn't remove the label which you have added. It removed the original label.

image

Looks wrong

wannacfuture commented 1 year ago

haha you battery is running out lol

I investigated about it. The bot removes all the price labels which isn't appropriate for given priority and time labels even you set a price label in your mind manually.

in the beginning of the issue the bot added 200 USD for priority 1 time 1 day. but after that, not sure if the rule of pricing is changed(maybe config??) but the bot accepted 200 USD for priority 2 and time 1 day at last.

So that's why bot removed your 200USD label after you labeled it, because he thought 200 is not appropriate for priority1 and time 1 day.

But I'm not sure why he didn't add right label after removing yours even it is designed to do in the code. We need to investigate the logs.

So I suggest this:

0x4007 commented 1 year ago

It does not fully satisfy the requirements regarding the bot undoing user actions (e.g., unassigning users or removing labels).

I don't get why this is difficult to understand.

Regardless of the details of what the bot thinks the right thing to do is currently, it should respect the user's wishes like setting the label or setting assigns.

This should be generalized to work with any user input/action because we will continue to add capabilities.

wannacfuture commented 1 year ago

It does not fully satisfy the requirements regarding the bot undoing user actions (e.g., unassigning users or removing labels).

I don't get why this is difficult to understand.

Regardless of the details of what the bot thinks the right thing to do is currently, it should respect the user's wishes like setting the label or setting assigns.

This should be generalized to work with any user input/action because we will continue to add capabilities.

It does make sense.

I pushed commit but is it enough to check there is existing price label? @pavlovcik

0x4007 commented 1 year ago

I pushed commit but is it enough to check there is existing price label? @pavlovcik

This should be generalized to work with any user input/action because we will continue to add capabilities.

0xcodercrane commented 1 year ago

we're fine to merge the PR @wannacfuture @pavlovcik? no comments from me

whilefoo commented 1 year ago

it seems the code already got merged from somewhere else? https://github.com/ubiquity/ubiquibot/blob/9d00d7f8be6e7b2eeb91f8a01246a1d9697881d7/src/handlers/wildcard/unassign.ts#L104

wannacfuture commented 1 year ago

it seems the code already got merged from somewhere else?

https://github.com/ubiquity/ubiquibot/blob/9d00d7f8be6e7b2eeb91f8a01246a1d9697881d7/src/handlers/wildcard/unassign.ts#L104

Yeah, another hunter pulled this branch.

rndquu commented 1 year ago

@whilefoo Good to see you're back online 💊 👍