(chore) remove user with no data

ubiquity-os-marketplace / text-conversation-rewards

1 stars 27 forks source link

(chore) remove user with no data #150

Closed jaykayudo closed 1 month ago

jaykayudo commented 1 month ago

Resolves https://github.com/ubiquity-os-marketplace/text-conversation-rewards/issues/145

jaykayudo commented 1 month ago

@gentlementlegen @0x4007 Please look at my new implementation and see if it fulfills the requirements. I have formatted my code and added tests.

gentlementlegen commented 1 month ago

@jaykayudo Thank you for the test. However I think the logic should be different:

the user that got 0 reward should not be removed from the result
however, the posted comment on GitHub should not display that user, see this file

jaykayudo commented 1 month ago

@jaykayudo Thank you for the test. However I think the logic should be different:

the user that got 0 reward should not be removed from the result

however, the posted comment on GitHub should not display that user, see this file

Okay. i am on it though the test will the complicated now because i will need a mock of a github comment json that has comments that amounts to zero and an actual issue url that the mock is generated from and a html mock as well. @gentlementlegen is it possible for this to be provided to me?

gentlementlegen commented 1 month ago

@jaykayudo You have a few options:

you can put the GitHub API result in a json file and use it, like in the tests
you can directly call the function with a fake result and see if the call from the POST to GitHub is correct

The second one should be the easier.

jaykayudo commented 1 month ago

@gentlementlegen i just made a push and added a unit test and it passes but test of code sections that i did not touch are failing. Could you please look and this and also check out my new implementation and see whether it matches the requirement.

gentlementlegen commented 1 month ago

@jaykayudo We are having some trouble with RPCs at the moment so you did not introduced these problems, no worries. Please resolve the file conflicts.

jaykayudo commented 1 month ago

@jaykayudo We are having some trouble with RPCs at the moment so you did not introduced these problems, no worries. Please resolve the file conflicts.

@gentlementlegen i have resolved the conflicts. waiting on your review

jaykayudo commented 1 month ago

I tested and indeed the result did not contain the users with 0 as a reward, but the returned JSON did contain them. Let me give this a run in the org within the issue that was having this problem and I'll merge.

That’s great. I will expecting a successful merge then.