Fixes #2985 - Add postMessage support for report site issue data #3012

ksy36 · 2019-10-22T19:43:43Z

These changes are to make the bug form accept metadata via postMessage from the report site issue addon in addition to screenshot blob. A message is expected in the following format:

{
    screenshot: blob, // image blob
    message: metadata // JSON-stringified metadata 
};

r? @miketaylr and @karlcow

webcompat/views.py

webcompat/static/js/lib/bugform-prefill.js

miketaylr

In general, LGTM -- one question I have:

Right now if it gets a message event with a message property, it will handle that data and stick it in the form. When do we send this extra data (beyond the screenshot)? It's outside of the scope of this PR, I think, but I want to make sure we're not sending the data via POST and then sending it again via postMessage.

miketaylr · 2019-10-22T20:48:37Z

webcompat/static/js/lib/bugform-prefill.js

+
+  var prepareValue = function(field, value) {
+    if (field.label && isExperiment) {
+      value.push("form-v2-experiment");


I like this.

ksy36 · 2019-10-22T22:06:56Z

I was testing it the following way:

When a new tab is open, don't send postData here.
Passed an object instead of an image blob here

        const data = {
          screenshot: blob,
          message: ${json}
        };
        postMessage(data, "${url.origin}");

So webcompat.com only receives a GET request in this case (with no parameters). And when the page is open it receives postMessage with metadata and screenshot

ksy36 · 2019-10-22T22:08:37Z

That makes me think that I need to account for two paths of handling data that comes in postMessage:

If it's an image blob (as it is now), don't fill the form and only upload image.
If it's an object, then fill the form and upload image.

That way it won't break for users that don't get updated version of the addon yet

karlcow

A couple of things here and there.
Thanks @ksy36 This is cool.

webcompat/helpers.py

tests/unit/test_helpers.py

tests/functional/reporting-postMessage.js

tests/unit/test_helpers.py

webcompat/views.py

karlcow · 2019-10-23T05:40:52Z

ah also @ksy36 forgotten, the format of commits is not in line with our guidelines. :)

ksy36 · 2019-10-25T20:29:09Z

sorry, didn't notice the commit format is different :(

Thanks for the feedback, I've addressed the comments here 4cafa8f

miketaylr

r+, just the one question about handling the screenshot and message below.

@ksy36 AIUI, this is safe to land as is, correct? -- do we have plans to change the implementation in Firefox to send both screenshot + details via postMessage? Or do we only want to do that for Fenix? (keeping both the same makes sense). If so, is there a bug on file to do that work?

miketaylr · 2019-10-25T20:39:36Z

webcompat/static/js/lib/bugform.js

+      event.data.hasOwnProperty("screenshot") ||
+      event.data.hasOwnProperty("message")
+    ) {
+      this.handleScreenshot(event.data.screenshot);


I'm not sure what the actual risk is, but in theory event.data could have a missing screenshot member, but have a message one. In that case, would this.handleScreenshot(event.data.screenshot); throw?

(Hard to say for sure, since right now Firefox doesn't send this object in this format currently (but I think you have local patches that do this).

edit: maybe the || is intended to be &&?

@ksy36 this is the last question I guess before merging.

Ahh thanks, I'll change this

Ahh thanks, I'll change this

cool! Feel free to merge when you're ready.

ksy36 · 2019-11-04T14:38:10Z

@miketaylr Yeah, it's safe to land as is. As for sending details (in addition to screenshot) via postMessage I've created a bug for the desktop reporter https://bugzilla.mozilla.org/show_bug.cgi?id=1593684

miketaylr · 2019-11-05T20:29:42Z

Note: had a conversation with @ksy36 about this, and she wants to hold off landing until she fixes a bug she found doing related work. We'll land this and the bugfix PR when it's ready.

…ck to the backend

webcompat/helpers.py

ksy36 · 2019-11-06T18:42:12Z

@karlcow , @miketaylr some more changes to review, sorry :)

miketaylr

I just reviewed the JS bits, LGTM. Will leave Python to @karlcow.

karlcow

A couple of questions and some nits.
Thanks @ksy36 for working on this.

karlcow · 2019-11-07T05:32:09Z

tests/unit/test_helpers.py

+
+    def test_get_extra_labels_for_experiment(self):
+        """Test extra_labels extraction from form object if
+        experiment is active."""


nit: one line for the docstring.

I had it in one line, but the linter complained that it's too long

You can fix that with a magic # noqa comment, but what would you recommend for a better docstring here @karlcow?

yes :)

"""Test extra_labels extraction when A/B experiment is active. And if you need to add more prose. Jump one line and add more stuff. :) """

see https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings

(A couple of hours later… doh… forgotten to push comment)

tests/unit/test_helpers.py

karlcow · 2019-11-07T05:49:43Z

webcompat/helpers.py

+
+    extra_labels = session.pop('extra_labels', None)
+
+    if not extra_labels and 'extra_labels' in form and form['extra_labels']:


@ksy36 This part of the code is slightly hard to read and I'm not sure it does what is expected.

>>> form = {'a': 1, 'b': 2, 'c': 3} >>> extra_labels = None >>> not extra_labels and 'b' in form and form['b'] 2 >>> not None True >>> 'b' in form True >>> form['b'] 2 >>> not None and True and 2 2

So for this:

if extra_labels is not in the session, I check whether 'extra_labels' property exists in form and that it's not an empty string, otherwise if passing an empty string to json.loads I get this error json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

