Skip to content

Add Raven.showReportDialog (experimental) #456

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 8, 2016
Merged

Add Raven.showReportDialog (experimental) #456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26e7a83
Add Raven.showReportDialog (experimental)
benvinegar Jan 7, 2016
fbe377e
Can pass dsn to `showReportDialog`, add tests
benvinegar Jan 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions example/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
<head>
<title>Scratch Disk</title>
</head>
<script src="../vendor/TraceKit/tracekit.js"></script>
<script src="../src/raven.js"></script>
<script src="../dist/raven.js"></script>
<!-- <script src="scratch.min.js"></script> -->
<script src="scratch.js" crossorigin></script>
<script src="file.min.js" crossorigin></script>
Expand All @@ -14,7 +13,8 @@
//awesome
Raven.config('http://50dbe04cd1224d439e9c49bf1d0464df@localhost:8000/1', {
whitelistUrls: [
/localhost/
/localhost/,
/127\.0\.0\.1/
],
dataCallback: function(data) {
console.log(data);
Expand All @@ -25,7 +25,8 @@
Raven.setUserContext({
email: '[email protected]',
id: 5
})
});


</script>
<body>
Expand All @@ -36,6 +37,7 @@
<button onclick="derp()">window.onerror</button>
<button onclick="testOptions()">test options</button>
<button onclick="throwString()">throw string</button>
<button onclick="showDialog()">show dialog</button>

</body>
</html>
5 changes: 5 additions & 0 deletions example/scratch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,8 @@ function testOptions() {
function throwString() {
throw 'oops';
}

function showDialog() {
broken();
Raven.showReportDialog();
}
65 changes: 55 additions & 10 deletions src/raven.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ Raven.prototype = {
});
}

this._dsn = dsn;

