Allow 'Exceptions' For Kill Count Notifier

jdanthdavis commented 1 month ago

Checklist

[X] I've searched the issues and pull requests for similar looking suggestions.
[X] I've checked the Unreleased section of the changelog for newly added features that sound like my suggestion.

Describe your Suggestion

Allow users to enter exceptions that doesn't follow the Kill Count Interval value; similar to Ignored Region IDs in the Death setting. Whether this setting would just send a notification for every time the boss is killed or if possible allow the user to also define what the Kill Count Interval would be for that exception.

Reasoning

This would be useful in situations where a group has their Kill Count Interval set to 100 to avoid spam notifications for the average boss but being able to set Kill Count Interval to say 5 for Zuk/Colosseum would be a nice to have.

iProdigy commented 1 month ago

Possible syntax

Boss:interval - e.g., Nex:5

If :interval suffix is missing, assume :1

Longer example

Hespori:20
Nex
Vorkath:100
Zulrah:1

but if the same boss is listed twice with different intervals, it's unclear which to use

iProdigy commented 1 month ago

This setting may be too complicated to accurately explain to users

Since you have programming experience, I would recommend setting your interval to 1 and write code for a custom webhook handler to forward the notification if your boss-specific interval is met

jdanthdavis commented 1 month ago

This setting may be too complicated to accurately explain to users

Since you have programming experience, I would recommend setting your interval to 1 and write code for a custom webhook handler to forward the notification if your boss-specific interval is met

Thanks for the information. This may be a good opportunity for me to explore plugin development! Do you have any recommended guides/docs for creating custom webhooks?

iProdigy commented 1 month ago

To clarify, rather than writing runelite plugin code, you can write a HTTP request handler that accepts Dink's POST requests and performs custom logic. If you set your kc interval to 1, every kc would hit your logic, which can selectively forward the notification to discord webhook URLs depending on whether the kc is divisible by arbitrary boss-specific values.

To host your webhook handler for free, you can use Cloudflare Workers or AWS Lambda or a localhost webserver.

To parse Dink requests, I recommend you read these docs: https://github.com/pajlads/DinkPlugin/blob/master/docs/json-examples.md#structure

jdanthdavis commented 1 month ago

To clarify, rather than writing runelite plugin code, you can write a HTTP request handler that accepts Dink's POST requests and performs custom logic. If you set your kc interval to 1, every kc would hit your logic, which can selectively forward the notification to discord webhook URLs depending on whether the kc is divisible by arbitrary boss-specific values.

To host your webhook handler for free, you can use Cloudflare Workers or AWS Lambda or a localhost webserver.

To parse Dink requests, I recommend you read these docs: https://github.com/pajlads/DinkPlugin/blob/master/docs/json-examples.md#structure

Thank you for the clarification. I'm making good progress! I was wondering if there was some documentation for boss names? I.e. will Dink send me Inferno vs Tzkal-Zuk or Fortis Colosseum vs Sol heredit?

iProdigy commented 1 month ago

Thank you for the clarification. I'm making good progress! I was wondering if there was some documentation for boss names? I.e. will Dink send me Inferno vs Tzkal-Zuk or Fortis Colosseum vs Sol heredit?

Glad to hear! For the kill count plugin, it uses the exact name used in the chat message. The main exception to this is (Corrupted) Gauntlet: we report the NPC name instead (i.e., Crystalline Hunllef or Corrupted Hunllef). For your two examples, TzKal-Zuk and Sol Heredit are correct

jdanthdavis commented 1 month ago

Thank you for the clarification. I'm making good progress! I was wondering if there was some documentation for boss names? I.e. will Dink send me Inferno vs Tzkal-Zuk or Fortis Colosseum vs Sol heredit?

Glad to hear! For the kill count plugin, it uses the exact name used in the chat message. The main exception to this is (Corrupted) Gauntlet: we report the NPC name instead (i.e., Crystalline Hunllef or Corrupted Hunllef). For your two examples, TzKal-Zuk and Sol Heredit are correct

