ubiquibot / conversation-rewards

0 stars 17 forks source link

Rename variables to match the configuration and introduce `score` concept #83

Closed gentlementlegen closed 3 weeks ago

gentlementlegen commented 3 weeks ago
          At least in the config we renamed things accordingly. For example I'm pretty sure we renamed `symbol` to `regex`, so we should do the same in the code here. 

Also score should just sum per instance. For example if we want to award $1 for every paragraph, then score for p should be 1.

score is not supposed to be a multiplier.

However in the config I believe we also support a multiplier property. This should be separate from score.

_Originally posted by @0x4007 in https://github.com/ubiquibot/conversation-rewards/pull/68#discussion_r1721049100_

0x4007 commented 3 weeks ago

I was reviewing this action run and its a lot more clear to me when looking at the clean JSON logs exactly whats going on here. I realized that the scoring is implemented incorrectly. The primary crediting strategy is supposed to be based on the amount of elements. The word counter is a separate scoring mechanism. This was added as an after thought in version one, but it seems that you prioritized word count over element count.

The problem is that the original emphasis on element count allows us to easily target and credit special and useful elements such as <a>, <img>, and <code>. Helpful comments generally have links, images and code samples.

Now you are keeping track of word count PER element which is more complex than it needs to be. Remember, the word count scoring strategy was added as an after thought for version one. Version one simply counts all the words in the comment (but also ignores words in specific elements, like code.) at the end of all the complex calculations.

Aside from the ignore capability, it doesn't care which element contains the words.

{
  "id": 2014495969,
  "content": "Wouldn't solve scenarios requiring keys or credentials",
  "url": "https://github.com/ubiquity/cloudflare-deploy-action/issues/6#issuecomment-2014495969",
  "type": "ISSUE_ASSIGNEE",
  "score": {
    "formatting": {
      "content": {
        "p": {
          "count": 7, // should be changed to `wordCount` and you should correctly count the amount of `p` tags to be `elementCount`
          "score": 1
        }
      },
      "wordValue": 0,
      "formattingMultiplier": 0
    },
    "reward": 0,
    "relevance": 0.3
  }
}

Source

0x4007 commented 3 weeks ago

@gentlementlegen we don't have label pricing which is a big problem for switching to the new bot.

Does it need to have the ubiquity:listeners on its manifest? https://ubiquibot-assistive-pricing.ubiquity.workers.dev/manifest.json

0x4007 commented 3 weeks ago

@gentlementlegen not sure what else to do here, but it doesn't seem to listen to the label events.

gentlementlegen commented 3 weeks ago

@0x4007 I am looking at it but we are doing the same actions at the same time haha. Probably the configuration is not valid, gimme a minute.

gentlementlegen commented 3 weeks ago

@0x4007 It seems to run fine but answers with a 403 when executed.

0x4007 commented 3 weeks ago

I have no idea about what to do with that.

gentlementlegen commented 3 weeks ago

I suspect this is the culprit: https://github.com/ubiquibot/assistive-pricing/blob/478942e5f25e4dfad8f85b219900b699aa804b9f/src/worker.ts#L44

0x4007 commented 3 weeks ago

I suspect this is the culprit: https://github.com/ubiquibot/assistive-pricing/blob/478942e5f25e4dfad8f85b219900b699aa804b9f/src/worker.ts#L44

So this is some old auth mechanism? Should we remove it from the secrets and the code?

image
gentlementlegen commented 3 weeks ago

@0x4007 I am attempting to redeploy with the proper key value. If don't know if I have the proper one. If you wanna change it: https://github.com/ubiquibot/assistive-pricing/settings/secrets/actions then go to Actions and run Deploy on the main branch.

0x4007 commented 3 weeks ago

This is necessary for every plugin? Doesn't seem right.

0x4007 commented 3 weeks ago

I just updated the public key.

gentlementlegen commented 3 weeks ago

It should be in the future for security purposes if I recall. It was only implemented there though.

0x4007 commented 3 weeks ago

Remade to reduce clutter

ubiquityos[bot] commented 3 weeks ago
# Issue was not closed as completed. Skipping.
gentlementlegen commented 3 weeks ago

@0x4007 Remember our good friend

        "InvalidCharacterError: atob() called with invalid base64-encoded data. (Only whitespace, '+', '/', alphanumeric ASCII, and up to two terminal '=' signs when the input data length is divisible by 4 are allowed.)"

Did you set line breaks or no line breaks?

0x4007 commented 3 weeks ago

Oh I used the x25519 public key. We really need to change the property name to be APP_PUBLIC_KEY

0x4007 commented 3 weeks ago

Updated to the correct key.

gentlementlegen commented 3 weeks ago

@0x4007 Key seems to give a 403. Not sure about the expected logic there, I think @whilefoo had implemented this one. I can temporarily comment that logic maybe?

0x4007 commented 3 weeks ago

Yes lets remove authentication

gentlementlegen commented 3 weeks ago

@0x4007 Works without the auth, to be added back later.

0x4007 commented 3 weeks ago

I don't see the need for auth but thanks for fixing.