// "Script error." is hard coded into browsers for errors that it can't read.
// this is the result of a script being pulled in from an external domain and CORS.
this._globalOptions.ignoreErrors.push(/^Script error\.?$/);
Expand All @@ -121,14 +123,10 @@ Raven.prototype = {
this._globalKey = uri.user;
this._globalProject = uri.path.substr(lastSlash + 1);

// assemble the endpoint from the uri pieces
this._globalServer = '//' + uri.host +
(uri.port ? ':' + uri.port : '') +
'/' + path + 'api/' + this._globalProject + '/store/';
this._globalServer = this._getGlobalServer(uri);

if (uri.protocol) {
this._globalServer = uri.protocol + ':' + this._globalServer;
}
this._globalEndpoint = this._globalServer +
'/' + path + 'api/' + this._globalProject + '/store/';

if (this._globalOptions.fetchContext) {
TraceKit.remoteFetching = true;
Expand Down Expand Up @@ -498,6 +496,41 @@ Raven.prototype = {
}
},

showReportDialog: function (options) {
if (!window.document) // doesn't work without a document (React native)
return;

options = options || {};

var lastEventId = options.eventId || this.lastEventId();
if (!lastEventId) {
throw new RavenConfigError('Missing eventId');
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably shouldn't throw errors like this since they'd then get caught and in turn reported to Sentry. Thoughts?

Or we have a rule to ignore RavenConfigError's. This wasn't an issue before because RavenConfigError was only ever thrown before raven was even hooked up. Now we'll be throwing errors that will be caught and logged.

Not sure if bad or not, just pointing it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't you want them reported to Sentry? It means that you misconfigured Raven (for dialog purposes) and you might never otherwise find out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that if you have Raven configured to accept an error, you will have set a DSN, and will thus never hit Missing DSN.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could see validity in the Missing DSN case, since that's before Raven is configured, but for missing event id... I'm not sure. I feel like we could just return a true/false instead for success/failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel it should be reported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's really hard to get into an infinite loop situation here, because when calling showReportDialog:

  1. If you don't have a DSN, then Raven wasn't installed and the global error handler isn't present – the exception is thrown but nothing catches it and control flow ends.

  2. If you don't have an eventId (or there was no lastEventId), then Raven will throw an exception, and this will be caught by the global error handler. Now if showReportDialog is called a second time, now there is a lastEventId and the function will no longer error.

Also, if you have an onerror handler, and inside that handler you throw another error (e.g. RavenConfigError), onerror doesn't get called again in an infinite loop – it just stops there:

http://jsbin.com/qoqoviduru/1/edit?html,js

The user would have to use a setTimeout call to leave the onerror stack to reach this infinite loop scenario:

http://jsbin.com/moxulabigi/1/edit?html,js,output (warning, this will freeze your browser)

But I mean, if they did this, any kind of bug in their code would trigger the infinite/setTimeout scenario – not strictly this RavenConfigError. So altogether, I think this scenario is really unlikely, and would be their fault anyways.

I'm going to move ahead with merging the branch. It's flagged experimental anyways.

}

var dsn = options.dsn || this._dsn;
if (!dsn) {
throw new RavenConfigError('Missing DSN');
}

var encode = encodeURIComponent;
var qs = '';
qs += '?eventId=' + encode(lastEventId);
qs += '&dsn=' + encode(dsn);

var user = options.user || this._globalContext.user;
if (user) {
Copy link
Member

Choose a reason for hiding this comment

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

needs to be configurable via options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would the DSN differ from what's specified via Raven.config?

Copy link
Member

Choose a reason for hiding this comment

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

because the DSN contains the project, and the eventID is bound to the project

often server side project != client side project

if (user.name) qs += '&name=' + encode(user.name);
if (user.email) qs += '&email=' + encode(user.email);
}

var globalServer = this._getGlobalServer(this._parseDSN(dsn));

var script = document.createElement('script');
script.async = true;
script.src = globalServer + '/api/embed/error-page/' + qs;
(document.head || document.body).appendChild(script);
},

/**** Private functions ****/
_ignoreNextOnError: function () {
var self = this;
Expand Down Expand Up @@ -683,6 +716,17 @@ Raven.prototype = {
return dsn;
},

_getGlobalServer: function(uri) {
// assemble the endpoint from the uri pieces
var globalServer = '//' + uri.host +
(uri.port ? ':' + uri.port : '');

if (uri.protocol) {
globalServer = uri.protocol + ':' + globalServer;
}
return globalServer;
},

_handleOnErrorStackInfo: function() {
// if we are intentionally ignoring errors via onerror, bail out
if (!this._ignoreOnError) {
Expand Down Expand Up @@ -933,8 +977,9 @@ Raven.prototype = {

if (!this.isSetup()) return;

var url = this._globalEndpoint;
(globalOptions.transport || this._makeRequest).call(this, {
url: this._globalServer,
url: url,
auth: {
sentry_version: '7',
sentry_client: 'raven-js/' + this.VERSION,
Expand All @@ -945,13 +990,13 @@ Raven.prototype = {
onSuccess: function success() {
self._triggerEvent('success', {
data: data,
src: self._globalServer
src: url
});
},
onError: function failure() {
self._triggerEvent('failure', {
data: data,
src: self._globalServer
src: url
});
}
});
Expand Down
93 changes: 85 additions & 8 deletions test/raven.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
var proxyquire = require('proxyquireify')(require);

var TraceKit = require('../vendor/TraceKit/tracekit');

var _Raven = proxyquire('../src/raven', {
'./utils': {
// patched to return a predictable result
Expand Down Expand Up @@ -1013,7 +1014,7 @@ describe('globals', function() {
maxMessageLength: 100,
release: 'abc123',
};
Raven._globalServer = 'http://localhost/store/';
Raven._globalEndpoint = 'http://localhost/store/';
Raven._globalOptions = globalOptions;

Raven._send({message: 'bar'});
Expand Down Expand Up @@ -1226,7 +1227,7 @@ describe('globals', function() {

it('should populate crossOrigin based on options', function() {
Raven._makeImageRequest({
url: Raven._globalServer,
url: Raven._globalEndpoint,
auth: {lol: '1'},
data: {foo: 'bar'},
options: {
Expand All @@ -1239,7 +1240,7 @@ describe('globals', function() {

it('should populate crossOrigin if empty string', function() {
Raven._makeImageRequest({
url: Raven._globalServer,
url: Raven._globalEndpoint,
auth: {lol: '1'},
data: {foo: 'bar'},
options: {
Expand All @@ -1252,7 +1253,7 @@ describe('globals', function() {

it('should not populate crossOrigin if falsey', function() {
Raven._makeImageRequest({
url: Raven._globalServer,
url: Raven._globalEndpoint,
auth: {lol: '1'},
data: {foo: 'bar'},
options: {
Expand Down Expand Up @@ -1487,7 +1488,7 @@ describe('Raven (public API)', function() {
Raven.afterLoad();

assert.equal(Raven._globalKey, 'random');
assert.equal(Raven._globalServer, 'http://some.other.server:80/api/2/store/');
assert.equal(Raven._globalEndpoint, 'http://some.other.server:80/api/2/store/');

assert.equal(Raven._globalOptions.some, 'config');
assert.equal(Raven._globalProject, '2');
Expand All @@ -1504,7 +1505,7 @@ describe('Raven (public API)', function() {
assert.equal(Raven, Raven.config(SENTRY_DSN, {foo: 'bar'}), 'it should return Raven');

assert.equal(Raven._globalKey, 'abc');
assert.equal(Raven._globalServer, 'http://example.com:80/api/2/store/');
assert.equal(Raven._globalEndpoint, 'http://example.com:80/api/2/store/');
assert.equal(Raven._globalOptions.foo, 'bar');
assert.equal(Raven._globalProject, '2');
assert.isTrue(Raven.isSetup());
Expand All @@ -1514,15 +1515,15 @@ describe('Raven (public API)', function() {
Raven.config('//[email protected]/2');

assert.equal(Raven._globalKey, 'abc');
assert.equal(Raven._globalServer, '//example.com/api/2/store/');
assert.equal(Raven._globalEndpoint, '//example.com/api/2/store/');
assert.equal(Raven._globalProject, '2');
assert.isTrue(Raven.isSetup());
});

it('should work should work at a non root path', function() {
Raven.config('//[email protected]/sentry/2');
assert.equal(Raven._globalKey, 'abc');
assert.equal(Raven._globalServer, '//example.com/sentry/api/2/store/');
assert.equal(Raven._globalEndpoint, '//example.com/sentry/api/2/store/');
assert.equal(Raven._globalProject, '2');
assert.isTrue(Raven.isSetup());
});
Expand Down Expand Up @@ -2028,4 +2029,80 @@ describe('Raven (public API)', function() {
assert.isFalse(Raven.isSetup());
});
});

describe('.showReportDialog', function () {
it('should throw a RavenConfigError if no eventId', function () {
assert.throws(function () {
Raven.showReportDialog({
dsn: SENTRY_DSN // dsn specified via options
});
}, 'Missing eventId');

Raven.config(SENTRY_DSN);
assert.throws(function () {
Raven.showReportDialog(); // dsn specified via Raven.config
}, 'Missing eventId');
});

it('should throw a RavenConfigError if no dsn', function () {
assert.throws(function () {
Raven.showReportDialog({
eventId: 'abc123'
});
}, 'Missing DSN');
});

describe('script tag insertion', function () {
beforeEach(function () {
this.appendChildStub = this.sinon.stub(document.head, 'appendChild');
});

it('should specify embed API endpoint and basic query string (DSN, eventId)', function () {
Raven.showReportDialog({
eventId: 'abc123',
dsn: SENTRY_DSN
});

var script = this.appendChildStub.getCall(0).args[0];
assert.equal(script.src, 'http://example.com/api/embed/error-page/?eventId=abc123&dsn=http%3A%2F%2Fabc%40example.com%3A80%2F2');

this.appendChildStub.reset();

Raven
.config(SENTRY_DSN)
.captureException(new Error('foo')) // generates lastEventId
.showReportDialog();

this.appendChildStub.getCall(0).args[0];
assert.equal(script.src, 'http://example.com/api/embed/error-page/?eventId=abc123&dsn=http%3A%2F%2Fabc%40example.com%3A80%2F2');
});

it('should specify embed API endpoint and full query string (DSN, eventId, user)', function () {
Raven.showReportDialog({
eventId: 'abc123',
dsn: SENTRY_DSN,
user: {
name: 'Average Normalperson',
email: '[email protected]'
}
});

var script = this.appendChildStub.getCall(0).args[0];
assert.equal(script.src, 'http://example.com/api/embed/error-page/?eventId=abc123&dsn=http%3A%2F%2Fabc%40example.com%3A80%2F2&name=Average%20Normalperson&email=an%40example.com');

this.appendChildStub.reset();
Raven
.config(SENTRY_DSN)
.captureException(new Error('foo')) // generates lastEventId
.setUserContext({
name: 'Average Normalperson 2',
email: '[email protected]'
})
.showReportDialog();

var script = this.appendChildStub.getCall(0).args[0];
assert.equal(script.src, 'http://example.com/api/embed/error-page/?eventId=abc123&dsn=http%3A%2F%2Fabc%40example.com%3A80%2F2&name=Average%20Normalperson%202&email=an2%40example.com');
});
});
});
});