Skip to content

Conversation

@BaranekD
Copy link

This PR does not allow to send empty errorreports. It can help before spamming unwanted reports.

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #1237 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1237   +/-   ##
=========================================
  Coverage     37.05%   37.05%           
  Complexity     3759     3759           
=========================================
  Files           136      136           
  Lines         11462    11462           
=========================================
  Hits           4247     4247           
  Misses         7215     7215

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fc5a63...ba71377. Read the comment docs.

@jaimeperez
Copy link
Member

jaimeperez commented Nov 18, 2019

Thanks a lot for your contribution, @BaranekD!

This one is a bit hard to decide on, because we definitely see legitimate error reports with the exception and URL not set. In the end, those fields being not set means something went really wrong, but not necessarily that the message is spam, since the data may still have been successfully recovered. Of course, lacking a backtrace means it's almost impossible for us to figure out where did that error originate, and fix it.

I'd say the most clear case here is when loading the exception data from the session fails, as that might indeed indicate that this could be automated in any way, for whatever purpose. But if loading the data succeeds, I think then we should send the report, even if parts of it are missing.

Also, bear in mind that if someone really wants to use this to spam, it would be simple to circumvent your check by manually inducing an error (e.g. page not found).

@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 1c686ab to eb20457 Compare August 17, 2020 20:43
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 08ebb9c to 64fca25 Compare July 2, 2021 14:12
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 7a53fc8 to d73ae47 Compare September 26, 2021 13:03
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from e5c0e21 to d5616df Compare January 9, 2022 11:00
@thijskh
Copy link
Member

thijskh commented Jan 18, 2022

I understand the issue but I agree that this might not be the best solution. What kind of reports are you getting that you want to prevent?

@tvdijen tvdijen force-pushed the master branch 7 times, most recently from 3b5f5ba to 96357ee Compare July 19, 2023 12:37
@tvdijen tvdijen force-pushed the master branch 5 times, most recently from 7587851 to d523b31 Compare August 2, 2023 11:58
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 8c90121 to d534e3b Compare September 5, 2023 08:09
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from bc1c5c8 to d0a5974 Compare October 17, 2023 21:17
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from ccb9b02 to 120a100 Compare December 1, 2023 14:34
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 6004a77 to 58bf8db Compare May 4, 2024 23:45
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 5c9fb2c to 0970efc Compare May 27, 2024 12:27
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from c27831c to 71e49f4 Compare June 19, 2024 17:03
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from c06a17a to a52c98d Compare August 20, 2025 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants