Merge lp:~bac/charmworld/bug-1257878 into lp:charmworld

[Brad Crittenden]

New QA assessment questions.

A migration is necessary to remove the old questions and answers and then to
reinstate the new ones. I've separated these two tasks into two different
migrations for easier testing.

Note the second one is forced to reach into Version and call the newly
refactored setup_qa_data because initialize is only called if the database has
not previously been versioned.

https://codereview.appspot.com/44070045/

R=rick, benji

New QA assessment questions.

A migration is necessary to remove the old questions and answers and then to
reinstate the new ones. I've separated these two tasks into two different
migrations for easier testing.

Note the second one is forced to reach into Version and call the newly
refactored setup_qa_data because initialize is only called if the database has
not previously been versioned.

https://codereview.appspot.com/44070045/

[Brad Crittenden (bac)]

Reviewers: mp+199713_code.launchpad.net,

Message:
Please take a look.

Description:
New QA assessment questions.

A migration is necessary to remove the old questions and answers and
then to
reinstate the new ones. I've separated these two tasks into two
different
migrations for easier testing.

Note the second one is forced to reach into Version and call the newly
refactored setup_qa_data because initialize is only called if the
database has
not previously been versioned.

https://code.launchpad.net/~bac/charmworld/bug-1257878/+merge/199713

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/44070045/

Affected files (+317, -132 lines):
   A [revision details]
   M charmworld/forms/qa_assessment.py
   M charmworld/migrations/migrate.py
   A charmworld/migrations/versions/022_Remove_all_QA_questions_and_data.py
   A charmworld/migrations/versions/023_insert_new_questions.py
   M charmworld/migrations/versions/tests/test_migrations.py
   M charmworld/qa_questions.py
   M charmworld/static/css/base.css
   M charmworld/templates/form/mapping_item.pt
   M charmworld/templates/qa_edit.pt
   M charmworld/testing/factory.py
   M charmworld/views/charms.py
   M docs/migrations.rst

[Richard Harding (rharding)]

Code looks good. QA'ing next.

https://codereview.appspot.com/44070045/diff/1/charmworld/migrations/versions/tests/test_migrations.py
File charmworld/migrations/versions/tests/test_migrations.py (right):

https://codereview.appspot.com/44070045/diff/1/charmworld/migrations/versions/tests/test_migrations.py#newcode148
charmworld/migrations/versions/tests/test_migrations.py:148: # Test
removal of QA questions and data. The new questions will be
double space

https://codereview.appspot.com/44070045/diff/1/charmworld/templates/form/mapping_item.pt
File charmworld/templates/form/mapping_item.pt (right):

https://codereview.appspot.com/44070045/diff/1/charmworld/templates/form/mapping_item.pt#newcode5
charmworld/templates/form/mapping_item.pt:5:
title='${field.description}'
any reason to change this one from " to '?

https://codereview.appspot.com/44070045/

[Benji York (benji)]

QA looks good.

review: Approve

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

=== modified file 'charmworld/forms/qa_assessment.py'
--- charmworld/forms/qa_assessment.py	2013-02-11 17:04:31 +0000
+++ charmworld/forms/qa_assessment.py	2013-12-19 20:19:08 +0000
@@ -40,25 +40,27 @@
         ('0', 'No'),
         ('', 'Unknown'),
     )
-    for q in score_questions:
-        category = q['name']
-        category_description = q['description']
-        questions = q['questions']
+    validator = OneOf([x[0] for x in choices])
+    for sq in score_questions:
+        category = sq['name']
+        category_description = sq['description']
+        questions = sq['questions']
 
         category_schema = Schema(
             name=category,
             title=category_description)
