1332016 - Integrate the Socorro Lens into Bug Modal

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: User Interface, defect)

Description

[Dylan Hardison [:dylan] (he/him)]

The plugin just parses the crash signature
and then makes an iframe pointing to https://github.com/ashughes1/bugzilla-socorro-lens/blob/master/chart.html

https://github.com/ashughes1/bugzilla-socorro-lens/blob/master/addon/content.js#L12

ideally we would want to make that not an iframe, but realistically we can serve chart.html from BMO. That part's not so bad.

The bad part is we don't allow loading JS/CSS from CDNs, so we'd need to bundle that too.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Adding Anthony to the CC list.

Comment 2

[Sebastin Santy [:seban]]

Attachment: Patch: Integrates Socorro Lens with the bmo.

I am not sure, but it is working well with the CDNs. Dylan, can you please confirm?

Comment 3

[u279076]

Hi Sebastian, what feedback were you looking for me specifically?

Comment 4

[Sebastin Santy [:seban]]

Hi Anthony,

Is this how socorro lens should look? Link: https://bmo-contrib.dur.me/socorro/socorro-lens.html

Comment 5

[u279076]

(In reply to Sebastin Santy [:seban] from comment #4)
> Hi Anthony,
> 
> Is this how socorro lens should look? Link:
> https://bmo-contrib.dur.me/socorro/socorro-lens.html

Looks good to me.

Comment 6

[u279076]

Attachment: bmo-mg-integration-v1.patch

Hi Dylan,

I've got this working now with functional parity to the add-on. Please review.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

heh, reviewing this triggered a too large comment error. heh

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8918402 [details] [diff] [review]
bmo-mg-integration-v1.patch

Review of attachment 8918402 [details] [diff] [review]:
-----------------------------------------------------------------

Move extensions/MetricsGraphics/ to static/metricsgraphics/, there is no reason to keep this under extensions.

To make sure the (new) static directory is accessible, incorporate this patch:

diff --git a/Bugzilla/Install/Filesystem.pm b/Bugzilla/Install/Filesystem.pm
index 2a2e9b71a..a3191be33 100644
--- a/Bugzilla/Install/Filesystem.pm
+++ b/Bugzilla/Install/Filesystem.pm
@@ -302,6 +302,8 @@ sub FILESYSTEM {
                                      dirs => DIR_WS_SERVE },
          js                    => { files => WS_SERVE,
                                      dirs => DIR_WS_SERVE },
+         static                => { files => WS_SERVE,
+                                    dirs  => DIR_WS_SERVE },
          $skinsdir             => { files => WS_SERVE,
                                      dirs => DIR_WS_SERVE },
          'docs/*/html'         => { files => WS_SERVE,

::: extensions/BMO/template/en/default/hook/bug_modal/edit-custom_field-cf_crash_signature.html.tmpl
@@ +25,5 @@
>              [@ [% sig FILTER html %] ]
>            </a>
>            [%
> +          IF querystring == "";
> +            querystring = "$sig";

No need to do "$sig" here, can just write sig

@@ +27,5 @@
>            [%
> +          IF querystring == "";
> +            querystring = "$sig";
> +          ELSE;
> +            querystring = "$querystring\\\$sig";

I think this is a mistake -- one extra \ means it becomes literally '$sig'

Comment 9

[u279076]

(In reply to Dylan Hardison [:dylan] (he/him) from comment #8)
> @@ +27,5 @@
> >            [%
> > +          IF querystring == "";
> > +            querystring = "$sig";
> > +          ELSE;
> > +            querystring = "$querystring\\\$sig";
> 
> I think this is a mistake -- one extra \ means it becomes literally '$sig'

Hi Dylan, thanks for the review.

I've tested this and here's what I get...

With Crash Signatures == "[@ libart.so@0x337849 ]"
> querystring = "$querystring\\\$sig"; returns "libart.so@0x337849"
> querystring = "$querystring\\$sig"; returns "libart.so@0x337849"

With Crash Signatures == "[@ nvd3d9wrapx.dll@0x2e67 ][@ nvd3d9wrapx.dll@0x2ef7 ][@ nvd3d9wrapx.dll@0x2d37 ][@ nvd3d9wrapx.dll@0x5c5b ][@ nvd3d9wrapx.dll@0x3017 ][@ nvd3d9wrapx.dll@0x3c0b ][@ nvd3d9wrapx.dll@0x3c6b ][@ nvd3d9wrapx.dll@0x2d97 ]"
> querystring = "$querystring\\\$sig"; returns "nvd3d9wrapx.dll@0x2e67\\nvd3d9wrapx.dll@0x2ef7\\nvd3d9wrapx.dll@0x2d37\\nvd3d9wrapx.dll@0x5c5b\\nvd3d9wrapx.dll@0x3017\\nvd3d9wrapx.dll@0x3c0b\\nvd3d9wrapx.dll@0x3c6b\\nvd3d9wrapx.dll@0x2d97"
> querystring = "$querystring\\\$sig"; returns "nvd3d9wrapx.dll@0x2e67\\nvd3d9wrapx.dll@0x2ef7\\nvd3d9wrapx.dll@0x2d37\\nvd3d9wrapx.dll@0x5c5b\\nvd3d9wrapx.dll@0x3017\\nvd3d9wrapx.dll@0x3c0b\\nvd3d9wrapx.dll@0x3c6b\\nvd3d9wrapx.dll@0x2d97$sig"

(Chart.htm splits the querystring on '\\')

So it seems like the "correct" method you propose would result in an extraneous $sig in the last signature although it would work correctly with just one signature. Not sure why '\\\' works correctly in this case.

Comment 10

[u279076]

(In reply to Anthony Hughes (:ashughes) from comment #9)
> With Crash Signatures == "[@ nvd3d9wrapx.dll@0x2e67 ][@
> nvd3d9wrapx.dll@0x2ef7 ][@ nvd3d9wrapx.dll@0x2d37 ][@ nvd3d9wrapx.dll@0x5c5b
> ][@ nvd3d9wrapx.dll@0x3017 ][@ nvd3d9wrapx.dll@0x3c0b ][@
> nvd3d9wrapx.dll@0x3c6b ][@ nvd3d9wrapx.dll@0x2d97 ]"

> > querystring = "$querystring\\\$sig"; returns "nvd3d9wrapx.dll@0x2e67\\nvd3d9wrapx.dll@0x2ef7\\nvd3d9wrapx.dll@0x2d37\\nvd3d9wrapx.dll@0x5c5b\\nvd3d9wrapx.dll@0x3017\\nvd3d9wrapx.dll@0x3c0b\\nvd3d9wrapx.dll@0x3c6b\\nvd3d9wrapx.dll@0x2d97"
> > querystring = "$querystring\\\$sig"; returns "nvd3d9wrapx.dll@0x2e67\\nvd3d9wrapx.dll@0x2ef7\\nvd3d9wrapx.dll@0x2d37\\nvd3d9wrapx.dll@0x5c5b\\nvd3d9wrapx.dll@0x3017\\nvd3d9wrapx.dll@0x3c0b\\nvd3d9wrapx.dll@0x3c6b\\nvd3d9wrapx.dll@0x2d97$sig"
Sorry, that should be a \\ on the second line. 

> 
> (Chart.htm splits the querystring on '\\')
> 
> So it seems like the "correct" method you propose would result in an
> extraneous $sig in the last signature although it would work correctly with
> just one signature. Not sure why '\\\' works correctly in this case.

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Excellent! Aside from that it seems ready to test, if you can turn it into a PR I will push it up to bugzilla-dev and we can have other people test it too.

Comment 12

[u279076]

(In reply to Dylan Hardison [:dylan] (he/him) from comment #11)
> Excellent! Aside from that it seems ready to test, if you can turn it into a
> PR I will push it up to bugzilla-dev and we can have other people test it
> too.

Okay, I have submitted https://github.com/mozilla-bteam/bmo/pull/263 for review.

Comment 13

[Dylan Hardison [:dylan] (he/him)]

I made some tweaks you can merge into your PR (see https://github.com/ashughes1/bmo/pull/1) and I've pushed this up to bugzilla-dev.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Attachment: PR

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8920693 [details] [review]
PR

r=dylan