De dupe error uploads & Add Map Name to Error Uploads

[DanVanAtta]

Commits

commit 6af5f46

De-Dupe Error Report Uploads

Prevent redundant error message uploads by first checking
with the server if an error report already exists.
If the error report exists, then we will show a prompt
to the user and give them a link to the existing error report.

Change Summary:
- Add columns in error report history table to track the
  title of the error report, reporting game version,
  and the issue link that was created.
- Add APIs to error reporting DAO to get the link of
  an existing error report by title and game version.
- Add server API to check if an error report exists
- Update client so that after clicking 'upload error report',
  we first query the server if an error report exists.
  If one exists, we show the link to the user, othewise
  we show the user the upload form.

Note: we use both title and game version for de-duplication.
  This may lead to some duplication as version number
  changes, but in case we do not fix a problem or if
  the title matches but is actually different in a different
  version we will allow the error report upload.

commit 8ed128a

Add field to error report upload for map name

commit e9da7eb

Do not print Exception in its own block (redundant)

Exception is always at the head of the stack trace which
we already print in the issue body. This update removes
the redundant 'exception' block.

Functional Changes

[] New map or map update
[] New Feature
[x] Feature update or enhancement
[] Feature Removal
[] Code Cleanup or refactor
[] Configuration Change
[] Problem fix
[] Other:

Testing

Add an explicit throw exception at the end of gameRunner launch to test out against a local server. Created an error report and then reported and verified that there was a de-dupe message.

Screens Shots

De-Dupe Sequence

Existing screen prompt to upload:

Clicking upload, get the de-dupe message:

New Map Name Field

Notice at the bottom of the dialog there is a 'map name' field:

Notice in the preview there is a 'map name' section:

Additional Notes to Reviewer

Release Note

UPDATE|Error uploads will now recognize duplicates and give you a link to an existing error upload if there is one. Added 'map-name' as an error upload field

[DanVanAtta]

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- http-server/src/main/java/org/triplea/modules/error/reporting/ErrorReportController.java  2
- http-server/src/main/java/org/triplea/modules/error/reporting/CanUploadErrorReportStrategy.java  1
- http-server/src/test/java/org/triplea/modules/error/reporting/CanUploadErrorReportStrategyTest.java  1
- game-core/src/main/java/org/triplea/debug/error/reporting/UploadDecisionModule.java  2
- game-core/src/main/java/org/triplea/debug/error/reporting/CanNotUploadSwingView.java  1
         

See the complete overview on Codacy

[codeclimate]

Code Climate has analyzed commit e9da7eb and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 26.4% (0.0% change).

View more on Code Climate.

[Cernelius]

I think it should say "game name", as that is what the players have visible the most, it is what they select and it should give a more specific insight, for maps having two or more games. For example, if a player is playing "WAW 1940", it makes the most sense that it should tell such game name, instead of delving into finding what is the map name for that game, and posting "world_at_war". On top of this, the map names have the issue that are usually defined differently (upper cases additions and underscores removals) when you are prompted to download the maps, so the game name is consistent the most.

So, I strongly advice changing "which map are you playing" with "which game are you playing" and "Map" with "Game" in the report.

[DanVanAtta]

Let's bike shed the map vs game naming terminology in: #6994

[RoiEXLab]

Something worth considering:
As we all know the report body is truncated at X characters. This means that the trailing \n```\n\n will be cut off if the stacktrace is too long.

Perhaps we want to truncate the stracktrace so we don't ever hit the other limit

[DanVanAtta]

If you're missing the trailing ```, you'll still get the code format.

If we truncate only the stack trace, you'll need to calculate how much the rest of the content is taking and do a subtraction.

It then seems just easier to truncate the whole thing knowing that the stack trace is at the end.

[RoiEXLab]

True, just wanted to through the idea out there

[RoiEXLab]

I wonder if a check for equality is the right move here.
If we fix an issue, but people are still using an unpatched version, they shouldn't be able to create an issue IMO.
Thoughts?

[DanVanAtta]

That'll be a problem if you go back in time. So we can certainly get 'n' issues up until a problem is fixed. That is a risk. If we fail to fix an issue, it's more risky if we suppress it. False-positives are not great, false-negatives are worse.

Does that make sense?

[DanVanAtta]

@RoiEXLab how do you mean by an unpatched version?

Let's say a problem is reported for 'v2', and someone is using a 'v3' and we only have a fix in 'v4', we can get a duplicate report fro both 'v2' and 'v3', is that the scenario you are considering?

IMO it's okay:

• the stack traces could change between v2 and v3 along with line numbers. Having that be different is slightly helpful.
• we still go from 'n' reports being linear to the number of versions instead of number of occurrances.
• we avoid a situation where we think we fixed the problem in 'v4', but perhaps we did not. If we de-dupe on title only, then a closed issue is theoretically definitive even if the problem is re-introduced or never fixed in the first place.

In part we're being clever about detecting when problems are fixed by just de-duping by title and version. If we suppressed all future version reports we'd need to know when a problem is fixed. As-is, a person looks at the bug report, sees it's closed and hopefully we'll comment which version has a fix.

Going forward pre-release hopefully will get back to being used for preview and testing and not early access like it was. In part the early access was due to map features and because map versioning is deficient when it comes to forward & backward compatibility. Removing the need for early access implies we've some work to improve how maps are handled.

[RoiEXLab]

is that the scenario you are considering

Yes, that's exactly what I meant

[DanVanAtta]

@RoiEXLab I'm going to merge this so I can cut a 2.1 tonight. That gives some time tomorrow to respond to any issues. I'm happy to respond to any further feedback post-merge.