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: disable permit gen for users in non collaborative issues #182

Merged

Conversation

ishowvel
Copy link
Contributor

@ishowvel ishowvel commented Nov 7, 2024

Resolves #160

@0x4007
Copy link
Member

0x4007 commented Nov 8, 2024

Please post QA and prove it works as expected

@ishowvel
Copy link
Contributor Author

ishowvel commented Nov 9, 2024

Currently out for an university, will qa this issue in a few hours

@ishowvel
Copy link
Contributor Author

ishowvel commented Nov 9, 2024

Qa is in progress please wait

@ishowvel
Copy link
Contributor Author

everything has been a mess since this pr got merged, tests not working resulting to it become weirdly hard to debug code

@ishowvel
Copy link
Contributor Author

ishowvel-org/.ubiquity-os#16 when issue closer is admin
ishowvel-org/.ubiquity-os#17 when issue closer is not admin

@ishowvel
Copy link
Contributor Author

i hope for the tests to get fixed as this has caused me a magnitude of problems and headaches, waiting for workflows to run is not a good alternative to local test runs

@ishowvel
Copy link
Contributor Author

Please post QA and prove it works as expected

@0x4007 posted qa

@0x4007
Copy link
Member

0x4007 commented Nov 10, 2024

@gentlementlegen They claim your changes made this harder to test so you'll know best

@gentlementlegen
Copy link
Member

@ishowvel You can just merge the development branch here and you should not have any issues with the tests.

@ishowvel
Copy link
Contributor Author

@ishowvel
Copy link
Contributor Author

test and qa both are passing

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.

Seems close but study the code elsewhere to see how we do logging and comments. They are formatted using our logger methods.

src/parser/github-comment-module.ts Outdated Show resolved Hide resolved
src/parser/permit-generation-module.ts Outdated Show resolved Hide resolved
@ishowvel ishowvel requested a review from 0x4007 November 12, 2024 07:19
@gentlementlegen
Copy link
Member

@ishowvel please delete the dist folder so the binary gets re-generated thanks.

@ishowvel
Copy link
Contributor Author

  ● web3.ts › getERC20TokenSymbol() › Should return ERC20 token symbol

    thrown: "Exceeded timeout of 60000 ms for a test.
    Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

      3 | describe("web3.ts", () => {
      4 |   describe("getERC20TokenSymbol()", () => {
    > 5 |     it("Should return ERC20 token symbol", async () => {
        |     ^
      6 |       const networkId = 100; // gnosis
      7 |       const tokenAddress = "0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d"; // WXDAI
      8 |       const tokenSymbol = await getErc20TokenSymbol(networkId, tokenAddress);

this is just a one time failure because tests pass locally and this should get fixed if the workflow is ran once again
@gentlementlegen this should be ready to be merged

@ishowvel
Copy link
Contributor Author

@gentlementlegen
Copy link
Member

@ishowvel I restarted the Jest site and indeed tests work. However, when I re-opened and closed an issue within an organization where I am an administrator, it didn't work, see Meniole#33 (comment) did you test this case?

@ishowvel
Copy link
Contributor Author

ishowvel commented Nov 27, 2024

ishowvel-org/.ubiquity-os#39
Yes I did
In this test case ishowvel2 is an admin in the ishowvel-org organisation, ishowvel2 non collaboratively opened and completed an issue and rewards were generated

(At the time of closing the issue ishowvel2 was admin, ishowvel2 is now a member)

@ishowvel
Copy link
Contributor Author

ishowvel commented Nov 27, 2024

Here's the workflow run for this reward and the commit
ishowvel@5b0d6ea

https://github.com/ishowvel/text-conversation-rewards/actions/runs/12046833453

@ishowvel
Copy link
Contributor Author

@ishowvel I restarted the Jest site and indeed tests work. However, when I re-opened and closed an issue within an organization where I am an administrator, it didn't work, see Meniole#33 (comment) did you test this case?

@gentlementlegen can you give me the commit of the workflow run which produced that reward so I can debug further

@gentlementlegen
Copy link
Member

@ishowvel I am not sure about what you're asking me exactly, the workflow run url? If so, I ran that locally on my machine. Otherwise, the commit is the latest on your branch.

@ishowvel
Copy link
Contributor Author

ishowvel commented Nov 27, 2024

@gentlementlegen things work on my end
ishowvel-org/.ubiquity-os#42
this issue is an exact replica of your test case which is working as expected
can i ask of you to try using workflows because there might be something wrong with local runs?

@ishowvel
Copy link
Contributor Author

ishowvel commented Nov 28, 2024

@gentlementlegen
image
organization roles dont support billing manager, organization memberships support this. can you try to change the type of the qa account like given in the above image
i think this may be the problem as i saw the account you were using to do qa was a "collaborator" and not a "member"

@gentlementlegen
Copy link
Member

@ubiquity-ubiquibot is indeed a collaborator which is why it should fail, that worked fine. By my account is the admin of the org so I should have the capability to generate permits, which didn't work.

@ishowvel
Copy link
Contributor Author

admin
ishowvel-org/.ubiquity-os#45
non admin
ishowvel-org/.ubiquity-os#46

the problem was me assuming that people will always follow the devflow of filing an issue, pricing the issue, assigning themselves and then closing

@ishowvel
Copy link
Contributor Author

@gentlementlegen https://github.com/ishowvel/text-conversation-rewards/actions/runs/12097760718/job/33733486500
This is another case of tests failing for no reason, I think this might be an issue worth being looked into

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.

_isCollaborative, _nonAssigneeApprovedReviews and _isAdmin are duplicated. Please move these into another file and reuse them. Otherwise it seems to work fine now.

@gentlementlegen
Copy link
Member

Seems ok
Meniole#39
Meniole#38
Meniole#37

@ishowvel
Copy link
Contributor Author

ishowvel commented Dec 5, 2024

I hate you mr GitHub actions

@gentlementlegen gentlementlegen merged commit 6fbddbb into ubiquity-os-marketplace:development Dec 6, 2024
3 checks passed
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.

Only admin can generate rewards non collaboratively
3 participants