basically this test case covers this:
self.assertEqual(get_extra_labels({'extra_labels': ''}), None)

but yeah it's hard to read, maybe I'll split this into separate functions

I've refactored this and added some more tests here aabf44f

See #3012 (comment)
and #3012 (comment)

karlcow

@ksy36 Another set of comments. Thanks for your patience.
I hope it helps.

ksy36 · 2019-11-08T16:05:19Z

Thanks for taking time to look into this @karlcow . This looks much shorter :)
It still errors out though, if extra_labels is an empty string, e.g.

>>> form = {'extra_labels': ''}
>>> json.loads(form.get('extra_labels', []))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

so perhaps, I could do:

 >>> json.loads(form.get('extra_labels', []) or '[]')
[]

So, it'll be

    if not extra_labels:
        extra_labels = json.loads(form.get('extra_labels', '[]') or '[]')

then my test passes

miketaylr · 2019-11-08T16:58:45Z

TIL that dict.get will return an empty string, rather than the default value. Using or [] like @ksy36 suggests seems like a good path forward.

ksy36 · 2019-11-11T01:32:13Z

Added that change, could you please take one more look @karlcow ?

karlcow · 2019-11-11T03:12:59Z

TIL that dict.get will return an empty string, rather than the default value. Using or [] like @ksy36 suggests seems like a good path forward.

I'm looking a last time at the full code right now.

karlcow

@ksy36 @miketaylr the tests are passing. I have a feeling we don't do the exact right thing.
I will close here. And I will open a new specific issue for optimizing this part of the code. Too many conversions where the initial goal is just to add the v2 form experiment. Something seems to be off.

Thanks @ksy36 for being patient with the review.

karlcow · 2019-11-11T03:46:58Z

@ksy36 once you have resolved the conflict we can merge.

ksy36 · 2019-11-11T20:43:03Z

cool, thanks @karlcow

…down, so all other tests are not affected by it

Issue #2985: Add postMessage support for report site issue data

aa2a47b

ksy36 commented Oct 22, 2019

View reviewed changes

webcompat/views.py Show resolved Hide resolved

ksy36 commented Oct 22, 2019

View reviewed changes

webcompat/static/js/lib/bugform-prefill.js Show resolved Hide resolved

Issue #2725: Python lint fix

acf0acc

ksy36 requested a review from karlcow October 22, 2019 19:55

miketaylr self-requested a review October 22, 2019 20:43

miketaylr reviewed Oct 22, 2019

View reviewed changes

karlcow requested changes Oct 23, 2019

View reviewed changes

karlcow changed the title ~~Fixes #2985: Add postMessage support for report site issue data~~ Fixes #2985 - Add postMessage support for report site issue data Oct 23, 2019

Issue #2985 - Code review fixes

4cafa8f

miketaylr approved these changes Oct 25, 2019

View reviewed changes

karlcow approved these changes Oct 27, 2019

View reviewed changes

Issue #2985 - Code review fixes, moved adding 'form-v2-experiment' ba…

6fdbe1a

…ck to the backend

ksy36 commented Nov 6, 2019

View reviewed changes

webcompat/helpers.py Show resolved Hide resolved

miketaylr approved these changes Nov 6, 2019

View reviewed changes

karlcow requested changes Nov 7, 2019

View reviewed changes

Issue #2985 - Code review fixes and more tests

aabf44f

karlcow requested changes Nov 8, 2019

View reviewed changes

Issue #2985 - Code review fixes

959f8f5

karlcow approved these changes Nov 11, 2019

View reviewed changes

karlcow mentioned this pull request Nov 11, 2019

Optimize the extra_labels code #3084

Open

Merge branch 'master' into issue/2985/1

b60a181

Issue #2985 - Fix test failure by resetting to default config in tear…

60f65f9

…down, so all other tests are not affected by it

ksy36 merged commit f71c931 into master Nov 12, 2019

ksy36 deleted the issue/2985/1 branch November 12, 2019 02:18

miketaylr added status: add to changelog and removed status: add to changelog labels Nov 12, 2019


		extra_labels = session.pop('extra_labels', None)

		if not extra_labels and 'extra_labels' in form and form['extra_labels']:

Fixes #2985 - Add postMessage support for report site issue data #3012

Fixes #2985 - Add postMessage support for report site issue data #3012

Conversation

ksy36 commented Oct 22, 2019 • edited Loading

miketaylr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksy36 commented Oct 22, 2019

ksy36 commented Oct 22, 2019

karlcow left a comment

Choose a reason for hiding this comment

karlcow commented Oct 23, 2019

ksy36 commented Oct 25, 2019

miketaylr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksy36 commented Nov 4, 2019

miketaylr commented Nov 5, 2019

ksy36 commented Nov 6, 2019

miketaylr left a comment

Choose a reason for hiding this comment

karlcow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlcow left a comment

Choose a reason for hiding this comment

ksy36 commented Nov 8, 2019 • edited Loading

miketaylr commented Nov 8, 2019

ksy36 commented Nov 11, 2019

karlcow commented Nov 11, 2019

karlcow left a comment

Choose a reason for hiding this comment

karlcow commented Nov 11, 2019

ksy36 commented Nov 11, 2019

ksy36 commented Oct 22, 2019 •

edited

Loading

ksy36 commented Nov 8, 2019 •

edited

Loading