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

Improvement: Mining event sending check #3055

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

hannibal002
Copy link
Owner

@hannibal002 hannibal002 commented Dec 13, 2024

What

Somehow we send the data from every player all the time to Soopy server? This is definitely not intended.
Reported: https://discord.com/channels/997079228510117908/1316207755719348324

Changelog Improvements

  • Added option to disable sharing mining event data. - hannibal2
    • Improved accuracy when multiple users share mining event data.
    • Shared data includes event type, start, and end times.
  • Limited the "Mining Event Data can't be loaded from the server" error message to once in chat. - hannibal2

Changelog Technical Details

  • Added option to send a chat message only once. - hannibal2

@hannibal002 hannibal002 added the Soon This Pull Request will be merged within the next couple of betas label Dec 13, 2024
@hannibal002 hannibal002 added this to the Version 0.28 milestone Dec 13, 2024
@CalMWolfs
Copy link
Collaborator

I dont like this change as not sending the events when the setting is off significantly reduces the data that is available for the people that use this feature or other projects that rely on this api, is there an actual reason to disable this behaviour?

@hannibal002
Copy link
Owner Author

hannibal002 commented Dec 13, 2024

Why should a user send this data to others when they don't actively benefit from it by displaying it?
See farming contest sharing, we only send the data when the user has the farming contest display enabled.

This also falls in the the gray-ish category about sending user data to a server, without opt out options

@CalMWolfs
Copy link
Collaborator

the difference with farming contests and this is that farming contests needs 1 person to send it every 5 days, this needs constant data. Instead if people really want to opt out of this there should be a new option for this.

@CalMWolfs
Copy link
Collaborator

We also had this discussion before afaik and thats when we made it always send but hide errors if you had it off. The problem this user is having is that the errors when posting (from ApiUtils not from MiningEventTracker itself) still show. The posting method in ApiUtils should just have a boolean for wether to post errors in chat instead

@hannibal002
Copy link
Owner Author

i dont like the idea of hiding technical errors ever. we can hide known reasons (like the unspecified http 500 error or empty result when we know that soopy api is overloaded/offline) though

@CalMWolfs
Copy link
Collaborator

i dont like the idea of hiding technical errors ever. we can hide known reasons (like the unspecified http 500 error or empty result when we know that soopy api is overloaded/offline) though

I feel like generally this is the case but every rule has an exception, and i think having the base of a feature off is a fair exception for hiding errors relating to that feature

Copy link

I have detected some issues with your pull request:

Body issues:
Unknown category: Bug Fixes in text: ## Changelog Bug Fixes

Please fix these issues. For the correct format, refer to the pull request template.

@github-actions github-actions bot added the Wrong Title/Changelog There is an error in the title or changelog label Dec 13, 2024
@hannibal002
Copy link
Owner Author

in this case, the base is no real base option, since opt out is independent.
we want to know what random technical problems might occur for every user that is sending data, even when they dont want to see the feature themselfes.

As well as this is a reminder for the user that they are still sending data even though they dont see the display active.

@CalMWolfs CalMWolfs changed the title Improvement: Mining event sending check Fix: Mining event sending check Dec 13, 2024
@CalMWolfs CalMWolfs changed the title Fix: Mining event sending check Fix + Improvement: Mining event sending check Dec 13, 2024
@github-actions github-actions bot added the Bug Fix Bug fixes label Dec 13, 2024
@CalMWolfs
Copy link
Collaborator

Fair enough

@github-actions github-actions bot removed the Wrong Title/Changelog There is an error in the title or changelog label Dec 13, 2024
@github-actions github-actions bot added the Wrong Title/Changelog There is an error in the title or changelog label Dec 13, 2024
Copy link

I have detected some issues with your pull request:

Title issues:
PR has category 'Fix' which is not in the changelog. Expected categories: Improvement

Please fix these issues. For the correct format, refer to the pull request template.

@github-actions github-actions bot added the Detekt Has detekt problem label Dec 13, 2024
Copy link

One or more Detekt Failures were detected:

@github-actions github-actions bot removed the Detekt Has detekt problem label Dec 13, 2024
@CalMWolfs CalMWolfs changed the title Fix + Improvement: Mining event sending check Improvement + Backend: Mining event sending check Dec 13, 2024
@github-actions github-actions bot added Backend A backend pull request that will be merged soon and removed Bug Fix Bug fixes Wrong Title/Changelog There is an error in the title or changelog labels Dec 13, 2024
@hannibal002 hannibal002 merged commit df944a8 into beta Dec 13, 2024
14 of 15 checks passed
@github-actions github-actions bot removed Soon This Pull Request will be merged within the next couple of betas Backend A backend pull request that will be merged soon labels Dec 13, 2024
@hannibal002 hannibal002 changed the title Improvement + Backend: Mining event sending check Improvement: Mining event sending check Dec 13, 2024
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.

2 participants