From bbb1c045c43220398916c00cec192d5f6b6def8b Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Tue, 13 Mar 2012 18:43:55 -0700 Subject: [PATCH 1/5] adds PackageRevision.extra_json * set using .set_extra_json() * value will be passed to simplejson.loads(), and be the base dict for which the manifest will be. generated properties override user defined properties. * includes database migration --- apps/jetpack/models.py | 22 +++++++++- apps/jetpack/tests/revision_tests.py | 28 ++++++++++++ apps/jetpack/tests/test_views.py | 44 +++++++++++++++++-- apps/jetpack/views.py | 8 ++++ .../032-add_packagerevision_extra_json.sql | 1 + 5 files changed, 97 insertions(+), 6 deletions(-) create mode 100644 migrations/032-add_packagerevision_extra_json.sql diff --git a/apps/jetpack/models.py b/apps/jetpack/models.py index 1e70966a..4b20370f 100644 --- a/apps/jetpack/models.py +++ b/apps/jetpack/models.py @@ -144,6 +144,9 @@ class PackageRevision(BaseModel): #: SDK which should be used to create the XPI sdk = models.ForeignKey('SDK', blank=True, null=True) + #: Extra package.json properties + extra_json = models.TextField(blank=True) + class Meta: " PackageRevision ordering and uniqueness " ordering = ('-revision_number',) @@ -528,8 +531,12 @@ class PackageRevision(BaseModel): name = self.name #if not self.package.is_addon(): # name = "%s-%s" % (name, self.package.id_number) + try: + manifest = simplejson.loads(self.extra_json) + except simplejson.JSONDecodeError: + manifest = {} - manifest = { + manifest.update({ 'fullName': self.full_name, 'name': name, 'description': escape(self.package.description), @@ -543,7 +550,7 @@ class PackageRevision(BaseModel): 'url': str(self.package.url), 'contributors': self.get_contributors_list(), 'lib': self.get_lib_dir() - } + }) if (self.package.is_library() and waffle.switch_is_active( 'LibDirInMainAttributeWorkaround')): @@ -778,6 +785,17 @@ class PackageRevision(BaseModel): return super(PackageRevision, self).save() + def set_extra_json(self, extra_json, save=True): + """ + Sets self.extra_json, adds commit message, and saves revision + by default. Pass save=False if you will save later. + """ + self.add_commit_message('Extra JSON properties changed') + self.extra_json = extra_json + if save: + self.save() + + def validate_module_filename(self, filename): """ returns False if the package revision contains a module with given diff --git a/apps/jetpack/tests/revision_tests.py b/apps/jetpack/tests/revision_tests.py index 772a93ba..74dae9de 100644 --- a/apps/jetpack/tests/revision_tests.py +++ b/apps/jetpack/tests/revision_tests.py @@ -354,6 +354,34 @@ class PackageRevisionTest(TestCase): ) self.assertRaises(FilenameExistException, first.attachment_add, att) + def test_adding_extra_package_properties(self): + addon = Package(type='a', author=self.author) + addon.save() + pk = addon.pk + rev = addon.latest + + rev.set_extra_json(''' + { + "preferences": [{ + "name": "example", + "type": "string", + "title": "foo", + "value": "bar" + }], + "id": "baz" + } + ''') + + addon = Package.objects.get(pk=pk) # breaking cache + manifest = addon.latest.get_manifest() + assert 'preferences' in manifest + eq_(manifest['preferences'][0]['name'], 'example') + + # user-provide values don't override our generated ones + self.assertNotEqual(manifest['id'], 'baz') + + assert 'Extra JSON' in addon.latest.commit_message + def test_add_commit_message(self): author = User.objects.all()[0] addon = Package(type='a', author=author) diff --git a/apps/jetpack/tests/test_views.py b/apps/jetpack/tests/test_views.py index af2e45c4..6fc8b30e 100644 --- a/apps/jetpack/tests/test_views.py +++ b/apps/jetpack/tests/test_views.py @@ -236,6 +236,13 @@ class TestEditing(TestCase): def setUp(self): self.hashtag = hashtag() + def _login(self): + self.author = User.objects.get(username='jan') + self.author.set_password('test') + self.author.save() + self.client.login(username=self.author.username, password='test') + return self.author + def test_revision_list_contains_added_modules(self): author = User.objects.get(username='john') addon = Package(author=author, type='a') @@ -251,13 +258,10 @@ class TestEditing(TestCase): assert 'test_filename' in r.content def test_package_name_change(self): - author = User.objects.get(username='jan') - author.set_password('secure') - author.save() + author = self._login() addon1 = Package(author=author, type='a') addon1.save() rev1 = addon1.latest - self.client.login(username=author.username, password='secure') response = self.client.post(addon1.latest.get_save_url(), { 'full_name': 'FULL NAME'}) eq_(response.status_code, 200) @@ -266,6 +270,38 @@ class TestEditing(TestCase): eq_(addon2.full_name, addon2.latest.full_name) assert rev1.name != addon2.latest.name + def test_package_extra_json_change(self): + author = self._login() + addon = Package(author=author, type='a') + addon.save() + pk = addon.pk + + homepage = 'https://builder.addons.mozilla.org' + extra_json = '{ "homepage": "%s" }' % homepage + response = self.client.post(addon.latest.get_save_url(), { + 'extra_json': extra_json}) + + addon = Package.objects.get(pk=pk) # old one is cached + + eq_(addon.latest.extra_json, extra_json) + + def test_package_remove_extra_json(self): + author = self._login() + addon = Package(author=author, type='a') + addon.save() + pk = addon.pk + + homepage = 'https://builder.addons.mozilla.org' + extra_json = '{ "homepage": "%s" }' % homepage + addon.latest.extra_json = extra_json + addon.latest.save() + + response = self.client.post(addon.latest.get_save_url(), { + 'extra_json': ''}) + + addon = Package.objects.get(pk=pk) # old on is cached + + eq_(addon.latest.extra_json, '') class TestRevision(TestCase): fixtures = ('mozilla_user', 'core_sdk', 'users', 'packages') diff --git a/apps/jetpack/views.py b/apps/jetpack/views.py index 09e6dd67..5f0b833b 100644 --- a/apps/jetpack/views.py +++ b/apps/jetpack/views.py @@ -900,6 +900,14 @@ def save(request, id_number, type_id, revision_number=None, revision.package.description = package_description response_data['package_description'] = package_description + extra_json = request.POST.get('extra_json') + if extra_json is not None: + # None means it wasn't submitted. We want to accept blank strings. + save_revision = True + revision.set_extra_json(extra_json, save=False) + response_data['extra_json'] = extra_json + + changes = [] for mod in revision.modules.all(): if request.POST.get(mod.filename, False): diff --git a/migrations/032-add_packagerevision_extra_json.sql b/migrations/032-add_packagerevision_extra_json.sql new file mode 100644 index 00000000..148044be --- /dev/null +++ b/migrations/032-add_packagerevision_extra_json.sql @@ -0,0 +1 @@ +ALTER TABLE `jetpack_packagerevision` ADD COLUMN `extra_json` TEXT; From 3dc2fb3ddf4b85bdb481f4a10c6800fe29f2d1de Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Wed, 21 Mar 2012 15:21:39 -0700 Subject: [PATCH 2/5] adds extra_json field, wrapped in ACE --- .../jetpack/templates/_edit_package_info.html | 8 ++- apps/jetpack/tests/test_views.py | 2 +- apps/jetpack/views.py | 4 +- media/jetpack/css/UI.Modal.css | 26 ++++++++ .../js/ide/controllers/PackageController.js | 61 ++++++++++++++++-- media/jetpack/js/ide/models/Package.js | 1 + media/jetpack/js/ide/views/JSONValidator.js | 23 +++++++ media/jetpack/js/ide/views/Validator.js | 63 ++++++++++--------- 8 files changed, 150 insertions(+), 38 deletions(-) create mode 100644 media/jetpack/js/ide/views/JSONValidator.js diff --git a/apps/jetpack/templates/_edit_package_info.html b/apps/jetpack/templates/_edit_package_info.html index aef80542..c460db36 100644 --- a/apps/jetpack/templates/_edit_package_info.html +++ b/apps/jetpack/templates/_edit_package_info.html @@ -1,9 +1,9 @@
-

