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

/review enhancement - official review states and toggle draft pull requests #791

Open
0x4007 opened this issue Sep 21, 2023 · 15 comments · May be fixed by #868
Open

/review enhancement - official review states and toggle draft pull requests #791

0x4007 opened this issue Sep 21, 2023 · 15 comments · May be fixed by #868

Comments

@0x4007
Copy link
Member

0x4007 commented Sep 21, 2023

This will require testing but I wanted to write down the idea before I forget.

  1. We ask contributors to open a draft pull request as soon as they start working on the task. When they are ready, they convert it to a "finalized" pull request.
  2. A collaborator can call /review and if the results are satisfactory, then UbiquiBot should "approve" the pull request using the official pull request "approve" review state.
  3. If changes are requested then the bot should "request changes" and convert to the pull request to a draft.
    1. Converting to a draft clearly signals to everyone that the pull request is not ready to be reviewed by humans yet.

Perhaps if the bot is unsure, it can neither "approve" or "request changes" which is perfectly fine. In this case, it should not convert it to a draft.

@0x4007
Copy link
Member Author

0x4007 commented Sep 21, 2023

@Keyrxng this one is for you

@Keyrxng
Copy link
Contributor

Keyrxng commented Oct 17, 2023

shit I totally missed this my apologies, I'll get this done for the weekend

@Keyrxng Keyrxng linked a pull request Oct 21, 2023 that will close this issue
@Keyrxng
Copy link
Contributor

Keyrxng commented Oct 22, 2023

I can't seem to submit the draft ready for review, I tried many a method but none were fruitful so I've left it to gpt to tell them essentially 'because the bot has approved the review they should submit the draft ready for review' while also submitting an approved review

@0x4007
Copy link
Member Author

0x4007 commented Nov 8, 2023

I can't seem to submit the draft ready for review, I tried many a method but none were fruitful so I've left it to gpt to tell them essentially 'because the bot has approved the review they should submit the draft ready for review' while also submitting an approved review

The original specification actually just requires you to convert from "ready" to "draft". Not necessarily from "draft" to "ready"

However not sure if that solves the problem you seem to have.

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 9, 2023

If changes are requested then the bot should "request changes" and convert to the pull request to a draft.
Converting to a draft clearly signals to everyone that the pull request is not ready to be reviewed by humans yet.

I extrapolated this intention so that, the pr will always be a draft until AI confirms to the user it is ready for human eyes, ultimately the PR never leaves draft until it's been reviewed and approved by AI.

I assume notifications are fired across the org when a pr is submitted for review (not a draft), my intention was to minimize those notifications.

But if you don't like this approach and would rather that they actually ready it for review and then have the bot either bounce it back to draft or submit an approved review (which is is doing at the moment on the draft) I can rework things.

It's unclear to me whether you pointing out the original spec was signalling that the implementation is not right or if that was just context, my apologies

@0x4007
Copy link
Member Author

0x4007 commented Nov 10, 2023

I extrapolated this intention so that, the pr will always be a draft until AI confirms to the user it is ready for human eyes, ultimately the PR never leaves draft until it's been reviewed and approved by AI.

The intent behind draft pull requests in the context of Ubiquity is that they should opened immediately after being assigned to a task, to communicate that the work is being handled. I expect that draft pull requests can remain as drafts for several weeks (e.g. my refactor) until they are ready for review. The author should indicate when it is ready for review, not the bot.

Converting a draft pull request into an "ready" pull request is the proper way to communicate to the reviewers that it is ready for review. I don't believe that the bot will understand this better than the author for when the work is "complete."

I assume notifications are fired across the org when a pr is submitted for review (not a draft), my intention was to minimize those notifications.

No, I don't believe so.


Something I realized but it would be a pretty straightforward developer experience if:

  1. the bot automatically performs the review as soon as the draft is converted to a finalized pull request.
  2. the bot can request changes, and then mark it back as a draft.
  3. if the author converts it back to a finalized pull request, the bot will check if there have been any commits since the last review. If there are commits, then it will attempt a new review.

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 13, 2023

Alright I'm following, still learning this ins and outs of open source and Git at this level.

the bot automatically performs the review as soon as the draft is converted to a finalized pull request.

