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: Include world and regionId as metadata with every notifier message #490

Merged
merged 11 commits into from
Jun 15, 2024
Merged

feat: Include world and regionId as metadata with every notifier message #490

merged 11 commits into from
Jun 15, 2024

Conversation

BlackCoyote
Copy link
Contributor

This pull request consists of changes that include a "world" and "regionId" property in the NotificationBody, so that they are included with every derived notification message. These properties are automatically populated in the DiscordMessageHandler if they hadn't been populated otherwise.

With the world property you can always know which world the notifying user is in, even after world hopping. (LoginNotifier does not act upon world hops), which allows for:

  • Better understanding what kind of seasonal world the player is in. (Assuming Deadman Mode, Leagues, and Beta worlds are all seasonal)

With the regionId property you can always know where in the world the notifying user is in, which allows for:

  • Better understanding certain identical chat messages in different scenarios. (For example: "Wave: X" in the Inferno, Fortis Colosseum, and the Fight caves.)
  • Make educated guesses about the context of various other kinds of notifier messages. (For example, the training method used during a level/xp notifier message.)
  • Build more interesting custom notifier messages across the board by including the region name.

I believe the world and regionId to be very valuable context for various notifiers, and harmless enough to include with every notifier message by default.

@BlackCoyote BlackCoyote changed the title Include world and regionId as metadata with every notifier message Feat: Include world and regionId as metadata with every notifier message Jun 5, 2024
@BlackCoyote BlackCoyote changed the title Feat: Include world and regionId as metadata with every notifier message feat: Include world and regionId as metadata with every notifier message Jun 5, 2024
@BlackCoyote
Copy link
Contributor Author

I'm afraid I don't understand why the notifier tests would throw an NPE. Any insight would be appreciated. Thanks!

@iProdigy
Copy link
Member

iProdigy commented Jun 5, 2024

not every notifier has the player's location defined, which causes an NPE when trying to get the region id - you can fix this by adding this line to MockedNotifierTest#setUp:

when(localPlayer.getWorldLocation()).thenReturn(new WorldPoint(2500, 2500, 0))

(i just chose a random location)

@BlackCoyote
Copy link
Contributor Author

not every notifier has the player's location defined, which causes an NPE when trying to get the region id - you can fix this by adding this line to MockedNotifierTest#setUp:

when(localPlayer.getWorldLocation()).thenReturn(new WorldPoint(2500, 2500, 0))

(i just chose a random location)

Thanks. Is the World already provided some way, or should that be added too?

@iProdigy
Copy link
Member

iProdigy commented Jun 5, 2024

Since world is an int, mockito defaults that to 0

@iProdigy
Copy link
Member

iProdigy commented Jun 5, 2024

We should have an advanced config setting to be able to opt-out of sending this data. For the pk notifier, for example, we have pkIncludeLocation (defaults to true).

with this new setting, the pk-specific setting could be removed. this would also require config migration code to set the new config value to false if the user set pkIncludeLocation to false - let us know if you'd prefer a maintainer to add this code

@BlackCoyote
Copy link
Contributor Author

That makes sense. I'll take you up on that offer to have a maintainer implement that, if that's alright.

@iProdigy iProdigy self-assigned this Jun 5, 2024
@iProdigy iProdigy requested review from Felanbird and pajlada June 6, 2024 06:19
@BlackCoyote BlackCoyote mentioned this pull request Jun 7, 2024
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe update the changelog entry to clarify that this is enabled by default, and only accessible to non-discord webhooks, and make it a non-Dev entry

@iProdigy
Copy link
Member

iProdigy commented Jun 8, 2024

@Felanbird is it bad UX that pk notifier location setting is moved to advanced section without any clear indication? (i.e., should we have 2 config settings for this)

@Felanbird
Copy link
Member

@Felanbird is it bad UX that pk notifier location setting is moved to advanced section without any clear indication? (i.e., should we have 2 config settings for this)

I would say yes, I assume PKers would perpetrate the same issues as sweats and not read the version bump chat message, even if the only bump info was MOVED SETTING TO ADVANCED SECTION

@iProdigy
Copy link
Member

✅ tested in-game

{
  "type": "LOOT",
  "playerName": "dankgg",
  "accountType": "NORMAL",
  "dinkAccountHash": "172d48fcbfce074e5385d67760091497dd5a761d97d19e871586ec54",
  "content": "dankgg has looted: \n\n1 x Big bones (306)\n3 x Law rune (357)\n\nFrom: Moss giant",
  "seasonalWorld": false,
  "world": 393,
  "regionId": 12698,
  "extra": {
    "items": [
      {
        "id": 532,
        "quantity": 1,
        "priceEach": 306,
        "name": "Big bones"
      },
      {
        "rarity": 0.03125,
        "id": 563,
        "quantity": 3,
        "priceEach": 119,
        "name": "Law rune"
      }
    ],
    "source": "Moss giant",
    "category": "NPC",
    "killCount": 2344,
    "rarestProbability": 0.03125
  }
}

@iProdigy iProdigy merged commit 62355b8 into pajlads:master Jun 15, 2024
4 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.

4 participants