Appreciate the information! I am getting a bit stuck on creating the img url to where when it sends to the webhook it posts the screenshot from Dink. Do you have any tips on how to tackle this? Does the img actually have to be hosted somewhere?

APKiwi commented 1 month ago

Ignoring the heresy that is my snake case, this is my node.js running express + multer to handle dink requests. I've stripped out the surrounding code, this should hopefully point you in the right direction.

This link has a lot of great documentation: https://expressjs.com/en/resources/middleware/multer.html

//Require express and multer (for multi-part webhook/API body handling)
const express = require("express");
const multer = require("multer");
//Initialize express as webhook_handler and define a port
const webhook_handler = express();
const PORT = 1234;
//Start express on the defined port
webhook_handler.listen(PORT, () => console.log(`🚀 Webhook Listening-Server running on port ${PORT}.`));
webhook_handler.use(express.json());

//Multer diskstorage for image up/down load to local
const storage = multer.diskStorage({
    destination: './webhook_files',
    filename: function (req, file, cb) {
        cb(null, path.parse(file.originalname).name + "-" + Date.now() + path.extname(file.originalname));
    }
});

//Multer file filter for extention
const file_filter = function (req, file, cb) {
    const allowed_mimes = ['image/jpeg', 'image/jpg', 'image/png', 'image/gif'];
    if (allowed_mimes.includes(file.mimetype)) {
        cb(null, true);
    } else {
        cb(null, false);
    }
};

//upload called when file receieved on webhook, file size limit and filter
//maximum file size is 15Mb
const upload = multer({
    storage: storage,
    limits: { fileSize: 15 ** 7 },
    fileFilter: file_filter
}).single('file');

We define standard express server, define port, begin listening
We define the storage function: using multer we define the parameters for storing the file locally. For me this is on my destination ./webhook_files
We ensure that the name of the file is consistent naming wise and uses the same extension provided initially
We defined the file_filter function which is a security catch to ensure we are not saving anything outside of the desired file types.
We define the upload function which includes the previous functions, and also defines single file mode (avoids multiple files, again a minor security concern that we prevent).
Following this, I have some reasonably complex code that would be difficult to share here, but you can initialize a call with something like I do (calling to another file) using the originally defined webhook_handler and upload: webhook_handler.post(webhook.route_param, upload, (...args) => another_file.execute(bot, sqlcon, upload, ...args));
In the separate file you will want to consider validation/other security to ensure you are handling "bad" requests or malicious requests.
You will want to consider then routing different things to different channels or servers depending on your use case (different GIMs/discords/whatever)

APKiwi commented 1 month ago

Once you handle all of this and build some infrastructure around your webhook_handler, when you get up to the handling of KC (I had the same use case) it's very simple:

//Assume request is parsed to this class on API call, which includes the whole incoming request, body, imagepath etc

const content = request.body;
//Set payload to JSON parsed payload_json from body.
const payload = JSON.parse(content.payload_json);
//Create type, player_name, account_type const, from above payload information.
const type = payload.type;
const player_name = payload.playerName;
const account_type = payload.accountType;
//Create extra const, from supplied JSON metadata from 'Dink'
const extra = payload.extra;

//File handling
const image_name = request.file ? request.file.filename : null;
const image_path = request.file ? request.file.path : null;

//Prepare generic event_embed (used for all types).
const event_embed = new Discord.EmbedBuilder()
    .setColor("#ad4234")
    .setAuthor({ name: author_name, iconURL: payload.embeds[0].author.icon_url, url: payload.embeds[0].author.url })
    .setTimestamp()

//THIS STEP IS IMPORTANT, if the user did not submit an image, you want to account for that
//Check if image was submitted -> if image, set the attachment to the image.
if(image_path){
     event_embed.setImage(`attachment://${image_name}`);
}

//HANDLE KILL COUNT (BOSSING)
if(payload.type === "KILL_COUNT"){
     const boss_kc = extra.count;
     const remainder_by_5 = extra.count % 5;
     const remainder_by_50 = extra.count % 50;

     if(boss_kc < 5){
          //OPTIONAL MESSAGE
     }else if(boss_kc < 50 && remainder_by_5 === 0){
          //OPTIONAL MESSAGE
     }else if(remainder_by_50 === 0){
          //OPTIONAL MESSAGE
     }else{
       //Otherwise, no condition above matched, so delete the file if any and return.
       if(image_path) //handle deletion of image from server (free up storage space)
       return;
     }

     event_embed
        .setDescription(`${source.aquire_type} **[${extra.boss}](${source.wiki_URL}})** for the ${extra.count}${utils_formatting.ordinal(extra.count)} time!`)
        .setThumbnail('https://oldschool.runescape.wiki/images/Multicombat.png')
}

iProdigy commented 1 month ago

you can also simplify this by just reusing the incoming request so you don't need to save the image to file or bother with discord.js

if (payload.type === "KILL_COUNT") {
  const boss = extra.boss;
  const kc = extra.count;
  if (isNoteworthyKc(boss, kc)) {
    return await fetch(env["url"], request);
  }
}

jdanthdavis commented 1 month ago

I would think this approach would work for me with fastify as well but something as simple as -

await fetch(WH_URL, { method: "POST", headers: { 'content-type':'multipart/form-data; boundary=--------------------------807394529256768815026859'}, body: request, })

but I'm having no luck. I'm leaning towards the issue being with the body and how Discord is expecting a multipart/form-data post to look like.

I've tried with Dink sending the request and with Postman. Here is the log from Postman -

POST / HTTP/1.1
User-Agent: PostmanRuntime/7.39.0
Accept: */*
Postman-Token: 7c33dadb-b80f-42ad-992e-729050ff32de
Host: 127.0.0.1:3000
Accept-Encoding: gzip, deflate, br
Connection: keep-alive
Content-Type: multipart/form-data; boundary=--------------------------740985452248073530185399
Cookie: __dcfduid=896cb03a1d2511ef85d3aa593567cc5c; __sdcfduid=896cb03a1d2511ef85d3aa593567cc5c8e91ab58643ba6ce1b7b7810faf449fea093a0b42c4ffd728b108ee2134e15bb
Content-Length: 501747

----------------------------740985452248073530185399
Content-Disposition: form-data; name="payload_json"
{"type":"KILL_COUNT","playerName":"LSx Swap","accountType":"NORMAL","dinkAccountHash":"","seasonalWorld":false,"extra":{"boss":"Sarachnis","count":691,"gameMessage":"Your Sarachnis kill count is: 691.","time":"PT32S","isPersonalBest":false},"content":"**LSx Swap** has defeated **Sarachnis** with a completion count of **691**","embeds":[]}
----------------------------740985452248073530185399
Content-Disposition: form-data; name="file"; filename="Untitled.png"
<Untitled.png>
----------------------------740985452248073530185399--

HTTP/1.1 200 OK
content-type: text/plain; charset=utf-8
content-length: 2
Date: Tue, 28 May 2024 19:31:43 GMT
Connection: keep-alive
Keep-Alive: timeout=72

iProdigy commented 1 month ago

don't think you can pass fastify's Request object as a fetch body. might be easier to yield http code 307 to trigger client-sided redirect: reply.code(307).redirect('WH_URL')

iProdigy commented 1 month ago

oh actually code 307 won't work because runelite uses an ancient okhttp version (3.14.9), and 307/308 POST are not supported until 4.6.0: https://togithub.com/square/okhttp/issues/3111

iProdigy commented 1 month ago

if you're willing to wait, we can add logic to dink to workaround that okhttp bug

jdanthdavis commented 1 month ago

don't think you can pass fastify's Request object as a fetch body. might be easier to yield http code 307 to trigger client-sided redirect: reply.code(307).redirect('WH_URL')

I went ahead and started looking down this approach but testing with Postman for the time being. It does work however, if an image is sent with the redirect I get error Error: You cannot pipe to this stream after the outbound request has started.. No matter the flow of redirects. I.e. -

No img redirect ->Posts -> No img redirect -> Posts -> img redirect -> error img redirect -> Posts -> img redirect -> error img redirect -> Posts -> No img redirect -> error

Pretty clear the image has something to do with it but I can't seem to find any similar situations online.

iProdigy commented 1 month ago

@jdanthdavis could you share your full js code? this problem is outside the realm of dink (can't replicate using a cloudflare worker), but maybe one of us can spot where things are going wrong

iProdigy commented 1 month ago

@jdanthdavis sample cloudflare worker handler https://gist.github.com/iProdigy/53654691a76679221a74fd442dea068a

jdanthdavis commented 1 month ago

Here is my current Cloudflare Worker (would love feedback if you're not busy) - https://gist.github.com/jdanthdavis/647ab73453373f2381f357c9652b58b7

After reviewing yours I was able to get Postman to work flawlessly! Of course, an attempt with Dink didn't succeed but I'm sure your fix will address that. Here is the Cloudflare log of the Dink post in case it could be helpful for you. At this point I think we just wait til the fix is published. I really appreciate your help @iProdigy and @APKiwi! It was my first time working with webhooks and workers and it was awesome to learn.

{
  "outcome": "ok",
  "scriptVersion": {
    "id": "redacted"
  },
  "scriptName": "redacted",
  "diagnosticsChannelEvents": [],
  "exceptions": [],
  "logs": [],
  "eventTimestamp": 1716968129827,
  "event": {
    "request": {
      "url": "redactedCloudflareUrl",
      "method": "POST",
      "headers": {
        "accept-encoding": "gzip, br",
        "cf-connecting-ip": "redacted",
        "cf-ipcountry": "US",
        "cf-ray": "redacted",
        "cf-visitor": "{\"scheme\":\"https\"}",
        "connection": "Keep-Alive",
        "content-length": "556",
        "content-type": "multipart/form-data; boundary=bde7ef43-32e6-4137-babc-40c8e9e2b0f1",
        "host": "redactedCloudflareUrl",
        "user-agent": "RuneLite/1.10.31.2-b22f15e (Dink/1.x)",
        "x-forwarded-proto": "https",
        "x-real-ip": "redacted"
      },
      "cf": {
        "clientTcpRtt": 18,
        "longitude": "redacted",
        "httpProtocol": "HTTP/2",
        "tlsCipher": "redacted",
        "continent": "NA",
        "asn": 209,
        "clientAcceptEncoding": "gzip",
        "country": "US",
        "verifiedBotCategory": "",
        "tlsClientAuth": {
          "certIssuerDNLegacy": "",
          "certIssuerSKI": "",
          "certSubjectDNRFC2253": "",
          "certSubjectDNLegacy": "",
          "certFingerprintSHA256": "",
          "certNotBefore": "",
          "certSKI": "",
          "certSerial": "",
          "certIssuerDN": "",
          "certVerified": "NONE",
          "certNotAfter": "",
          "certSubjectDN": "",
          "certPresented": "0",
          "certRevoked": "0",
          "certIssuerSerial": "",
          "certIssuerDNRFC2253": "",
          "certFingerprintSHA1": ""
        },
        "tlsExportedAuthenticator": {
          "clientFinished": redacted,
          "clientHandshake": redacted,
          "serverHandshake": redacted,
          "serverFinished": redacted
        },
        "tlsVersion": "TLSv1.3",
        "city": redacted,
        "timezone": redacted,
        "colo": "BNA",
        "tlsClientHelloLength": "385",
        "edgeRequestKeepAliveStatus": 1,
        "postalCode": redacted,
        "region": redacted,
        "latitude": redacted,
        "requestPriority": "weight=16;exclusive=0;group=0;group-weight=0",
        "regionCode": redacted,
        "asOrganization": redacted,
        "metroCode": redacted,
        "tlsClientExtensionsSha1": redacted,
        "tlsClientRandom": redacted
      }
    },
    "response": {
      "status": 200
    }
  },
  "id": 0
}

iProdigy commented 1 month ago

@jdanthdavis glad to hear!

in terms of code review:

extra.playerName should be payload.playerName
nit: if you plan to handle other notification types in the future, i wouldn't store the kc-specific extra fields to variables before checking the notification type (but js is a loose language so your code is fine too). a side effect: when someone achieves a quest speedrunning personal best, the notification is forwarded to PB_URL with your current code
nit: bossMap.has(bossName.toUpperCase()) can just be bossInterval !== undefined
THEATRE OF BLOOD HARD MORE has a typo (MORE should be MODE)
for your "special occasion" handling, bossName === 'SOL HEREDIT' and bossName === 'TZKAL-ZUK' will always evaluate to false since you don't have bossName.toUpperCase()
that said, isPersonalBest is usually true for kc === 1 (sometimes not, due to jagex spaghetti), so the notification would be forwarded anyways

lastly, since you're on cf workers now, if you don't want to wait for the okhttp patch, you can switch back to return await fetch(isPb ? PB_URL : KC_URL, request);

jdanthdavis commented 1 month ago

@iProdigy thanks for the feedback; some really good catches their as well! I went ahead and made the adjustments - https://gist.github.com/jdanthdavis/647ab73453373f2381f357c9652b58b7

It is fully working now as intended. However, I did receive this 500 error stating This ReadableStream is disturbed (has already been read from), and cannot be used as a body. The solution was to change const form = await request.formData(); to const form = await request.clone().formData();

Again, greatly appreciate all the help with this!

jdanthdavis commented 1 month ago

@iProdigy Quick question, we ran into an instance where the special occasion logic triggered and we saw some unexpected results. A player got their first KC but it also was their personal best since it's their first completion. However, the player double-deathed with the boss. What's interesting is that we had the message for a personal best but it was sent to our KC URL.

I've setup a fiddle with manipulated data and it works as intended. Did we see this because Dink would be sending two different payloads for this scenario? I.e it sent the personal best payload first and then it would have sent the kc payload after? I'm just confused on how we received the dink message for a personal best but the return await fetch(!isPb ? KC_URL : PB_URL, request); went to the PB_URL.

Fiddle

iProdigy commented 1 month ago

Your logic has: const isPb = extra.isPersonalBest && killCount !== 1;

So isPb would be false since it's their first kill

jdanthdavis commented 1 month ago

Which aligns with what I intended but we received the personal best message (but sent to our KC_URL) from Dink. We never received a notification for KC. Shouldn't Dink have sent 2 payloads in this scenario?

iProdigy commented 1 month ago

It's a single kill so dink sends 1 kc notif

The forwarded message is missing the specific kc because the user has modified the PB Notification Message to not include %COUNT%

jdanthdavis commented 1 month ago

I see. We achieved a PB so dink sends the PB message but we modified our message to not include %COUNT% which by default is included in the Dink message.

So, to achieve the logic/messages I want I would have to actually modify the message from Dink once I've done my logic? Which won't be possible until your PR is merged if I'm not mistaking.

iProdigy commented 1 month ago

you don't need the 307/308 pr to fix this

The easiest approach is to just have users update the value of PB Notification Message. For example, folks could just append at a completion count of **%COUNT%**. If your users are doing ::dinkimport, you can update the json to make this easier on users

Alternatively, you can use the values in the extra object to construct the message on your server instead, and POST directly to the discord url. This is more involved and folks typically utilize a discord webhook library to create the embed (but it seems like you're not doing embeds, so prolly easy enough without a lib)

jdanthdavis commented 1 month ago

The easiest approach is to just have users update the value of PB Notification Message. For example, folks could just append at a completion count of %COUNT%. If your users are doing ::dinkimport, you can update the json to make this easier on users

100% is the easiest solution for this. This is definitely a super picky scenario (plus learning, ya know?)

Alternatively, you can use the values in the extra object to construct the message on your server instead, and POST directly to the discord url. This is more involved and folks typically utilize a discord webhook library to create the embed (but it seems like you're not doing embeds, so prolly easy enough without a lib)

I am able to deconstruct everything from extra that I need and build specific messages depending on our logic in utils.js. However, what I am struggling with is creating the correct structure for the payload to the webhook. From reading the official Discord docs it seems not utilizing embeds is a bit of an odd approach these days so the documentation is subpar for constructing a payload in this manner. I'm finding mixed information between needing content, message instead of content, or even using payload_json for the body(?).

My next approach will be to review the logs of my worker to see what the request from Dink looks like. Cloudflare is a bit lame in the fact that you need to pay for their upgraded plan to actual have logs that are within your worker and not just the real time logs that only shows the status, trigger, url, ect and of course they have an open issue for upgrading workers currently 😅

iProdigy commented 1 month ago

yeah the discord docs may not be so clear:

You should use FormData to prepare the POST body. The first part should be called payload_json. The second optional part can be named file, which would contain the screenshot (when available)
For the json part, you just need {"content": "your message here"}

cloudflare's real-time logs do report console.log calls, so you can just leave that running until a notification comes through (and avoid needing to upgrade to a paid worker)

jdanthdavis commented 1 month ago

Happy to say with your help we're making wonderful progress. We're getting successful posts with Dink with the custom messages. Albeit, I'm currently passing a random blob for the img since I'm issues grabbing the Dink img. Would that be from:

const file = await request.file;

I also couldn't find the property names of file in the Dink docs either. request.file.name(?)

Just for reference on how I'm building the blobfrom an img URL:

let blob = await fetch("https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcRtGtjx1KW-dZmtSyZP2bOPl0PVxtDdLAMUGg&s").then(r => r.blob());

let formData = new FormData();
formData.append('payload_json', '{"content": "your message here"}');
formData.append("file", blob, "mockPhoto.jpg");

iProdigy commented 1 month ago

you should use const file = form.get("file") since you already have const form = await request.clone().formData();

then:

const formData = new FormData();
formData.append("payload_json", JSON.stringify({"content": "your message here"}));
if (file !== null) {
    formData.append("file", file)
}

jdanthdavis commented 1 month ago

I really appreciate your help! With mock data we're looking good. Getting each custom message sent to it's corresponding URL. If you have the time I'd love some feedback on my approach.

https://gist.github.com/jdanthdavis/647ab73453373f2381f357c9652b58b7

iProdigy commented 1 month ago

I really appreciate your help! With mock data we're looking good. Getting each custom message sent to it's corresponding URL. If you have the time I'd love some feedback on my approach.

https://gist.github.com/jdanthdavis/647ab73453373f2381f357c9652b58b7

yw! I'd add a null check before appending file (and make sure it can be reused across multiple requests)

may also want to consider a try/catch in case one request fails (which would cause dink to attempt a repeated notif)

jdanthdavis commented 1 month ago

Made those adjustments! We did just have an instance where a PB was obtained in game but for the time we posted PT21M35S which is just straight from extra.time. Does Dink handle the formatting here and applies it to the gameMessage before sending the payload? If that's the case and I'm not overlooking something I guess I'll have to do some cleaning of that string.

In the case I do need to clean the string myself; do you have an example of what extra.time will look like when the player has the precise timing setting in game enabled? I.e. PT21M35S10MS?

iProdigy commented 1 month ago

yeah we send extra.time as a ISO-8601 duration

with precise timing, it'd look like PT21M35.01S

one trick is to remove PT prefix and add a space after every letter (e.g., via regex)

jdanthdavis commented 1 month ago

Seems to be working flawlessly with mock data! Went with this solution - let sanitizedTime = time.replace("PT", "").replace("S", "").replaceAll(regex, ":") for our preferred formatting. Thank you again, it's been a pleasure learning!

jdanthdavis commented 1 day ago

Hey @iProdigy, noticed something a bit weird with the payload for HMT PBs. We got a 5 man PB but I got isPersonalBest: false. Below are screenshots of the exact in-game message and the payload from that exact raid. Any idea what may be going on here?

EDIT: Looks like I'm getting send my "Theatre of Blood total completion time" when the "Theatre of Blood completion time" was the new PB.

iProdigy commented 1 day ago

Could you screenshot the game tab where it says new personal best (rather than clan tab)

jdanthdavis commented 1 day ago

Could you screenshot the game tab where it says new personal best (rather than clan tab)

Sure!

iProdigy commented 1 day ago

Dink uses the last time posted in chat, so it's reporting 18:25.80, which technically wasn't a pb

pajlads / DinkPlugin