-        for q in questions:
-            category_schema.add(
-                SchemaNode(
-                    String(),
-                    missing='',
-                    validator=OneOf([x[0] for x in choices]),
-                    widget=RadioChoiceWidget(values=choices),
-                    name=q['id'],
-                    title=q['description'],
-                    description=q['extended_description'])
+
+        for question in questions:
+            node = SchemaNode(
+                String(),
+                missing='',
+                validator=validator,
+                widget=RadioChoiceWidget(values=choices),
+                name=question['id'],
+                title=question['description'],
+                description=question['extended_description'],
             )
+            category_schema.add(node)
 
         schema.add(category_schema)
 

=== modified file 'charmworld/migrations/migrate.py'
--- charmworld/migrations/migrate.py	2013-11-05 19:56:37 +0000
+++ charmworld/migrations/migrate.py	2013-12-19 20:19:08 +0000
@@ -147,10 +147,14 @@
             else:
                 pass  # Must be a helper file or something, let's ignore it.
 
+    @classmethod
+    def setup_qa_data(cls, db):
+        for category_dict in iter_categories():
+            db.qa.insert(category_dict)
+
     def initialize(self, datastore):
         datastore.version_datastore()
-        for category_dict in iter_categories():
-            datastore.db.qa.insert(category_dict)
+        self.setup_qa_data(datastore.db)
 
     def ensure_initialized(self, datastore, init=False):
         if datastore.current_version is not None:

=== added file 'charmworld/migrations/versions/022_Remove_all_QA_questions_and_data.py'
--- charmworld/migrations/versions/022_Remove_all_QA_questions_and_data.py	1970-01-01 00:00:00 +0000
+++ charmworld/migrations/versions/022_Remove_all_QA_questions_and_data.py	2013-12-19 20:19:08 +0000
@@ -0,0 +1,17 @@
+
+# Remove all QA questions and data
+
+def upgrade(db, index_client):
+    """Complete this function with work to be done for the migration/update.
+
+    db is the pymongo db instance for our datastore. Charms are in db.charms
+    for instance.
+
+    index_client is the ElasticSearch client.
+    """
+
+    # Remove the QA questions.
+    db.qa.remove()
+
+    # Remove the old QA answers.
+    db.charm_qa_answers.remove()

=== added file 'charmworld/migrations/versions/023_insert_new_questions.py'
--- charmworld/migrations/versions/023_insert_new_questions.py	1970-01-01 00:00:00 +0000
+++ charmworld/migrations/versions/023_insert_new_questions.py	2013-12-19 20:19:08 +0000
@@ -0,0 +1,13 @@
+from charmworld.migrations.migrate import Versions
+
+# insert new questions
+
+def upgrade(db, index_client):
+    """Complete this function with work to be done for the migration/update.
+
+    db is the pymongo db instance for our datastore. Charms are in db.charms
+    for instance.
+
+    index_client is the ElasticSearch client.
+    """
+    Versions.setup_qa_data(db)

=== modified file 'charmworld/migrations/versions/tests/test_migrations.py'
--- charmworld/migrations/versions/tests/test_migrations.py	2013-11-05 21:57:46 +0000
+++ charmworld/migrations/versions/tests/test_migrations.py	2013-12-19 20:19:08 +0000
@@ -6,9 +6,13 @@
 
 from charmworld.models import (
     Bundle,
+    Charm,
     CharmSource,
+    QAData,
+    QADataSource,
     store_bundles,
 )
+from charmworld import qa_questions
 from charmworld.search import (
     BUNDLE,
     IndexMissing,
@@ -139,6 +143,71 @@
             self.assertEqual(0, len(results[BUNDLE]))
 
 
+class TestMigration022(MigrationTestBase):
+
+    # Test removal of  QA questions and data.  The new questions will be
+    # updated in the next migration.
+
+    def setUp(self):
+        super(TestMigration022, self).setUp()
+        self.use_index_client()
+
+    def test_question_removal(self):
+        # Load question set.
+        category = factory.makeQAQuestionGroup()
+        self.db.qa.insert(category)
+        self.qa_data_source = QADataSource.from_db(self.db)
+        # Show that questions exist before running the migration.
+        qadata = QAData(self.qa_data_source)
+        self.assertListEqual(['reliable'], qadata.categories)
+        # Run the migration.
+        self.versions.run_migration(
+            self.db, self.index_client,
+            '022_Remove_all_QA_questions_and_data.py')
+        # And verify the data were removed.
+        qadata = QAData(self.qa_data_source)
+        self.assertListEqual([], qadata.categories)
+
+    def test_charm_score_removal(self):
+        charm_data = factory.makeCharm(self.db)[1]
+        charm = Charm(charm_data)
+        qa_data_source = QADataSource.from_db(self.db)
+        qa_data_source.save_qa_data(charm, {'a': 'b'})
+        answers = self.db.charm_qa_answers.find()
+        self.assertEqual(1, answers.count())
+        # Run the migration.
+        self.versions.run_migration(
+            self.db, self.index_client,
+            '022_Remove_all_QA_questions_and_data.py')
+        # And verify the data were removed.
+        answers = self.db.charm_qa_answers.find()
+        self.assertEqual(0, answers.count())
+
+
+class TestMigration023(MigrationTestBase):
+
+    # Test restoration of new QA questions.
+
+    def setUp(self):
+        super(TestMigration023, self).setUp()
+        self.use_index_client()
+
+    def test_questions_inserted(self):
+        # Load question set.
+        self.qa_data_source = QADataSource.from_db(self.db)
+        # Show that no questions exist before running the migration.
+        qadata = QAData(self.qa_data_source)
+        self.assertListEqual([], qadata.categories)
+        # Run the migration.
+        self.versions.run_migration(
+            self.db, self.index_client,
+            '023_insert_new_questions.py')
+        # And verify the data were removed.
+        qadata = QAData(self.qa_data_source)
+        self.assertEqual(7, len(qadata.categories))
+        expected = [cd['name'] for cd in qa_questions.iter_categories()]
+        self.assertListEqual(sorted(expected), sorted(qadata.categories))
+
 
 class TestExodusRestrictions(MigrationTestBase):
 

=== modified file 'charmworld/qa_questions.py'
--- charmworld/qa_questions.py	2013-09-10 13:54:39 +0000
+++ charmworld/qa_questions.py	2013-12-19 20:19:08 +0000
@@ -16,27 +16,113 @@
     for instance.
     """
 
+    # A list of tuples for question categories.
+    # (name, description, questions)
+    # where questions = (description, points, extended_description)
+
     initial = [
         (
+            u'data_handling',
+            u'Data Handling',
+            [
+                (u"Handles the service's user data", 1,
+                 u"Gracefully handle the service's user data so that the user "
+                  "doesn't have to manage it separately."),
+
+                (u'Provides backup mechanism', 1,
+                 u"Provide a backup mechanism to the service's user data, "
+                  "such as a relationship to a backup subordinate charm, "
+                  "snapshots to a bucket, or other means of getting the data "
+                  "off of the instance."),
+
+                (u'Provides a restore mechanism', 1,
+                 u"Provide a restore mechanism to the service's user data, "
+                  "such as a relationship to a backup subordinate charm, "
+                  "restoring from a bucket, or other means of getting data "
+                  "onto the instance."),
+
+                (u'Provides encryption', 1,
+                 u'Provides encryption of service user data.'),
+            ],
+        ),
+
+        (
+            u'secure',
+            u'Secure',
+            [
+                (u'Contains a well-tested '
+                  '<a href='
+                  '"https://help.ubuntu.com/12.04/serverguide/apparmor.html">'
+                  'AppArmor</a> profile', 1,
+                 u'These can be provided by the package itself.'),
+
+                (u"Doesn't run as root", 1,
+                 u'The service should not run as root. Refer to the '
+                  '<a href="http://upstart.ubuntu.com/cookbook/'
+                  '#run-a-job-as-a-different-user">'
+                  'Upstart documentation</a> for tips on how to do this.'),
+
+                (u'Per instance or service access control', 1,
+                 u'Accept relationships only from trusted instances and/or '
+                  'services.'),
+
+                (u'Defaults to secure communication', 1,
+                 u'Defaults to secure channels when communicating with other '
+                  'services and/or multiple units of the same service.'),
+            ],
+        ),
+
+        (
             u'reliable',
             u'Reliable',
             [
-                (u'Check for integrity from upstream source', 1, u''),
-                (u'Fail gracefully if upstream source goes missing', 1, u''),
-                (u'Contain a suite of tests with the charm that pass', 1, u''),
-                (u'Passes tests from Jenkins on jujucharms.com', 1, u''),
-            ]
-        ),
-
-        (
-            u'secure',
-            u'Secure',
-            [
-                (u'Contain a well tested AppArmor profile', 1, ''),
-                (u'Conform to security policies of the charm store', 1,
-                 'Tight access control'),
-                (u"Doesn't run as root", 1, ''),
-                (u'Per instance or service access control', 1, ''),
+                (u'Fails gracefully if upstream source goes missing', 1,
+                 u'Sometimes URLs change or upstream services are not '
+                  'available.'),
+
+                (u'Contains a suite of integration tests with the charm '
+                  'that pass',
+                 1,
+                 u'We provide <a href="howto-amulet.html">Amulet</a> '
+                  'to help you do this.'),
+
+                (u'Configuration options have safe defaults', 1, u''),
+            ]
+        ),
+
+        (
+            u'scaleable',
+            u'Scaleable',
+            [
+                (u"Responds to add-unit based on the service's needs", 1,
+                 u'Users should be able to <tt>juju add-unit</tt> to your '
+                  'service and have it scale horizontally.'),
+
+                (u"Responds to remove-unit based on the service's needs", 1,
+                 u'Users should be able to <tt>juju remove-unit</tt> to '
+                  'your service and have it scale down.'),
+
+                (u'Reuses existing charms for supporting services', 1,
+                 u'If the service needs relationships to other services it '
+                  'should reuse existing charms from the charm store instead '
+                  'of bundling its own.'),
+            ]
+        ),
+
+
+        (
+            u'upstream_friendly',
+            u'Upstream Friendly',
+            [
+                (u'Follow deployment recommendations from upstream best '
+                  'practices',
+                 1,
+                 u'Most services have a known-good recommendation from the '
+                  'project itself, these should be available for users'),
+
+                (u'Provide up to date versions of the upstream release', 1,
+                 u'Provide a config option to allow the user to run newer '
+                  'versions of the service.')
             ]
         ),
 
@@ -44,102 +130,73 @@
             u'flexible',
             u'Flexible',
             [
-                (
-                    u'Contain opinionated tuning options', 1,
-                    u'Examples (depends on the service): "safe", "default",'
-                    ' "fast", "real fast, not so safe". Don\'t expose every'
-                    ' configuration, pick those that reflect real world usage.'
-                    ' Make it so I don\'t have to read the book.'
-                ),
-                (u'Use existing interfaces with other charms', 1,
-                 u'Highly relatable'),
-            ]
-        ),
-
-        (
-            u'data_handling',
-            u'Data Handling',
-            [
-                (u'Integrate data storage best practices', 1,
-                 u'Backups based on service usage'),
-                (u"Handle the service's user data", 1, u'Version control'),
-                (u"Handle the service's user data", 1,
-                 u'Automated snapshots and backup.'),
-            ]
-        ),
-
-        (
-            u'scalable',
-            u'Scaleable',
-            [
-                (u"Responds to add-unit based on the service's needs", 1,
-                 u'Configuration should not require additional steps to scale'
-                 ' horizontally'),
-                (u'Be tested with a real workload, not just a synthetic'
-                 ' benchmark', 1, ''),
-                (u'From upstream and existing devops practices for that'
-                 ' service', 1, ''),
-                (u'Community peer reviewed', 1, ''),
-                (u'Have a configure option for most performant configuration'
-                 ' if not the default', 1, ''),
-            ]
-        ),
-
-        (
-            u'easy_deploy',
+                (u'Exposes version of service as a config option to allow '
+                  'easy upgrades',
+                 1,
+                 u'Allows users to maintain their deployments without '
+                  'having to redeploy.'),
+
+                (u'Adheres to the coding guidelines of the language your '
+                  'charm is written',
+                 1,
+                 u''),
+
+                (u'Exposed configurations are subsets of opinionated '
+                  'deployments',
+                 1,
+                 u'Don\'t expose every service option as configuration. '
+                  'Make sets of decisions and expose those as one '
+                  'configuration option.  For example "fast" vs. "default" '
+                  'vs. "slow-and-safer".'),
+
+                (u'Allow installation from the Ubuntu repository',
+                 1,
+                 u'Should be the default if the service is in Ubuntu.'),
+
+                (u'Allow installation from pure upstream source',
+                 1,
+                 u'Sometimes people prefer to deploy from pure upstream'),
+
+                (u'Allow installation from your local source',
+                 1,
+                 u'Sometimes people prefer to deploy from their vetted '
+                  'local source.'),
+
+                (u'Allow installation from PPA or other repository if '
+                  'available',
+                 1,
+                 u'Some upstreams run their own repositories '
+                  'that they gate and support.  Users should have an option '
+                  'of using these.')
+            ]
+        ),
+
+        (
+            u'easy_to_deploy',
             u'Easy to Deploy',
             [
-                (u'README with examples of use for a typical workload', 1, ''),
-                (u'README with examples of use for workloads at scale', 1, ''),
-                (u'README with examples of use recommend best-practice'
-                 ' relationships', 1, ''),
-                (u'Allow installation from pure upstream source', 1, ''),
-                (u'Allow installation from your local source', 1, ''),
-                (u'Allow installation from PPA (if available)', 1, ''),
-                (u'Allow installation from the Ubuntu repository', 1, ''),
-            ]
-        ),
-
-        (
-            u'responsive',
-            u'Responsive to DevOps Needs',
-            [
-                (u'Allow for easy upgrade via juju upgrade-charm', 1, ''),
-                (u'Allow upgrading the service itself.', 1, ''),
-                (u'Responsive to user bug reports and concerns', 1, ''),
-                (u'Maintainable, easy to read and modify', 1, ''),
-            ]
-        ),
-
-        (
-            u'upstream',
-            u'Upstream Friendly',
-            [
-                (u'Follow upstream best practices', 1,
-                 u'Provide an option for a barebones "pure upstream"'
-                 ' configuration'),
-                (u'Should go lock-step with deployment recommendations', 1,
-                 u'Provide tip-of-trunk testing if feasible'),
-                (u'Fresh charm on release day!', 1, ''),
-                (
-                    u"Endeavour to be upstream's recommended way to deploy"
-                    ' that service in the cloud (website mention or'
-                    ' something)', 1, ''
-                ),
+                (u'README with examples of use for a typical workload', 1,
+                 u''),
+                (u'README with examples of use for workloads at scale', 1,
+                 u''),
+                (u'README with examples of use of recommended best-practice '
+                  'relationships',
+                 1,
+                 u'')
             ]
         ),
     ]
 
     """Add the sample data into the db."""
-    for cat in initial:
+    for name, description, questions in initial:
         category_dict = {
-            'name': cat[0],
-            'description': cat[1],
+            'name': name,
+            'description': description,
             'questions': [{
-                'id': '{0}_{1}'.format(cat[0].lower(), i),
-                'description': q[0],
-                'points': q[1],
-                'extended_description': q[2]
-            } for i, q in enumerate(cat[2])]
+                'id': '{0}_{1}'.format(name.lower(), i),
+                'description': question[0],
+                'points': question[1],
+                'extended_description': question[2],
+            } for i, question in enumerate(questions)]
         }
         yield category_dict

=== modified file 'charmworld/static/css/base.css'
--- charmworld/static/css/base.css	2013-11-20 14:29:00 +0000
+++ charmworld/static/css/base.css	2013-12-19 20:19:08 +0000
@@ -42,3 +42,16 @@
     height: 120px;
     width: 120px;
 }
+
+.qa-edit legend {
+    font-weight: bold;
+}
+
+.qa-edit .help-block {
+    font-style: italic;
+    padding-bottom: 20px;
+}
+
+.qa-edit .control-label {
+    padding-top: 20px;
+}

=== modified file 'charmworld/templates/form/mapping_item.pt'
--- charmworld/templates/form/mapping_item.pt	2013-01-04 13:50:38 +0000
+++ charmworld/templates/form/mapping_item.pt	2013-12-19 20:19:08 +0000
@@ -2,7 +2,7 @@
                             field.widget.category == 'structural'"
      class="control-group ${field.error and 'error' or ''}"
      id="item-${field.oid}"
-     title="${field.description}"
+     title='${field.description}'
      tal:omit-tag="structural"
      i18n:domain="deform">
 

=== modified file 'charmworld/templates/qa_edit.pt'
--- charmworld/templates/qa_edit.pt	2013-01-21 14:01:03 +0000
+++ charmworld/templates/qa_edit.pt	2013-12-19 20:19:08 +0000
@@ -1,11 +1,13 @@
 <html tal:define="main load: main.pt" metal:use-macro="main.macros['page']">
     <body>
         <div class="container">
+          <div class="qa-edit">
             <metal:block metal:fill-slot="content">
                 <div tal:content="structure form">
                     form
                 </div>
             </metal:block>
+          </div>
         </div>
         <!-- /container -->
     </body>

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-11-18 20:38:47 +0000
+++ charmworld/testing/factory.py	2013-12-19 20:19:08 +0000
@@ -708,11 +708,11 @@
         'name': category[0],
         'description': category[1],
         'questions': [{
-            'id': hashlib.md5(q[0]).hexdigest(),
-            'description': q[0],
-            'points': q[1],
-            'extended_description': q[2]
-        } for q in category[2]]
+            'id': hashlib.md5(question[0]).hexdigest(),
+            'description': question[0],
+            'points': question[1],
+            'extended_description': question[2],
+        } for question in category[2]]
     }
 
     return category_dict

=== modified file 'charmworld/views/charms.py'
--- charmworld/views/charms.py	2013-10-17 15:35:53 +0000
+++ charmworld/views/charms.py	2013-12-19 20:19:08 +0000
@@ -5,6 +5,7 @@
     Form,
     ValidationFailure,
 )
+from HTMLParser import HTMLParser
 import json
 import os
 import pymongo
@@ -395,8 +396,9 @@
     qa_questions = request.db.qa.find()
     schema = get_qa_form_schema(qa_questions)
     qa_form = Form(schema, buttons=('submit',))
-    qa_form.css_class = 'form-horizontal well'
+    qa_form.css_class = 'form-horizontal well qa-edit'
     qa_data_source = QADataSource.from_db(request.db)
+
     if 'submit' in request.POST:
         data = request.POST.items()
         try:
@@ -409,11 +411,14 @@
             form = qa_form.render()
     else:
         qa_data = qa_data_source.get_qa_data(Charm(charm)).charm_data
-        if qa_data is not None:
-            # There's existing data to use to fill the form with.
-            valid_data = qa_data
-        form = qa_form.render(valid_data)
-
+        valid_data = qa_data or {}
+        if valid_data:
+            form = qa_form.render(valid_data)
+        else:
+            form = qa_form.render()
+    # The form rendering escapes all HTML in the text.  In order to have links
+    # in the QA descriptions and titles we must unescape it here.
+    form = HTMLParser().unescape(form)
     return {
         'charm': charm,
         'form': form,

=== modified file 'docs/migrations.rst'
--- docs/migrations.rst	2013-08-28 20:54:26 +0000
+++ docs/migrations.rst	2013-12-19 20:19:08 +0000
@@ -59,6 +59,7 @@
 ::
 
     $ bin/migrations prepare-upgrade --init
+    $ bin/migrations upgrade
     Updated the datastore to version: 1