This sounds to me like the command should be removed and have it just be an applied action whenever a pr is submit for review?

Originally the intention was that the assignee would call /review whenever they wished but it should be a hook of sorts as opposed to a command or should both functionalities exist?

@0x4007
Copy link
Member Author

0x4007 commented Nov 13, 2023

Perhaps without the command it will offer a better developer experience. As long as there are changes in the code, perhaps we can have no automated review limits in this early implementation.

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 13, 2023

perhaps we can have no automated review limits in this early implementation.

Hinting towards a consequence after a set amount in the future, I like it 😂

@0x4007
Copy link
Member Author

0x4007 commented Nov 13, 2023

There's a few planned "rewards" for the DevPool system.

  1. The cash reward that already exists.
  2. XP which is a virtual scoring system that projects can use for gating tasks
  3. NFTs (POAPs)

We can deduct, for example, 50 XP each time a review is requested. And the user earns 1 XP for every dollar earned from completed tasks.

If a task is worth $300, then the user can call review six times before they get disqualified etc.

This should push the user to actually try and deliver high quality work, while still receiving the agreed upon cash payment.

I understand for many contributors, the cash payments may be less flexible to manipulate as a disincentive for undesirable behaviors.

XP can provide an incentive for producing higher quality work by using it as a virtual currency for perks like this potentially.

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 15, 2023

I really like the vision, so you get the cash payout in full regardless so long as you do not get disqualified through poor quality work.

Will the xp system be retroactively applied or is it a fresh clean slate for everyone when it goes live?

It could be interesting (I know I probs would spend) to have some of that fine merch xp-redeemable lmao 🤣

@0x4007
Copy link
Member Author

0x4007 commented Nov 20, 2023

Will the xp system be retroactively applied or is it a fresh clean slate for everyone when it goes live?

We would have to write a script to figure that out if thats the case. I think it makes sense to import XP as there are some contributors that have been with us for over a year. However I'm unsure how burdensome it will be to produce.

I was also thinking a UBQ token airdrop to correspond with these XP migrations so perhaps its worth writing software for.

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 23, 2023

It would be interesting to see a clean slate for everyone though, you could maybe put it to a vote?

If retro then those that have been here longer would have an 'early-bird' bonus towards accessing the new gated bounties (I assume gating and xp will be released together) which is fair I guess because first in and all that but imagine the scenes.

Everyone having to clear out the level 1 jobs before they can move on up essentially forcing those bounties that get overlooked for being too easy or 'not worth it', even those who are 'grandfathered' into the new system having been able to overlook them in the past can no longer, the 'pvp-ness' would be unreal 😂

The trouble then is:

  • Is there enough current bounties to allow N hunters to be able to progress to the next level without clearing out all current level bounties and is there then enough of that level to allow progression, so on and so forth.

  • It's probs safer to migrate xp that way when it's rolled out there will be at least a handful of hunters that could likely already access level 2/3/4 jobs leaving those entry jobs for new hunters

@0x4007
Copy link
Member Author

0x4007 commented Nov 24, 2023

If we do XP gating based on priority level, then theres plenty of Priority: 1 (Normal) tasks available.

I'm okay with the grandfathering because those who have closed out many issues generally are better equipped to handle the complex tasks.

I have a feeling we won't write that script to migrate the XP unless we see a need to though. I suppose it depends on if there's other urgent development tasks that take precedence or not.

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 24, 2023

Oh that's interesting, I assumed somehow it would be task complexity based as opposed to urgency based.

I'll have about 10xp so I'm not fussed either way in that sense but others may be.

Anyway back on track, so review now going forward won't be an invocable command but instead should be a hook that is called anytime a hunter submits their PR for review.

If it passes the llm assessment should the bot say anything to the hunter or should it just allow it to stay in ready for review?

Obv if it fails then it should bounce the pr back into a draft with the tasklist etc as to whats still to be done.

Atm no limit on submissions but in the future there will be:

  • We could make it as simple as embedding a metadata tag in the bounce comment and counting how many time it appears
  • If we do include a comment from the bot even if it passes then if it's the case that after human review it needs bounced back then we can count the failed pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants