Closed Bug 1332016 Opened 7 years ago Closed 7 years ago

Integrate the Socorro Lens into Bug Modal

Categories

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

Production
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dylan, Assigned: u279076)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Summary: Integrate the Soccoro Lens into Bug Modal → Integrate the Socorro Lens into Bug Modal
Adding Anthony to the CC list.
No longer blocks: bmo_csp_modal
Blocks: bmo_csp
I am not sure, but it is working well with the CDNs. Dylan, can you please confirm?
Attachment #8832375 - Flags: review?(dylan)
Attachment #8832375 - Flags: feedback?(anthony.s.hughes)
Hi Sebastian, what feedback were you looking for me specifically?
Hi Anthony,

Is this how socorro lens should look? Link: https://bmo-contrib.dur.me/socorro/socorro-lens.html
Attachment #8832375 - Flags: review?(dylan) → review-
(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.
Attached patch bmo-mg-integration-v1.patch (obsolete) — Splinter Review
Hi Dylan,

I've got this working now with functional parity to the add-on. Please review.
Assignee: sebastinssanty → anthony.s.hughes
Status: NEW → ASSIGNED
Attachment #8918402 - Flags: review?(dylan)
heh, reviewing this triggered a too large comment error. heh
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'
Attachment #8918402 - Flags: review?(dylan) → review-
Attachment #8832375 - Flags: feedback?(anthony.s.hughes)
(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.
Flags: needinfo?(dylan)
(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.
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.
Flags: needinfo?(dylan)
(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.
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.
Attached file PR
Attachment #8832375 - Attachment is obsolete: true
Attachment #8918402 - Attachment is obsolete: true
Attachment #8920693 - Flags: review?(dylan)
Comment on attachment 8920693 [details] [review]
PR

r=dylan
Attachment #8920693 - Flags: review?(dylan) → review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: User Interface: Modal → User Interface
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: