Skip to content
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.

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

Fixes #2659 - Make console logs in browser configuration details easier to read #3103

Merged
merged 7 commits into from
Dec 16, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
5 changes: 5 additions & 0 deletions grunt-tasks/cssmin.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ module.exports = function(grunt) {
"<%= cssPath %>/dist/webcompat.min.css": [
// input
"<%= cssPath %>/webcompat.dev.css"
],
// output
"<%= cssPath %>/dist/webcompat-logs.min.css": [
// input
"<%= cssPath %>/webcompat-logs.dev.css"
]
}
}
Expand Down
4 changes: 3 additions & 1 deletion grunt-tasks/cssnext.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ module.exports = function(grunt) {
dist: {
files: {
"<%= cssPath %>/dist/webcompat.min.css":
"<%= cssPath %>/webcompat.dev.css"
"<%= cssPath %>/webcompat.dev.css",
"<%= cssPath %>/dist/webcompat-logs.min.css":
"<%= cssPath %>/webcompat-logs.dev.css"
}
}
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
[
{
"level": "log",
"log": [
"test log"
],
"uri": "http://example.com/vendor.js",
"pos": "95:13"
},
{
"level": "warn",
"log": [
"test warn"
],
"uri": "http://example.com/",
"pos": "1:28535"
},
{
"level": "error",
"log": [
"test error"
],
"uri": "http://example.com/test.js?param=1&param22=13",
"pos": "1:1"
}
]
58 changes: 58 additions & 0 deletions tests/functional/console-logs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
"use strict";
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
const intern = require("intern").default;
const { assert } = intern.getPlugin("chai");
const { registerSuite } = intern.getInterface("object");
const FunctionalHelpers = require("./lib/helpers.js");

var url = function(path) {
return intern.config.siteRoot + path;
};

registerSuite("Console logs page", {
tests: {
"console log page loads with right data"() {
return FunctionalHelpers.openPage(
this,
url("/console_logs/2019/12/77d565bd-f1b4-4e76-a480-357f72df80b2"),
"#console_logs_data"
)
.findByCssSelector("#console_logs_data")
.getAttribute("data-file-id")
.then(function(text) {
assert.include(text, "7d565bd-f1b4-4e76-a480-357f72df80b2");
})
.getAttribute("data-subpath")
.then(function(text) {
assert.include(text, "2019/12");
})
.end()
.findByCssSelector(".level-log > .log-message")
.getVisibleText()
.then(function(text) {
assert.include(text, "test log");
})
.end()
.findByCssSelector(".level-log > .log-file")
.getVisibleText()
.then(function(text) {
assert.include(text, "vendor.js:95:13");
})
.end()
.findByCssSelector(".level-warn > .log-file")
.getVisibleText()
.then(function(text) {
assert.include(text, "http://example.com/:1:28535");
})
.end()
.findByCssSelector(".level-error > .log-file")
.getVisibleText()
.then(function(text) {
assert.include(text, "test.js:1:1");
})
.end();
}
}
});
95 changes: 95 additions & 0 deletions tests/unit/test_console_logs.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

'''Tests for image upload API endpoint.'''

import json
import os.path
import sys
import unittest

# Add webcompat module to import path
sys.path.append(os.path.realpath(os.pardir))

from webcompat import app # noqa

LOG = [
{
'level': 'log',
'log': [
'test log'
],
'uri': 'http://example.com/vendor.js',
'pos': '95:13'
},
{
'level': 'warn',
'log': [
'test warn'
],
'uri': 'http://example.com/test.js',
'pos': '1:28535'
}
]


def get_json_contents(filepath):
with open(filepath, 'r') as f:
json_contents = json.load(f)
return json_contents


def get_path(self, url):
return self.app.config['UPLOADS_DEFAULT_DEST'] + url + '.json'


def cleanup(filepath):
os.remove(filepath)


class TestConsoleUploads(unittest.TestCase):
def setUp(self):
app.config['TESTING'] = True
self.app = app
self.test_client = self.app.test_client()

def tearDown(self):
pass

def test_get(self):
'''Test that /console_logs/ doesn't let you GET.'''
rv = self.test_client.get('/console_logs/')
self.assertEqual(rv.status_code, 404)

def test_console_log_upload(self):
rv = self.test_client.post(
'/console_logs/', data=json.dumps(LOG))
self.assertEqual(rv.status_code, 201)

response = json.loads(rv.data)
self.assertTrue('url' in response)
cleanup(get_path(self, response.get('url')))

rv = self.test_client.post(
'/console_logs/', data='{test}')
self.assertEqual(rv.status_code, 400)

rv = self.test_client.post(
'/console_logs/', data='')
self.assertEqual(rv.status_code, 400)

def test_console_log_file_contents(self):
rv = self.test_client.post(
'/console_logs/', data=json.dumps(LOG))
response = json.loads(rv.data)
filepath = get_path(self, response.get('url'))
actual = get_json_contents(filepath)
self.assertEqual(actual, LOG)
cleanup(filepath)


if __name__ == '__main__':
unittest.main()
26 changes: 13 additions & 13 deletions tests/unit/test_form.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,37 +209,37 @@ def test_build_details(self):
# Test for receiving JSON object as a string
actual_json_arg = form.build_details(json.dumps(
{'a': 'b', 'c': False}))
expected_json_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>a: b</li><li>c: false</li>\n</ul>\n\n</details>' # noqa
expected_json_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>a: b</li><li>c: false</li>\n</ul>\n</details>' # noqa
self.assertEqual(actual_json_arg, expected_json_arg)
# Test for receiving a JSON value which is not an object
actual_json_arg = form.build_details('null')
expected_json_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>None</li>\n</ul>\n\n</details>' # noqa
expected_json_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>None</li>\n</ul>\n</details>' # noqa
self.assertEqual(actual_json_arg, expected_json_arg)
# Test for receiving a string
actual_string_arg = form.build_details('cool')
expected_string_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>cool</li>\n</ul>\n\n</details>' # noqa
expected_string_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>cool</li>\n</ul>\n</details>' # noqa
self.assertEqual(actual_string_arg, expected_string_arg)

def test_build_details_with_console_logs(self):
"""Expected HTML is returned for a json object with console logs."""
def test_build_details_without_console_logs(self):
"""Expected HTML is returned for a json object without console logs."""
actual_json_arg = form.build_details(json.dumps(
{'a': 'b', 'c': False, 'consoleLog': ['console.log(hi)']}))
expected_json_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>a: b</li><li>c: false</li>\n</ul>\n<p>Console Messages:</p>\n<pre>\n[\'console.log(hi)\']\n</pre>\n</details>' # noqa
expected_json_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>a: b</li><li>c: false</li>\n</ul>\n</details>' # noqa
self.assertEqual(actual_json_arg, expected_json_arg)
actual_empty_log_arg = form.build_details(json.dumps(
{'a': 'b', 'c': False, 'consoleLog': ''}))
expected_empty_log_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>a: b</li><li>c: false</li>\n</ul>\n\n</details>' # noqa
expected_empty_log_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n <li>a: b</li><li>c: false</li>\n</ul>\n</details>' # noqa
self.assertEqual(actual_empty_log_arg, expected_empty_log_arg)

def test_get_console_section(self):
"""Assert we return an empty string, or a pre with console messages."""
actual_empty_arg = form.get_console_section('')
def test_get_console_logs_url(self):
"""Assert we return an empty string, or a link to console logs page."""
actual_empty_arg = form.get_console_logs_url('')
expected_empty_arg = ''
self.assertEqual(actual_empty_arg, expected_empty_arg)
actual_none_arg = form.get_console_section(None)
actual_none_arg = form.get_console_logs_url(None)
self.assertEqual(actual_none_arg, expected_empty_arg)
actual_stringy_arg = form.get_console_section('sup')
expected_stringy_arg = '<p>Console Messages:</p>\n<pre>\nsup\n</pre>'
actual_stringy_arg = form.get_console_logs_url('some url')
expected_stringy_arg = '\n\n[View console log messages](some url)'
self.assertEqual(actual_stringy_arg, expected_stringy_arg)

def test_is_valid_issue_form(self):
Expand Down
5 changes: 5 additions & 0 deletions tests/unit/test_urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@ def test_contact(self):
rv = self.app.get('/contact')
self.assertEqual(rv.status_code, 200)

def test_console_logs(self):
"""Test that /console_logs exists."""
rv = self.app.get('/console_logs/11/20/1c0a7174-2c15-480f-9cee-e6183e6a781b') # noqa
Copy link
Member

Choose a reason for hiding this comment

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

note to self, we probably needs to create a console_logs directory

self.assertEqual(rv.status_code, 200)

def test_activity_page_401_if_not_logged_in(self):
"""Test that asks user to log in before displaying activity."""
rv = self.app.get('/me')
Expand Down
3 changes: 2 additions & 1 deletion webcompat/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@
from webcompat.api.endpoints import api # noqa
from webcompat.api.uploads import uploads # noqa
from webcompat.error_handlers import error_handlers # noqa
from webcompat.api.console_logs import console_logs # noqa

for blueprint in [api, error_handlers, uploads]:
for blueprint in [api, error_handlers, uploads, console_logs]:
app.register_blueprint(blueprint)


Expand Down
64 changes: 64 additions & 0 deletions webcompat/api/console_logs.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

'''Flask Blueprint for image uploads.'''

import datetime
import json
import os
import uuid

from flask import abort
from flask import Blueprint
from flask import request

from webcompat import app

console_logs = Blueprint('console_logs', __name__, url_prefix='/console_logs')
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I have a super strong opinion, but is there a reason you didn't just add support for uploading JSON content in the /uploads route? What are the advantages of having a new route to handle uploads of a different kind of file type?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. We should probably make the code versatile enough so that it can handle images and json.So it would be probably better to have the uploads route (blueprint) to behave differently with the content-type. So depending on the Content-Type, there is a validation process depending probably on the content of the file, size, etc. Like we do already for images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I've moved console log saving to uploads blueprint, could you please take another look @miketaylr @karlcow

JSON_MIME = 'application/json'


class UploadLog(object):
def __init__(self, data):
today = datetime.date.today()
self.data = data
self.year = str(today.year)
self.month = str(today.month)
self.file_id = str(uuid.uuid4())
self.file_path = self.get_path(self.month, self.year, self.file_id)

def get_path(self, month, year, file_id):
return os.path.join(year, month, file_id + '.json')

def get_url(self, file_path):
return os.path.dirname(file_path)\
+ '/' + os.path.splitext(os.path.basename(file_path))[0]

def save(self):
file_dest = app.config['UPLOADS_DEFAULT_DEST'] + self.file_path
dest_dir = os.path.dirname(file_dest)

if not os.path.exists(dest_dir):
os.makedirs(dest_dir)

with open(file_dest, 'w', encoding='utf-8') as f:
json.dump(self.data, f, ensure_ascii=False)


@console_logs.route('/', methods=['POST'])
def logs():
try:
content = json.loads(request.data)
log = UploadLog(content)
log.save()

data = {
'url': log.get_url(log.file_path)
}

return json.dumps(data), 201, {'content-type': JSON_MIME}
except ValueError:
abort(400)
Loading