Edit {full_name} info

+

Edit #{full_name} info

+
    diff --git a/apps/jetpack/tests/test_views.py b/apps/jetpack/tests/test_views.py index 6fc8b30e..92fcf997 100644 --- a/apps/jetpack/tests/test_views.py +++ b/apps/jetpack/tests/test_views.py @@ -279,7 +279,7 @@ class TestEditing(TestCase): homepage = 'https://builder.addons.mozilla.org' extra_json = '{ "homepage": "%s" }' % homepage response = self.client.post(addon.latest.get_save_url(), { - 'extra_json': extra_json}) + 'package_extra_json': extra_json}) addon = Package.objects.get(pk=pk) # old one is cached diff --git a/apps/jetpack/views.py b/apps/jetpack/views.py index 5f0b833b..6c596275 100644 --- a/apps/jetpack/views.py +++ b/apps/jetpack/views.py @@ -900,12 +900,12 @@ def save(request, id_number, type_id, revision_number=None, revision.package.description = package_description response_data['package_description'] = package_description - extra_json = request.POST.get('extra_json') + extra_json = request.POST.get('package_extra_json') if extra_json is not None: # None means it wasn't submitted. We want to accept blank strings. save_revision = True revision.set_extra_json(extra_json, save=False) - response_data['extra_json'] = extra_json + response_data['package_extra_json'] = extra_json changes = [] diff --git a/media/jetpack/css/UI.Modal.css b/media/jetpack/css/UI.Modal.css index d48d50b6..646453e0 100644 --- a/media/jetpack/css/UI.Modal.css +++ b/media/jetpack/css/UI.Modal.css @@ -287,3 +287,29 @@ authors: width:69%; padding:0 5px; } + +.UI_Modal .UI_Field.ace_editor_container { + position:relative; +} + .UI_Modal .UI_Field.ace_editor_container .ace_editor { + height:100px; + position:relative; + float:right; + width:260px; + } + + .UI_Modal .UI_Field.ace_editor_container .ace_editor .ace_gutter { + width:15px; + } + + .UI_Modal .UI_Field.ace_editor_container .validation-advice { + left:135px; + width:273px !important; + } + + .UI_Modal .UI_Field.ace_editor_container textarea { + width:10px; + height:30px; + float:none; + padding:0; + } diff --git a/media/jetpack/js/ide/controllers/PackageController.js b/media/jetpack/js/ide/controllers/PackageController.js index 50c1d85f..064ab557 100644 --- a/media/jetpack/js/ide/controllers/PackageController.js +++ b/media/jetpack/js/ide/controllers/PackageController.js @@ -18,6 +18,7 @@ var Class = require('shipyard/class/Class'), FloatingTips = require('../views/FloatingTips'), Validator = require('../views/Validator'), + JSONValidator = require('../views/JSONValidator'), //TODO: this is bad practice settings = dom.window.get('settings'); @@ -48,7 +49,11 @@ module.exports = new Class({ save_el: 'package-save', menu_el: 'UI_Editor_Menu', - package_info_form_elements: ['full_name', 'package_description'], + package_info_form_elements: [ + 'full_name', + 'package_description', + 'package_extra_json' + ], check_dependencies: true, check_if_latest: true // switch to false if displaying revisions @@ -1328,7 +1333,8 @@ module.exports = new Class({ this.savenow = false; var modal = fd().editPackageInfoModal = fd().displayModal( string.substitute(settings.edit_package_info_template, - object.merge({}, this.data, this.options))); + object.merge({}, this.data, this.options), + /#\{([^{}]+)\}/g)); modal.addListener('destroy', function() { delete fd().editPackageInfoModal; @@ -1339,6 +1345,8 @@ module.exports = new Class({ dom.$('package_description').addListener('change', function() { fd().emit('change'); }); + + var savenow = dom.$('savenow'); if (savenow) { savenow.addListener('click', function() { @@ -1362,16 +1370,31 @@ module.exports = new Class({ pressedBtn.addClass('pressed').getElement('a').addClass('inactive'); notPressedBtn.removeClass('pressed').getElement('a').removeClass('inactive'); - var validator = new Validator('full_name', { + var fullNameValidator = new Validator('full_name', { pattern: /^[A-Za-z0-9\s\-_\.\(\)]*$/, message: 'Please use only letters, numbers, spaces, or "_().-" in this field.' }); + var jsonValidator = new JSONValidator('package_extra_json', { + message: 'Must be blank or a valid JSON object.' + }); + + var package_extra_json_el = dom.$('package_extra_json'); + package_extra_json_el.store('json-validator', jsonValidator); + if ('package_extra_json' in this.options) { + package_extra_json_el.set('value', this.options.package_extra_json); + } + + this._setupExtraPropertiesEditor(); + + function isValid() { + return fullNameValidator.validate() && jsonValidator.validate(); + } dom.$('package-info_form').addListener('submit', function(e) { e.stop(); - if (validator.validate()) { + if (isValid()) { controller.submitInfo(); } else { - log.debug('Form field full_name field has invalid characters.'); + log.debug('Invalid name or JSON.'); } }); @@ -1384,6 +1407,34 @@ module.exports = new Class({ }); }, + _setupExtraPropertiesEditor: function() { + // Make package.json textarea an ACE editor + var ace = require('ace/ace'); + + var extra_json_el = dom.$('package_extra_json'); + extra_json_el.setStyle('display', 'none'); + var extra_json_editor_el = new dom.Element('div', { + id: 'extra_json_ace', + text: extra_json_el.get('value') + }); + extra_json_editor_el.inject(extra_json_el, 'before'); + var editor = ace.edit(extra_json_editor_el.getNode()); + var editorSession = editor.getSession(); + editorSession.on('change', function() { + extra_json_el.set('value', editorSession.getValue()); + fd().emit('change'); + }); + + var validator = extra_json_el.retrieve('json-validator'); + editor.on('blur', function() { + if (!validator.validate()) { + validator.show(); + } else { + validator.hide(); + } + }); + }, + /* * Method: submitInfo * submit info from EditInfoModalWindow diff --git a/media/jetpack/js/ide/models/Package.js b/media/jetpack/js/ide/models/Package.js index 9ad778b8..40293f1b 100644 --- a/media/jetpack/js/ide/models/Package.js +++ b/media/jetpack/js/ide/models/Package.js @@ -31,6 +31,7 @@ var Package = module.exports = new Class({ revision_number: fields.NumberField(), view_url: fields.TextField(), active: fields.BooleanField(), + extra_json: fields.TextField(), latest: fields.NumberField() // a FK to PackageRevision diff --git a/media/jetpack/js/ide/views/JSONValidator.js b/media/jetpack/js/ide/views/JSONValidator.js new file mode 100644 index 00000000..33a3c644 --- /dev/null +++ b/media/jetpack/js/ide/views/JSONValidator.js @@ -0,0 +1,23 @@ +var Class = require('shipyard/class/Class'), + typeOf = require('shipyard/utils/type').typeOf, + Validator = require('./Validator'); + +module.exports = new Class({ + + Extends: Validator, + + validate: function validate() { + var text = this.target.get('value').trim(); + if (text) { + try { + var json = JSON.parse(text); + // number, string, array, etc are illegal. + return typeOf(json) === 'object'; + } catch (jsonError) { + return false; + } + } + return true; + } + +}); diff --git a/media/jetpack/js/ide/views/Validator.js b/media/jetpack/js/ide/views/Validator.js index fb46d9d2..48828228 100644 --- a/media/jetpack/js/ide/views/Validator.js +++ b/media/jetpack/js/ide/views/Validator.js @@ -10,7 +10,12 @@ module.exports = new Class({ options: { pattern: /.*/, - message: 'Illegal characters found.' + messageTarget: null, + message: 'Illegal characters found.', + messageStyles: { + 'visibility': 'visible', + 'position': 'static' + } }, initialize: function Validator(element, options) { @@ -42,37 +47,39 @@ module.exports = new Class({ return this.options.pattern.test(value); }, + createElement: function() { + var messageTarget = dom.$(this.getOption('messageTarget')) || this.target; + this.element = new dom.Element('div', { + 'class': 'validation-advice', + 'text': this.getOption('message'), + 'styles': { + 'height': 0, + 'display': 'block', + 'overflow': 'hidden', + 'opacity': 0 + } + }); + + //measure the height + this.element.setStyles({ + 'visibility': 'hidden', + 'position': 'absolute' + }); + this.element.inject(messageTarget, 'after'); + this.height = this.element.getHeight(); + this.element.dispose().setStyles(this.getOption('messageStyles')); + + this.anim = new Anim(this.element, { + transition: Sine + }); + }, + show: function show() { var validator = this; if (!this.element) { - this.element = new dom.Element('div', { - 'class': 'validation-advice', - 'text': this.getOption('message'), - 'styles': { - 'height': 0, - 'display': 'block', - 'overflow': 'hidden', - 'opacity': 0 - } - }); - - //measure the height - this.element.setStyles({ - 'visibility': 'hidden', - 'position': 'absolute' - }); - this.element.inject(validator.target, 'after'); - this.height = this.element.getHeight(); - this.element.dispose().setStyles({ - visibility: 'visible', - position: 'static' - }); - - this.anim = new Anim(this.element, { - transition: Sine - }); - } + this.createElement(); + } this.anim.once('start', function() { validator.element.inject(validator.target, 'after'); From fc5a1d4731a7ad769f5a7c3789d962abd3bf5195 Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Thu, 22 Mar 2012 09:28:29 -0700 Subject: [PATCH 3/5] set_extra_json can raise JSONDecodeError. if it does, view returns HttpResponseBadRequest ( 400 code ) --- apps/jetpack/models.py | 7 +++++++ apps/jetpack/tests/revision_tests.py | 13 +++++++++++++ apps/jetpack/tests/test_views.py | 15 ++++++++++++++- apps/jetpack/views.py | 8 +++++++- 4 files changed, 41 insertions(+), 2 deletions(-) diff --git a/apps/jetpack/models.py b/apps/jetpack/models.py index 4b20370f..296440ec 100644 --- a/apps/jetpack/models.py +++ b/apps/jetpack/models.py @@ -789,8 +789,15 @@ class PackageRevision(BaseModel): """ Sets self.extra_json, adds commit message, and saves revision by default. Pass save=False if you will save later. + + raises JSONDecodeError """ self.add_commit_message('Extra JSON properties changed') + if extra_json: + # if not an empty string or None, just check it is + # valid JSON + simplejson.loads(extra_json) + self.extra_json = extra_json if save: self.save() diff --git a/apps/jetpack/tests/revision_tests.py b/apps/jetpack/tests/revision_tests.py index 74dae9de..d3015132 100644 --- a/apps/jetpack/tests/revision_tests.py +++ b/apps/jetpack/tests/revision_tests.py @@ -382,6 +382,19 @@ class PackageRevisionTest(TestCase): assert 'Extra JSON' in addon.latest.commit_message + def test_adding_invalid_extra_json(self): + addon = Package(type='a', author=self.author) + addon.save() + pk = addon.pk + rev = addon.latest + + from simplejson import JSONDecodeError + self.assertRaises(JSONDecodeError, rev.set_extra_json, ''' + { + foo: baz + } + ''') + def test_add_commit_message(self): author = User.objects.all()[0] addon = Package(type='a', author=author) diff --git a/apps/jetpack/tests/test_views.py b/apps/jetpack/tests/test_views.py index 92fcf997..c17b1082 100644 --- a/apps/jetpack/tests/test_views.py +++ b/apps/jetpack/tests/test_views.py @@ -297,12 +297,25 @@ class TestEditing(TestCase): addon.latest.save() response = self.client.post(addon.latest.get_save_url(), { - 'extra_json': ''}) + 'package_extra_json': ''}) addon = Package.objects.get(pk=pk) # old on is cached eq_(addon.latest.extra_json, '') + def test_package_invalid_extra_json(self): + author = self._login() + addon = Package(author=author, type='a') + addon.save() + + extra_json = '{ foo: bar }' + response = self.client.post(addon.latest.get_save_url(), { + 'package_extra_json': extra_json}) + + eq_(response.status_code, 400) + assert 'invalid JSON' in response.content + + class TestRevision(TestCase): fixtures = ('mozilla_user', 'core_sdk', 'users', 'packages') diff --git a/apps/jetpack/views.py b/apps/jetpack/views.py index 6c596275..f52f00e8 100644 --- a/apps/jetpack/views.py +++ b/apps/jetpack/views.py @@ -7,6 +7,7 @@ import shutil import codecs import tempfile import urllib2 +from simplejson import JSONDecodeError from django.contrib import messages from django.core.urlresolvers import reverse @@ -904,7 +905,12 @@ def save(request, id_number, type_id, revision_number=None, if extra_json is not None: # None means it wasn't submitted. We want to accept blank strings. save_revision = True - revision.set_extra_json(extra_json, save=False) + log.debug('extra_json: %s' % extra_json) + try: + revision.set_extra_json(extra_json, save=False) + except JSONDecodeError: + return HttpResponseBadRequest( + 'Extra package properties were invalid JSON.') response_data['package_extra_json'] = extra_json From ac1f75f214e7df1656fe2f93efda361c2267aa0e Mon Sep 17 00:00:00 2001 From: Piotr Zalewa Date: Thu, 22 Mar 2012 21:48:30 +0100 Subject: [PATCH 4/5] change the width of ace in Modal to line (almost) up with the textare --- media/jetpack/css/UI.Modal.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/media/jetpack/css/UI.Modal.css b/media/jetpack/css/UI.Modal.css index 646453e0..08f711f8 100644 --- a/media/jetpack/css/UI.Modal.css +++ b/media/jetpack/css/UI.Modal.css @@ -295,7 +295,7 @@ authors: height:100px; position:relative; float:right; - width:260px; + width: 70%; } .UI_Modal .UI_Field.ace_editor_container .ace_editor .ace_gutter { From 35e618f68172bcf2f99f3c2705978e8e7bce6e01 Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Tue, 27 Mar 2012 10:48:45 -0700 Subject: [PATCH 5/5] reseting validation styles back to default --- media/jetpack/css/UI.Modal.css | 4 ---- 1 file changed, 4 deletions(-) diff --git a/media/jetpack/css/UI.Modal.css b/media/jetpack/css/UI.Modal.css index 08f711f8..fe03db9e 100644 --- a/media/jetpack/css/UI.Modal.css +++ b/media/jetpack/css/UI.Modal.css @@ -302,10 +302,6 @@ authors: width:15px; } - .UI_Modal .UI_Field.ace_editor_container .validation-advice { - left:135px; - width:273px !important; - } .UI_Modal .UI_Field.ace_editor_container textarea { width:10px;