Skip to content

Commit

Permalink
De-Dupe Error Report Uploads
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
DanVanAtta committed Jul 1, 2020
1 parent c84ae44 commit 6af5f46
Show file tree
Hide file tree
Showing 26 changed files with 538 additions and 44 deletions.
4 changes: 2 additions & 2 deletions game-core/src/main/java/org/triplea/debug/ErrorMessage.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.SwingUtilities;
import org.triplea.debug.error.reporting.StackTraceReportView;
import org.triplea.debug.error.reporting.UploadDecisionModule;
import org.triplea.http.client.error.report.ErrorReportClient;
import org.triplea.live.servers.LiveServersFetcher;
import org.triplea.swing.JButtonBuilder;
Expand Down Expand Up @@ -141,7 +141,7 @@ private void activateUploadErrorReportButton(final URI lobbyUri, final LogRecord
INSTANCE.uploadButton.addActionListener(
e -> {
hide();
StackTraceReportView.showWindow(windowReference, errorReportClient, logRecord);
UploadDecisionModule.processUploadDecision(windowReference, errorReportClient, logRecord);
});
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package org.triplea.debug.error.reporting;

import java.util.Optional;
import javax.swing.JFrame;
import javax.swing.JOptionPane;
import lombok.experimental.UtilityClass;
import org.triplea.http.client.error.report.CanUploadErrorReportResponse;
import org.triplea.swing.JEditorPaneWithClickableLinks;
import org.triplea.swing.jpanel.JPanelBuilder;

/**
* Shows a dialog that gives a message to user about why they cannot report an error (usually
* because it already exists). If a link to an error report is provided in the server response, this
* dialog renders a clickable link the user can click to open the bug report.
*/
@UtilityClass
class CanNotUploadSwingView {

static void showView(
final JFrame parent, final CanUploadErrorReportResponse canUploadErrorReportResponse) {

JOptionPane.showMessageDialog(
parent,
new JPanelBuilder()
.border(10)
.add(
new JEditorPaneWithClickableLinks(
createWindowTextContents(canUploadErrorReportResponse)))
.build(),
"",
JOptionPane.INFORMATION_MESSAGE);
}

private static String createWindowTextContents(
final CanUploadErrorReportResponse reportResponse) {
return reportResponse.getResponseDetails()
+ "<br>"
+ Optional.ofNullable(reportResponse.getExistingBugReportUrl())
.map(url -> JEditorPaneWithClickableLinks.toLink(url, url))
.orElse("");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ class StackTraceErrorReportFormatter implements BiFunction<String, LogRecord, Er
@Override
public ErrorReportRequest apply(final String userDescription, final LogRecord logRecord) {
return ErrorReportRequest.builder()
.title(versionSupplier.get() + ": " + createTitle(logRecord))
.title(createTitle(logRecord))
.body(buildBody(userDescription, logRecord))
.gameVersion(versionSupplier.get())
.build();
}

Expand All @@ -44,7 +45,7 @@ public ErrorReportRequest apply(final String userDescription, final LogRecord lo
* logging or exception, and we expect for there to always either be at least a log message or an
* exception.
*/
private static String createTitle(final LogRecord logRecord) {
static String createTitle(final LogRecord logRecord) {
// if we have a stack trace with a triplea class in it, use that for the title;
// EG: org.triplea.TripleaClass.method:[lineNumber] - [exception]; Caused by: [exception cause]
return extractTripleAClassAndLine(logRecord)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package org.triplea.debug.error.reporting;

import games.strategy.engine.ClientContext;
import java.util.logging.LogRecord;
import javax.swing.JFrame;
import lombok.experimental.UtilityClass;
import org.triplea.http.client.error.report.CanUploadRequest;
import org.triplea.http.client.error.report.ErrorReportClient;

/**
* Decision module to handle the case where a user wishes to report an error to TripleA. First we
* need to invoke the 'can-upload' API on the server which will tell us if there is an existing bug
* report. If so we render a window that will let them see the bug report, otherwise we show the bug
* upload report form.
*/
@UtilityClass
public class UploadDecisionModule {

public static void processUploadDecision(
final JFrame parentWindow, final ErrorReportClient uploader, final LogRecord logRecord) {

final var canUploadRequest =
CanUploadRequest.builder()
.errorTitle(StackTraceErrorReportFormatter.createTitle(logRecord))
.gameVersion(ClientContext.engineVersion().toString())
.build();

final var canUploadErrorReportResponse = uploader.canUploadErrorReport(canUploadRequest);

if (canUploadErrorReportResponse.getCanUpload()) {
StackTraceReportView.showWindow(parentWindow, uploader, logRecord);
} else {
CanNotUploadSwingView.showView(parentWindow, canUploadErrorReportResponse);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,19 @@ class StackTraceErrorReportFormatterTest {

@Mock private LogRecord logRecord;

@Nested
final class VerifyVersion {
@Test
void verifyVersion() {
when(logRecord.getSourceClassName()).thenReturn("org.ClassName");

final ErrorReportRequest errorReportResult =
new StackTraceErrorReportFormatter(() -> "4.1").apply(SAMPLE_USER_DESCRIPTION, logRecord);

assertThat(errorReportResult.getGameVersion(), is("4.1"));
}
}

@Nested
final class VerifyTitle {
@Test
Expand All @@ -47,7 +60,7 @@ void logMessageOnly() {
new StackTraceErrorReportFormatter(() -> "4.1").apply(SAMPLE_USER_DESCRIPTION, logRecord);

assertThat(
errorReportResult.getTitle(), is("4.1: ClassName#" + METHOD_NAME + " - " + LOG_MESSAGE));
errorReportResult.getTitle(), is("ClassName#" + METHOD_NAME + " - " + LOG_MESSAGE));
}

@Test
Expand All @@ -60,7 +73,7 @@ void logMessageOnlyWithTripleaPackage() {
new StackTraceErrorReportFormatter(() -> "4.1").apply(SAMPLE_USER_DESCRIPTION, logRecord);

assertThat(
errorReportResult.getTitle(), is("4.1: ClassName#" + METHOD_NAME + " - " + LOG_MESSAGE));
errorReportResult.getTitle(), is("ClassName#" + METHOD_NAME + " - " + LOG_MESSAGE));
}

@Test
Expand All @@ -73,8 +86,7 @@ void handleNullLogMessageAndNullExceptionMessage() {
assertThat(
errorReportResult.getTitle(),
is(
"5.6: "
+ StackTraceErrorReportFormatterTest.class.getSimpleName()
StackTraceErrorReportFormatterTest.class.getSimpleName()
+ "#<clinit>:"
+ EXCEPTION_WITH_NO_MESSAGE.getStackTrace()[0].getLineNumber()
+ " - "
Expand Down Expand Up @@ -102,8 +114,7 @@ void handleExceptionWithCause() {
assertThat(
errorReportResult.getTitle(),
is(
"5.6: "
+ StackTraceErrorReportFormatterTest.class.getSimpleName()
StackTraceErrorReportFormatterTest.class.getSimpleName()
+ "#<clinit>:"
+ EXCEPTION_WITH_CAUSE.getCause().getStackTrace()[0].getLineNumber()
+ " - "
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package org.triplea.http.client.error.report;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import lombok.Builder;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.ToString;

@ToString
@Builder
@Getter
@EqualsAndHashCode
public class CanUploadErrorReportResponse {
/**
* True means a user can upload an error report. False means an error report is already uploaded.
* If true, then responseDetails and existingBugReportUrl will be null.
*/
@Nonnull private final Boolean canUpload;
/**
* Contains any message details that should be displayed to the user. EG: "This error is already
* uploaded"
*/
@Nullable private final String responseDetails;
/** Contains a link to any existing error report that matches the same error the user sees. */
@Nullable private final String existingBugReportUrl;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package org.triplea.http.client.error.report;

import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.ToString;

@ToString
@EqualsAndHashCode
@Builder
@NoArgsConstructor
@AllArgsConstructor
@Getter
public class CanUploadRequest {
private String gameVersion;
private String errorTitle;
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
@AllArgsConstructor(access = AccessLevel.PRIVATE)
public class ErrorReportClient {
public static final String ERROR_REPORT_PATH = "/error-report";
public static final String CAN_UPLOAD_ERROR_REPORT_PATH = "/error-report-check";
public static final int MAX_REPORTS_PER_DAY = 5;

private final Map<String, Object> headers;
Expand All @@ -31,4 +32,13 @@ public static ErrorReportClient newClient(final URI uri) {
public ErrorReportResponse uploadErrorReport(final ErrorReportRequest request) {
return errorReportFeignClient.uploadErrorReport(headers, request);
}

/**
* Checks if user can upload a request. A request can be uploaded if: - it has not yet been
* reported - reporting version is greater than fix version - user is not banned
*/
public CanUploadErrorReportResponse canUploadErrorReport(
final CanUploadRequest canUploadRequest) {
return errorReportFeignClient.canUploadErrorReport(headers, canUploadRequest);
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.triplea.http.client.error.report;

import feign.FeignException;
import feign.HeaderMap;
import feign.Headers;
import feign.RequestLine;
Expand All @@ -10,12 +9,11 @@
@SuppressWarnings("InterfaceNeverImplemented")
@Headers({HttpConstants.CONTENT_TYPE_JSON, HttpConstants.ACCEPT_JSON})
interface ErrorReportFeignClient {
/**
* API to upload an exception error report from a TripleA client to TripleA server.
*
* @throws FeignException Thrown on non-2xx responses.
*/
@RequestLine("POST " + ErrorReportClient.ERROR_REPORT_PATH)
ErrorReportResponse uploadErrorReport(
@HeaderMap Map<String, Object> headers, ErrorReportRequest request);

@RequestLine("POST " + ErrorReportClient.CAN_UPLOAD_ERROR_REPORT_PATH)
CanUploadErrorReportResponse canUploadErrorReport(
@HeaderMap Map<String, Object> headers, CanUploadRequest canUploadRequest);
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.ToString;
import org.triplea.http.client.github.issues.GithubIssueClient;
Expand All @@ -17,6 +18,7 @@
public class ErrorReportRequest {
private String title;
private String body;
@Getter private String gameVersion;

public String getTitle() {
return title == null ? null : Ascii.truncate(title, GithubIssueClient.TITLE_MAX_LENGTH, "...");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public CreateIssueResponse newIssue(final ErrorReportRequest uploadRequest) {
githubOrg,
githubRepo,
CreateIssueRequest.builder()
.title(uploadRequest.getTitle())
.title(uploadRequest.getGameVersion() + ": " + uploadRequest.getTitle())
.body(uploadRequest.getBody())
.labels(new String[] {"Error Report"})
.build());
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package org.triplea.http.client.error.report;

import static com.github.tomakehurst.wiremock.client.WireMock.post;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;

import com.github.tomakehurst.wiremock.WireMockServer;
import com.github.tomakehurst.wiremock.client.WireMock;
import com.google.gson.Gson;
import java.net.URI;
import java.util.List;
Expand All @@ -19,6 +21,17 @@ class ErrorReportClientTest extends WireMockTest {
private static final ErrorReportResponse SUCCESS_RESPONSE =
ErrorReportResponse.builder().githubIssueLink(LINK).build();

private static final CanUploadErrorReportResponse CAN_UPLOAD_ERROR_REPORT_RESPONSE =
CanUploadErrorReportResponse.builder()
.responseDetails("details")
.canUpload(true)
.existingBugReportUrl("url")
.build();

private static ErrorReportClient newClient(final WireMockServer wireMockServer) {
return newClient(wireMockServer, ErrorReportClient::newClient);
}

@Test
void sendErrorReportSuccessCase(@WiremockResolver.Wiremock final WireMockServer server) {
final ErrorReportResponse response =
Expand Down Expand Up @@ -51,4 +64,21 @@ void errorHandling(@WiremockResolver.Wiremock final WireMockServer wireMockServe
HttpClientTesting.RequestType.POST,
ErrorReportClientTest::doServiceCall);
}

@Test
void canUploadErrorReport(@WiremockResolver.Wiremock final WireMockServer wireMockServer) {
wireMockServer.stubFor(
post(ErrorReportClient.CAN_UPLOAD_ERROR_REPORT_PATH)
.willReturn(
WireMock.aResponse()
.withStatus(200)
.withBody(toJson(CAN_UPLOAD_ERROR_REPORT_RESPONSE))));

final CanUploadErrorReportResponse response =
newClient(wireMockServer)
.canUploadErrorReport(
CanUploadRequest.builder().gameVersion("2.0").errorTitle("title").build());

assertThat(response, is(CAN_UPLOAD_ERROR_REPORT_RESPONSE));
}
}
Original file line number Diff line number Diff line change
@@ -1,19 +1,22 @@
package org.triplea.db.dao.error.reporting;

import java.time.Instant;
import java.util.Optional;
import org.jdbi.v3.sqlobject.customizer.Bind;
import org.jdbi.v3.sqlobject.customizer.BindBean;
import org.jdbi.v3.sqlobject.statement.SqlQuery;
import org.jdbi.v3.sqlobject.statement.SqlUpdate;

/** DAO class for error reporting functionality. */
public interface ErrorReportingDao {

/**
* Inserts a new record indicating a user has submitted an error report at a given date.
*
* @param userIp Ip host address of the user submitting an error report.
*/
@SqlUpdate("insert into error_report_history(user_ip) values(:ip)")
void insertHistoryRecord(@Bind("ip") String userIp);
/** Inserts a new record indicating a user has submitted an error report at a given date. */
@SqlUpdate(
"insert into error_report_history"
+ "(user_ip, system_id, report_title, game_version, created_issue_link) "
+ "values"
+ "(:ip, :systemId, :title, :gameVersion, :githubIssueLink)")
void insertHistoryRecord(@BindBean InsertHistoryRecordParams insertHistoryRecordParams);

/**
* Method to clean up old records from the error report history table. This is to avoid the table
Expand All @@ -23,4 +26,12 @@ public interface ErrorReportingDao {
*/
@SqlUpdate("delete from error_report_history where date_created < :purgeSinceDate")
void purgeOld(@Bind("purgeSinceDate") Instant purgeSinceDate);

@SqlQuery(
"select created_issue_link"
+ " from error_report_history"
+ " where report_title = :reportTitle "
+ " and game_version = :gameVersion")
Optional<String> getErrorReportLink(
@Bind("reportTitle") String reportTitle, @Bind("gameVersion") String gameVersion);
}
Loading

0 comments on commit 6af5f46

Please sign in to comment.