This is an attempt to fix #117 and simplify the handling of properties and configuration. The idea is to split properties in three parts: - configuration, which is for projects only and is the only part that remains writable via the legacy API. However, the API call overwrites the configuration entirely, which simplifies the JavaScript code a bit (however, that part is untested) - flags, which are only for messages and are used for searches (again, mostly untested). I am using a not-particularly-efficient but simple text representation for them, hoping that indices save the day. - properties, which cover what's left and are basically the scribbling area for plugins. So now we have three concepts instead of one, but I think things do become simpler because we lose complicated cross-table manipulations in favor of just having some JSON stored in a field. In fact, if you do not consider migrations, the code loses a hundred lines or so. The old concept of properties also remains in the legacy API that is used by the importer. This however is limited to git.head and git.push_to; in order to drop that, we will have to switch to the REST API. The patch is on top of origin/next. Paolo Paolo Bonzini (10): api, patchew-cli: remove commands to directly access properties git: do not return properties from applier-get api: do not blindly return all properties from get-projects models: introduce flags into Messages mods: rename project_property_schema to project_config_schema mods: refactor extraction of configuration into a dictionary models: store plugin configuration in a single, separate JSONField models: add property fields to Message models: switch from property tables to field models: remove property tables api/admin.py | 14 +- api/migrations/0046_message_flags.py | 20 +++ .../0047_message_flags_postgres_fts.py | 22 +++ api/migrations/0048_populate_message_flags.py | 42 +++++ api/migrations/0049_project_config.py | 22 +++ .../0050_populate_project_config.py | 56 +++++++ api/migrations/0051_auto_20190418_1346.py | 33 ++++ .../0052_populate_property_fields.py | 67 ++++++++ api/migrations/0053_auto_20190418_1357.py | 37 +++++ api/models.py | 153 +++++++++--------- api/search.py | 15 +- api/views.py | 71 +++----- mod.py | 112 +++++-------- mods/email.py | 16 +- mods/git.py | 62 +++---- mods/tags.py | 5 +- mods/testing.py | 65 +++----- patchew-cli | 65 +------- tests/patchewtest.py | 12 +- tests/test_git.py | 11 +- tests/test_testing.py | 44 +++-- 21 files changed, 553 insertions(+), 391 deletions(-) create mode 100644 api/migrations/0046_message_flags.py create mode 100644 api/migrations/0047_message_flags_postgres_fts.py create mode 100644 api/migrations/0048_populate_message_flags.py create mode 100644 api/migrations/0049_project_config.py create mode 100644 api/migrations/0050_populate_project_config.py create mode 100644 api/migrations/0051_auto_20190418_1346.py create mode 100644 api/migrations/0052_populate_property_fields.py create mode 100644 api/migrations/0053_auto_20190418_1357.py -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel set-properties was completely unused; get-project-properties was not needed for the Javascript interface, only for the "project property" command. The Django admin is enough if one needs that kind of fine-grained access. --- api/views.py | 23 -------------------- patchew-cli | 61 ---------------------------------------------------- 2 files changed, 84 deletions(-) diff --git a/api/views.py b/api/views.py index XXXXXXX..XXXXXXX 100644 --- a/api/views.py +++ b/api/views.py @@ -XXX,XX +XXX,XX @@ class AddProjectView(APILoginRequiredView): p.save() -class GetProjectPropertiesView(APILoginRequiredView): - name = "get-project-properties" - - def handle(self, request, project): - po = Project.objects.get(name=project) - if not po.maintained_by(request.user): - raise PermissionDenied("Access denied to this project") - return po.get_properties() - - class UpdateProjectHeadView(APILoginRequiredView): name = "update-project-head" allowed_groups = ["importers"] @@ -XXX,XX +XXX,XX @@ class UpdateProjectHeadView(APILoginRequiredView): return ret -class SetPropertyView(APILoginRequiredView): - name = "set-properties" - allowed_groups = ["importers"] - - def handle(self, request, project, message_id, properties): - mo = Message.objects.filter(project__name=project, - message_id=message_id).first() - if not mo: - raise Http404("Message not found") - for k, v in properties.items(): - mo.set_property(k, v) - - class SetProjectPropertiesView(APILoginRequiredView): name = "set-project-properties" allowed_groups = ["maintainers"] diff --git a/patchew-cli b/patchew-cli index XXXXXXX..XXXXXXX 100755 --- a/patchew-cli +++ b/patchew-cli @@ -XXX,XX +XXX,XX @@ class ProjectCommand(SubCommand): finally: shutil.rmtree(wd) - def project_property(self, argv): - parser = argparse.ArgumentParser() - parser.add_argument("name", help="Name of the project") - parser.add_argument("prop", nargs="?", help="Name of the property") - parser.add_argument("--delete", "-d", action="store_true", - help="""delete the property with the given name. - Must give a property name""") - parser.add_argument("value", nargs="?", help="Value of the property to set") - args = parser.parse_args(argv) - if not args.value and not args.delete: - # Get property and print them or the specified one - r = self.api_do("get-project-properties", - project=args.name) - if not args.prop: - for k, v in iter(r.items()) if r else []: - print(k, v) - else: - if r and args.prop in r: - print(args.prop, r[args.prop]) - else: - print("Property Not found:", args.prop) - if r and list(r.keys()): - print("There are:", ", ".join(list(r.keys()))) - else: - if args.delete: - args.value = None - # Set property - self.api_do("set-project-properties", project=args.name, - properties={args.prop: args.value}) - def do(self, args, argv): if argv: if argv[0] == "add": return self.add_project(argv[1:]) - elif argv[0] == "property": - return self.project_property(argv[1:]) elif argv[0] == "info": return self.show_project(argv[1:]) elif argv[0] == "update": @@ -XXX,XX +XXX,XX @@ class UntestCommand(SubCommand): self.api_do("untest", terms=args.term) return 0 -class SetPropertyCommand(SubCommand): - name = "set-property" - want_argv = True - - def arguments(self, parser): - parser.add_argument("--project", "-p", required=True, - help="Project name") - parser.add_argument("--message-id", "-m", required=True, - help="Project name") - parser.add_argument("--file", action="store_true", - help="Read property values from given file") - parser.add_argument("--json", action="store_true", - help="Read property values as json") - - def do(self, args, argv): - if not argv: - return 0 - if len(argv) % 2: - print("Name and value unpaired:", argv[-1]) - props = dict(list(zip(argv[::2], argv[1::2]))) - if args.file: - props = dict([(k, open(v, "r").read()) for k, v in props.items()]) - if args.json: - props = dict([(k, json.loads(v)) for k, v in props.items()]) - self.api_do("set-properties", project=args.project, - message_id=args.message_id, - properties=props) - return 0 - class TesterCommand(SubCommand): name = "tester" want_argv = True -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Properties that are of interest to the applier are already returned directly in the applier-get reply; the applier is only using properties to retrieve tags from old servers that do not have commit 32bddb4 ("models: convert tags from property to field", 2018-10-30). It is not needed anymore, remove it to hide properties from the API. --- api/views.py | 14 ++++---------- patchew-cli | 4 +--- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/api/views.py b/api/views.py index XXXXXXX..XXXXXXX 100644 --- a/api/views.py +++ b/api/views.py @@ -XXX,XX +XXX,XX @@ class SetProjectPropertiesView(APILoginRequiredView): po.set_property(k, v) -def get_properties(m): - r = m.get_properties() - # for compatibility with patchew-cli's applier mode - if m.tags: - r['tags'] = m.tags - return r - - class DeleteProjectPropertiesByPrefixView(APILoginRequiredView): name = "delete-project-properties-by-prefix" allowed_groups = ["maintainers"] @@ -XXX,XX +XXX,XX @@ def prepare_patch(p): r = {"subject": p.subject, "message-id": p.message_id, "mbox": p.get_mbox(), - "properties": get_properties(p) + # For backwards compatibility with old clients + "properties": {} } return r @@ -XXX,XX +XXX,XX @@ def prepare_series(request, s, fields=None): if want_field("patches"): r["patches"] = [prepare_patch(x) for x in s.get_patches()] if want_field("properties"): - r["properties"] = get_properties(s) + # For backwards compatibility with old clients + r["properties"] = {} if want_field("tags"): r["tags"] = s.tags if want_field("is_complete"): diff --git a/patchew-cli b/patchew-cli index XXXXXXX..XXXXXXX 100755 --- a/patchew-cli +++ b/patchew-cli @@ -XXX,XX +XXX,XX @@ class ApplyCommand(SubCommand): subprocess.check_output(["git", "log", "-n", "1", "--format=%b"], cwd=repo) \ .decode('utf-8').splitlines() - # old servers do not have "tags" directly in the response - for t in set(p["properties"].get("tags", []) + p.get("tags", []) + \ - s["properties"].get("tags", []) + s.get("tags", [])): + for t in set(p.get("tags", []) + s.get("tags", [])): if t in commit_message_lines: continue filter_cmd += "echo '%s';" % t -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Only return the two properties that patchew-cli actually needs, through a new hook. --- api/views.py | 6 +++++- mods/git.py | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/api/views.py b/api/views.py index XXXXXXX..XXXXXXX 100644 --- a/api/views.py +++ b/api/views.py @@ -XXX,XX +XXX,XX @@ import json from .search import SearchEngine from django.views.decorators.csrf import csrf_exempt from django.utils.decorators import method_decorator +from mod import dispatch_module_hook class APIView(View): @@ -XXX,XX +XXX,XX @@ def prepare_project(p): "url": p.url, "git": p.git, "description": p.description, - "properties": p.get_properties(), + "properties": {}, } + dispatch_module_hook("get_projects_prepare_hook", project=p, + response=ret['properties']) + return ret diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): def rest_series_fields_hook(self, request, fields, detailed): fields['based_on'] = PluginMethodField(obj=self, required=False) + def get_projects_prepare_hook(self, project, response): + response["git.head"] = project.get_property("git.head") + response["git.push_to"] = project.get_property("git.push_to") + def prepare_message_hook(self, request, message, detailed): if not message.is_series_head: return -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Flags are used as a replacement for Boolean properties that are used by search queries. They can be searched efficiently using a trigram index. Unlike properties, flags are general and could be set by any plugin. For example, a GitHub integration webhook could set REVIEWED, OBSOLETE and TESTED based on changes made to a GitHub pull request. --- api/migrations/0046_message_flags.py | 20 +++++++++ .../0047_message_flags_postgres_fts.py | 22 ++++++++++ api/migrations/0048_populate_message_flags.py | 42 +++++++++++++++++++ api/models.py | 13 ++++++ api/search.py | 13 +++--- mods/tags.py | 5 ++- mods/testing.py | 22 +++++----- tests/test_testing.py | 9 ++-- 8 files changed, 124 insertions(+), 22 deletions(-) create mode 100644 api/migrations/0046_message_flags.py create mode 100644 api/migrations/0047_message_flags_postgres_fts.py create mode 100644 api/migrations/0048_populate_message_flags.py diff --git a/api/migrations/0046_message_flags.py b/api/migrations/0046_message_flags.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0046_message_flags.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-04-18 14:30 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0045_message_maintainers'), + ] + + operations = [ + migrations.AddField( + model_name='message', + name='flags', + field=models.CharField(blank=True, default='', max_length=64), + ), + ] diff --git a/api/migrations/0047_message_flags_postgres_fts.py b/api/migrations/0047_message_flags_postgres_fts.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0047_message_flags_postgres_fts.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.16 on 2018-11-07 15:28 +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion +from django.contrib.postgres.operations import TrigramExtension + +from api.migrations import PostgresOnlyMigration + + +class Migration(PostgresOnlyMigration): + + dependencies = [ + ('api', '0046_message_flags'), + ] + + operations = [ + TrigramExtension(), + migrations.RunSQL("create index api_message_flags_gin on api_message using gin(flags gin_trgm_ops);", + "drop index api_message_flags_gin"), + ] diff --git a/api/migrations/0048_populate_message_flags.py b/api/migrations/0048_populate_message_flags.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0048_populate_message_flags.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations + +from api.search import FLAG_TESTED, FLAG_REVIEWED, FLAG_OBSOLETE + +def property_to_flags(apps, schema_editor): + MessageProperty = apps.get_model('api', 'MessageProperty') + for p in MessageProperty.objects.filter(name='obsoleted-by'): + p.message.flags += FLAG_OBSOLETE + p.message.save() + for p in MessageProperty.objects.filter(name='reviewed'): + p.message.flags += FLAG_REVIEWED + p.message.save() + for p in MessageProperty.objects.filter(name='testing.done'): + p.message.flags += FLAG_TESTED + p.message.save() + MessageProperty.objects.filter(name='reviewed').delete() + MessageProperty.objects.filter(name='testing.done').delete() + +def flags_to_property(apps, schema_editor): + Message = apps.get_model('api', 'Message') + for m in Message.objects.exclude(flags=''): + if '[reviewed]' in m.flags: + new_prop = MessageProperty(message=p.message, name='reviewed', value=True) + new_prop.save() + if FLAG_TESTED in m.flags: + new_prop = MessageProperty(message=p.message, name='testing.done', value=True) + new_prop.save() + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0047_message_flags_postgres_fts'), + ] + + operations = [ + migrations.RunPython(property_to_flags, + reverse_code=flags_to_property), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class Message(models.Model): objects = MessageManager() maintainers = jsonfield.JSONField(blank=True, default=[]) + flags = models.CharField(max_length=64, blank=True, default="") def save_mbox(self, mbox_blob): save_blob(mbox_blob, self.message_id) @@ -XXX,XX +XXX,XX @@ class Message(models.Model): self.refresh_num_patches() return self.num_patches + def add_flag(self, flag): + assert flag[0] == '[' and flag[-1] == ']' + if not flag in self.flags: + self.flags += flag + self.save() + + def remove_flag(self, flag): + assert flag[0] == '[' and flag[-1] == ']' + if flag in self.flags: + self.flags = self.flags.replace(flag, '') + self.save() + def get_property(self, prop, default=None): return self.get_properties().get(prop, default) diff --git a/api/search.py b/api/search.py index XXXXXXX..XXXXXXX 100644 --- a/api/search.py +++ b/api/search.py @@ -XXX,XX +XXX,XX @@ from django.contrib.postgres.search import SearchQuery, SearchVector, SearchVect from django.db.models import Lookup from django.db.models.fields import Field +# These are used by migrations. Do not change them! +FLAG_REVIEWED = '[reviewed]' +FLAG_OBSOLETE = '[obsolete]' +FLAG_TESTED = '[tested]' @Field.register_lookup class NotEqual(Lookup): @@ -XXX,XX +XXX,XX @@ Search text keyword in the email message. Example: self._add_to_keywords('PULL') return Q(subject__contains='[PULL') | Q(subject__contains='[GIT PULL') elif cond == "reviewed": - return self._make_filter_subquery(MessageProperty, Q(name="reviewed", value=True)) + return Q(flags__contains=FLAG_REVIEWED) elif cond in ("obsoleted", "old"): - return self._make_filter_subquery( - MessageProperty, - Q(name="obsoleted-by", value__isnull=False) & ~Q(name="obsoleted-by", value__iexact='') - ) + return Q(flags__contains=FLAG_OBSOLETE) elif cond == "applied": return self._make_filter_subquery(MessageResult, Q(name="git", status=Result.SUCCESS)) elif cond == "tested": - return self._make_filter_subquery(MessageProperty, Q(name="testing.done", value=True)) + return Q(flags__contains=FLAG_TESTED) elif cond == "merged": return Q(is_merged=True) return None diff --git a/mods/tags.py b/mods/tags.py index XXXXXXX..XXXXXXX 100644 --- a/mods/tags.py +++ b/mods/tags.py @@ -XXX,XX +XXX,XX @@ from mbox import addr_db_to_rest, parse_address from event import register_handler, emit_event, declare_event from api.models import Message from api.rest import PluginMethodField +from api.search import FLAG_OBSOLETE, FLAG_REVIEWED import rest_framework REV_BY_PREFIX = "Reviewed-by:" @@ -XXX,XX +XXX,XX @@ series cover letter, patch mail body and their replies. return m1.version > m2.version and m1.date >= m2.date for m in series.get_alternative_revisions(): if newer_than(m, series): + series.add_flag(FLAG_OBSOLETE) series.set_property("obsoleted-by", m.message_id) elif newer_than(series, m): + m.add_flag(FLAG_OBSOLETE) m.set_property("obsoleted-by", series.message_id) updated = self.update_tags(series) @@ -XXX,XX +XXX,XX @@ series cover letter, patch mail body and their replies. series_reviewers = _find_reviewers(series) reviewers = reviewers.union(series_reviewers) if num_reviewed == series.get_num()[1] or series_reviewers: - series.set_property("reviewed", True) + series.add_flag(FLAG_REVIEWED) series.set_property("reviewers", list(reviewers)) if updated: emit_event("TagsUpdate", series=series) diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ from api.views import APILoginRequiredView from api.models import (Message, MessageProperty, MessageResult, Project, ProjectResult, Result) from api.rest import PluginMethodField, reverse_detail -from api.search import SearchEngine +from api.search import SearchEngine, FLAG_TESTED from event import emit_event, declare_event, register_handler from patchew.logviewer import LogView from schema import * @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): _instance.tester_check_in(po, result.data['tester']) if not self.get_testing_results(obj, status__in=(Result.PENDING, Result.RUNNING)).exists(): - obj.set_property("testing.done", True) obj.set_property("testing.tested-head", result.data["head"]) if isinstance(obj, Message): + obj.add_flag(FLAG_TESTED) obj.set_property("testing.tested-base", self.get_msg_base_tags(obj)) if isinstance(obj, Project): @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): for tn in all_tests: if not tn in done_tests: obj.create_result(name='testing.' + tn, status=Result.PENDING).save() - if len(done_tests) < len(all_tests): - obj.set_property("testing.done", None) - return - obj.set_property("testing.done", True) + if isinstance(obj, Message): + if len(all_tests) and len(done_tests) == len(all_tests): + obj.add_flag(FLAG_TESTED) + else: + obj.remove_flag(FLAG_TESTED) def project_recalc_pending_tests(self, project): self.recalc_pending_tests(project) @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): self.recalc_pending_tests(obj) def clear_and_start_testing(self, obj, test=""): - for k in list(obj.get_properties().keys()): - if k == "testing.done" or \ - k == "testing.tested-head": - obj.set_property(k, None) + obj.set_property("testing.tested-head", None) + if isinstance(obj, Message): + obj.remove_flag(FLAG_TESTED) if test: r = self.get_testing_result(obj, test) if r: @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): "type": "danger", "char": "T", }) - elif message.get_property("testing.done"): + elif FLAG_TESTED in message.flags: message.status_tags.append({ "title": "Testing passed", "url": reverse("series_detail", diff --git a/tests/test_testing.py b/tests/test_testing.py index XXXXXXX..XXXXXXX 100755 --- a/tests/test_testing.py +++ b/tests/test_testing.py @@ -XXX,XX +XXX,XX @@ import abc import subprocess from api.models import Message, Result +from api.search import FLAG_TESTED from .patchewtest import PatchewTestCase, main @@ -XXX,XX +XXX,XX @@ class TestingTestCase(PatchewTestCase, metaclass=abc.ABCMeta): if 'status' not in kwargs: kwargs['status'] = Result.SUCCESS self.modify_test_result(obj, **kwargs) - obj.set_property("testing.done", True) def do_testing_report(self, **report): self.api_login() @@ -XXX,XX +XXX,XX @@ class MessageTestingTest(TestingTestCase): def do_testing_done(self, **kwargs): self._do_testing_done(self.msg, **kwargs) + self.msg.add_flag(FLAG_TESTED) def do_testing_report(self, **report): r = super(MessageTestingTest, self).do_testing_report(**report) @@ -XXX,XX +XXX,XX @@ class TestingResetTest(PatchewTestCase): "testing.a": Result.SUCCESS, "testing.b": Result.SUCCESS, "testing.c": Result.FAILURE}) - self.assertTrue(msg.get_property("testing.done")) + self.assertIn(FLAG_TESTED, msg.flags) self.api_login() self.client.post('/login/', {'username': self.user, 'password': self.password}) @@ -XXX,XX +XXX,XX @@ class TestingResetTest(PatchewTestCase): "testing.a": Result.PENDING, "testing.b": Result.SUCCESS, "testing.c": Result.FAILURE}) - self.assertFalse(msg.get_property("testing.done")) + self.assertNotIn(FLAG_TESTED, msg.flags) self.client.get('/testing-reset/%s/?type=message&test=b' % msg.message_id) self.client.get('/testing-reset/%s/?type=message&test=c' % msg.message_id) @@ -XXX,XX +XXX,XX @@ class TestingResetTest(PatchewTestCase): "testing.a": Result.PENDING, "testing.b": Result.PENDING, "testing.c": Result.PENDING}) - self.assertFalse(msg.get_property("testing.done")) + self.assertNotIn(FLAG_TESTED, msg.flags) class TestingDisableTest(PatchewTestCase): -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel We would like to split plugin properties from configurations, and place them in two separate fields of the project. As a start, do not use "property" when referring to the module schemas. The fact that configuration is stored as a property is (more or less) an implementation detail. --- mod.py | 8 ++++---- mods/email.py | 2 +- mods/git.py | 2 +- mods/testing.py | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): """ Module base class """ name = None # The name of the module, must be unique default_config = "" # The default config string - project_property_schema = None + project_config_schema = None def get_model(self): # ALways read from DB to accept configuration update in-flight @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): assert False def build_config_html(self, request, project): - assert not isinstance(self.project_property_schema, StringSchema) - assert not isinstance(self.project_property_schema, IntegerSchema) - scm = self.project_property_schema + assert not isinstance(self.project_config_schema, StringSchema) + assert not isinstance(self.project_config_schema, IntegerSchema) + scm = self.project_config_schema tmpl = self._build_one(request, project, scm.name + ".", scm) tmpl += self._render_template(request, project, TMPL_END) return tmpl diff --git a/mods/email.py b/mods/email.py index XXXXXXX..XXXXXXX 100644 --- a/mods/email.py +++ b/mods/email.py @@ -XXX,XX +XXX,XX @@ Email information is configured in "INI" style: required=True), ]) - project_property_schema = \ + project_config_schema = \ ArraySchema("email", desc="Configuration for email module", members=[ MapSchema("notifications", "Email notifications", diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): allowed_groups = ('importers', ) result_data_serializer_class = ResultDataSerializer - project_property_schema = \ + project_config_schema = \ ArraySchema("git", desc="Configuration for git module", members=[ StringSchema("push_to", "Push remote", diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): required=True), ]) - project_property_schema = \ + project_config_schema = \ ArraySchema("testing", desc="Configuration for testing module", members=[ MapSchema("tests", "Tests", -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Both the email and testing modules have code to extract the configuration from project properties into a dictionary. Generalize that code, using the project_config_schema to drive the conversion, and use it in the git plugin as well This matches the way configuration will be stored in the database when we move away from project properties. In fact, all this nice visitor code will disappear very soon... --- mod.py | 37 ++++++++++++++++++++++++++++++++++++- mods/email.py | 14 +------------- mods/git.py | 28 ++++++++++++++++------------ mods/testing.py | 25 ++++--------------------- 4 files changed, 57 insertions(+), 47 deletions(-) diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): prefix = prefix + scm.name + "." def _build_map_items(): r = {} - for p, v in project.get_properties().items(): + for p in project.get_properties().keys(): if not p.startswith(prefix): continue name = p[len(prefix):] @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): tmpl += self._render_template(request, project, TMPL_END) return tmpl + def _get_map_scm(self, project, prop_name, scm): + prefix = prop_name + "." + result = {} + for p in project.get_properties().keys(): + if not p.startswith(prefix): + continue + name = p[len(prefix):] + name = name[:name.rfind(".")] + if name in result: + continue + assert scm.item.name == '{name}' + value = self._get_one(project, prefix + name, scm.item) + result[name] = value + return result + + def _get_array_scm(self, project, prop_name, scm): + prefix = prop_name + "." + result = {} + for i in scm.members: + assert i.name != '{name}' + result[i.name] = self._get_one(project, prefix + i.name, i) + return result + + def _get_one(self, project, prop_name, scm): + if type(scm) == MapSchema: + return self._get_map_scm(project, prop_name, scm) + elif type(scm) == ArraySchema: + return self._get_array_scm(project, prop_name, scm) + else: + return project.get_property(prop_name) + + def get_project_config(self, project): + scm = self.project_config_schema + return self._get_one(project, scm.name, scm) + _loaded_modules = {} def _module_init_config(cls): diff --git a/mods/email.py b/mods/email.py index XXXXXXX..XXXXXXX 100644 --- a/mods/email.py +++ b/mods/email.py @@ -XXX,XX +XXX,XX @@ Email information is configured in "INI" style: return "<%s@patchew.org>" % uuid.uuid1() def get_notifications(self, project): - ret = {} - for k, v in project.get_properties().items(): - if not k.startswith("email.notifications."): - continue - tn = k[len("email.notifications."):] - if "." not in tn: - continue - an = tn[tn.find(".") + 1:] - tn = tn[:tn.find(".")] - ret.setdefault(tn, {}) - ret[tn][an] = v - ret[tn]["name"] = tn - return ret + return self.get_project_config(project).get("notifications", {}) def on_event(self, event, **params): class EmailCancelled(Exception): diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): if series.is_complete: self.mark_as_pending_apply(series) - def get_project_config(self, project, what): - return project.get_property("git." + what) - def _is_repo(self, path): if not os.path.isdir(path): return False @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): def get_mirror(self, po, request, format): response = {} - for key, prop in (("head", "git.head"), - ("pushurl", "git.push_to"), - ("url", "git.public_repo")): - response[key] = po.get_property(prop) or None + config = self.get_project_config(po) + if "push_to" in config: + response["pushurl"] = config["push_to"] + if "public_repo" in config: + response["url"] = config["public_repo"] + head = po.get_property("git.head") + if head: + response["head"] = head return response def rest_project_fields_hook(self, request, fields): @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): def get_projects_prepare_hook(self, project, response): response["git.head"] = project.get_property("git.head") - response["git.push_to"] = project.get_property("git.push_to") + config = self.get_project_config(project) + if "push_to" in config: + response["git.push_to"] = config["push_to"] def prepare_message_hook(self, request, message, detailed): if not message.is_series_head: @@ -XXX,XX +XXX,XX @@ class ApplierGetView(APILoginRequiredView): "properties", "tags"]) po = m.project - for prop in ["git.push_to", "git.public_repo", "git.url_template"]: - if po.get_property(prop): - response[prop] = po.get_property(prop) + config = _instance.get_project_config(po) + for k, v in config.items(): + response["git." + k] = v base = _instance.get_base(m) if base: response["git.repo"] = base.data["repo"] @@ -XXX,XX +XXX,XX @@ class ApplierReportView(APILoginRequiredView): if url: data['url'] = url elif tag: - url_template = p.get_property("git.url_template") + config = _instance.get_project_config(po) + url_template = config.get("url_template") if url_template: data['url'] = url_template.replace("%t", tag) if base: diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): ret = {} if isinstance(obj, Message): obj = obj.project - for k, v in obj.get_properties().items(): - if not k.startswith("testing.tests."): - continue - tn = k[len("testing.tests."):] - if "." not in tn: - continue - an = tn[tn.find(".") + 1:] - tn = tn[:tn.find(".")] - ret.setdefault(tn, {}) - ret[tn][an] = v - ret[tn]["name"] = tn - return ret + return self.get_project_config(obj).get("tests", {}) def _build_reset_ops(self, obj): if isinstance(obj, Message): @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): project.extra_ops += self._build_reset_ops(project) def get_capability_probes(self, project): - ret = {} - for k, v in project.get_properties().items(): - prefix = "testing.requirements." - if not k.startswith(prefix): - continue - name = k[len(prefix):] - name = name[:name.find(".")] - ret[name] = v - return ret + props = self.get_project_config(project).get('requirements', {}) + return {k: v['script'] for k, v in props.items()} def get_testing_probes(self, project, request, format): return self.get_capability_probes(project) @@ -XXX,XX +XXX,XX @@ class TestingGetView(APILoginRequiredView): if req not in capabilities: break else: + t["name"] = tn yield r, t def _find_project_test(self, request, po, tester, capabilities): -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel This is the next step in the splitting the properties table into logically separate components. This time it's the turn of configuration data coming from the plugin schemas, which is extracted into a new JSON field. The configuration editor is simplified a bit because all the properties are sent to the server in a single shot and all configuration is overwritten. As a result, we no longer need the deletion API delete-project-properties-by-prefix. --- api/migrations/0049_project_config.py | 22 +++ .../0050_populate_project_config.py | 56 +++++++ api/models.py | 1 + api/views.py | 30 ++-- mod.py | 139 ++++-------------- mods/git.py | 27 ++-- mods/testing.py | 7 +- tests/patchewtest.py | 12 +- tests/test_git.py | 11 +- tests/test_testing.py | 35 +++-- 10 files changed, 185 insertions(+), 155 deletions(-) create mode 100644 api/migrations/0049_project_config.py create mode 100644 api/migrations/0050_populate_project_config.py diff --git a/api/migrations/0049_project_config.py b/api/migrations/0049_project_config.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0049_project_config.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-04-18 12:56 +from __future__ import unicode_literals + +from django.db import migrations +import jsonfield.encoder +import jsonfield.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0048_populate_message_flags'), + ] + + operations = [ + migrations.AddField( + model_name='project', + name='config', + field=jsonfield.fields.JSONField(default={}, dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, load_kwargs={}), + ), + ] diff --git a/api/migrations/0050_populate_project_config.py b/api/migrations/0050_populate_project_config.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0050_populate_project_config.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations +from django.db.models import Q + +def properties_to_config(apps, schema_editor): + Project = apps.get_model('api', 'Project') + for po in Project.objects.all(): + q = Q(name__startswith='testing.tests.') | \ + Q(name__startswith='testing.requirements.') | \ + Q(name__startswith='email.notifications.') | \ + Q(name__in=('git.push_to', 'git.url_template', 'git.public_repo')) + props = po.projectproperty_set.filter(q) + config = {} + for p in props: + *path, last = p.name.split('.') + parent = config + for item in path: + parent = parent.setdefault(item, {}) + parent[last] = p.value + #print(po, config) + po.config = config + po.save() + props.delete() + +def flatten_properties(source, prefix, result=None): + if result is None: + result = {} + for k, v in source.items(): + if isinstance(v, dict): + flatten_properties(v, prefix + k + '.', result) + else: + result[prefix + k] = v + return result + +def config_to_properties(apps, schema_editor): + Project = apps.get_model('api', 'Project') + ProjectProperty = apps.get_model('api', 'ProjectProperty') + for po in Project.objects.all(): + props = flatten_properties(po.config, '') + for k, v in props.items(): + new_prop = ProjectProperty(project=po, name=k, value=v) + new_prop.save() + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0049_project_config'), + ] + + operations = [ + migrations.RunPython(properties_to_config, + reverse_code=config_to_properties), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class Project(models.Model): "top project which has " "parent_project=NULL")) maintainers = models.ManyToManyField(User, blank=True) + config = jsonfield.JSONField(default={}) def __str__(self): return self.name diff --git a/api/views.py b/api/views.py index XXXXXXX..XXXXXXX 100644 --- a/api/views.py +++ b/api/views.py @@ -XXX,XX +XXX,XX @@ from django.contrib.auth import authenticate, login, logout from django.http import HttpResponse, Http404 from django.core.exceptions import PermissionDenied from django.conf import settings +from event import declare_event from .models import Project, Message import json from .search import SearchEngine @@ -XXX,XX +XXX,XX @@ class UpdateProjectHeadView(APILoginRequiredView): return ret -class SetProjectPropertiesView(APILoginRequiredView): - name = "set-project-properties" - allowed_groups = ["maintainers"] - - def handle(self, request, project, properties): - po = Project.objects.get(name=project) - if not po.maintained_by(request.user): - raise PermissionDenied("Access denied to this project") - for k, v in properties.items(): - po.set_property(k, v) +declare_event("SetProjectConfig", obj="project whose configuration was updated") -class DeleteProjectPropertiesByPrefixView(APILoginRequiredView): - name = "delete-project-properties-by-prefix" +class SetProjectConfigView(APILoginRequiredView): + name = "set-project-config" allowed_groups = ["maintainers"] - def handle(self, request, project, prefix): + def handle(self, request, project, config): po = Project.objects.get(name=project) if not po.maintained_by(request.user): raise PermissionDenied("Access denied to this project") - for k in [x for x in po.get_properties().keys() if x.startswith(prefix)]: - po.set_property(k, None) + new_config = {} + for k, v in properties.items(): + *path, last = k.split('.') + parent = new_config + for item in path: + parent = parent.setdefault(item, {}) + parent[last] = v + po.config = new_config + po.save() + emit_event("SetProjectConfig", obj=po) def prepare_patch(p): diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): data["module"] = self return Template(tmpl).render(Context(data)) - def _build_map_scm(self, request, project, prefix, scm): - prefix = prefix + scm.name + "." - def _build_map_items(): - r = {} - for p in project.get_properties().keys(): - if not p.startswith(prefix): - continue - name = p[len(prefix):] - name = name[:name.rfind(".")] - if name in r: - continue - pref = prefix + name + "." - r[name] = { - "name": name, - "html": self._build_one(request, project, - pref, scm.item) - } - return list(r.values()) - - schema_html = self._build_one(request, project, prefix, - scm.item) + def _build_map_scm(self, request, project, prefix, config, scm): + schema_html = self._build_one(request, project, prefix, scm.item) item = {"html": schema_html} - items = _build_map_items() + items = [{ + "name": name, + "html": self._build_one(request, project, prefix + name + ".", + value, scm.item)} for name, value in config.items()] return self._render_template(request, project, TMPL_MAP, schema=scm, item_schema=scm.item, @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): items=items, item=item) - def _build_array_scm(self, request, project, prefix, scm): + def _build_array_scm(self, request, project, prefix, config, scm, show_save_button): members = [self._build_one(request, project, - prefix, x) for x in scm.members] - show_save_button = False - for m in scm.members: - if type(m) == StringSchema: - show_save_button = True - break + prefix + x.name + ".", + config[x.name], x) for x in scm.members] return self._render_template(request, project, TMPL_ARRAY, schema=scm, members=members, show_save_button=show_save_button, prefix=prefix) - def _build_string_scm(self, request, project, prefix, scm): - prop_name = prefix + scm.name - prop_value = project.get_property(prop_name) + def _build_string_scm(self, request, project, prefix, config, scm): return self._render_template(request, project, TMPL_STRING, schema=scm, name=scm.name, prefix=prefix, - value=prop_value) + value=config) - def _build_integer_scm(self, request, project, prefix, scm): - prop_name = prefix + scm.name - prop_value = project.get_property(prop_name) + def _build_integer_scm(self, request, project, config, scm): return self._render_template(request, project, TMPL_INTEGER, schema=scm, name=scm.name, prefix=prefix, - value=prop_value) + value=config) - def _build_boolean_scm(self, request, project, prefix, scm): - prop_name = prefix + scm.name - prop_value = project.get_property(prop_name) + def _build_boolean_scm(self, request, project, config, scm): return self._render_template(request, project, TMPL_BOOLEAN, schema=scm, name=scm.name, prefix=prefix, - value=prop_value) + value=config) - def _build_enum_scm(self, request, project, prefix, scm): - prop_name = prefix + scm.name - prop_value = project.get_property(prop_name) + def _build_enum_scm(self, request, project, config, scm): return self._render_template(request, project, TMPL_ENUM, schema=scm, name=scm.name, prefix=prefix, - value=prop_value) + value=config) - def _build_one(self, request, project, prefix, scm): + def _build_one(self, request, project, prefix, config, scm, show_save_button=False): if type(scm) == MapSchema: - return self._build_map_scm(request, project, prefix, scm) + return self._build_map_scm(request, project, prefix, config, scm) elif type(scm) == StringSchema: - return self._build_string_scm(request, project, prefix, scm) + return self._build_string_scm(request, project, prefix, config, scm) elif type(scm) == IntegerSchema: - return self._build_integer_scm(request, project, prefix, scm) + return self._build_integer_scm(request, project, prefix, config, scm) elif type(scm) == BooleanSchema: - return self._build_boolean_scm(request, project, prefix, scm) + return self._build_boolean_scm(request, project, prefix, config, scm) elif type(scm) == EnumSchema: - return self._build_enum_scm(request, project, prefix, scm) + return self._build_enum_scm(request, project, prefix, config, scm) elif type(scm) == ArraySchema: - return self._build_array_scm(request, project, prefix, scm) + return self._build_array_scm(request, project, prefix, config, scm, show_save_button) assert False def build_config_html(self, request, project): - assert not isinstance(self.project_config_schema, StringSchema) - assert not isinstance(self.project_config_schema, IntegerSchema) + assert isinstance(self.project_config_schema, ArraySchema) scm = self.project_config_schema - tmpl = self._build_one(request, project, scm.name + ".", scm) + config = self.get_project_config(project) + tmpl = self._build_one(request, project, scm.name + ".", config, scm, True) tmpl += self._render_template(request, project, TMPL_END) return tmpl - def _get_map_scm(self, project, prop_name, scm): - prefix = prop_name + "." - result = {} - for p in project.get_properties().keys(): - if not p.startswith(prefix): - continue - name = p[len(prefix):] - name = name[:name.rfind(".")] - if name in result: - continue - assert scm.item.name == '{name}' - value = self._get_one(project, prefix + name, scm.item) - result[name] = value - return result - - def _get_array_scm(self, project, prop_name, scm): - prefix = prop_name + "." - result = {} - for i in scm.members: - assert i.name != '{name}' - result[i.name] = self._get_one(project, prefix + i.name, i) - return result - - def _get_one(self, project, prop_name, scm): - if type(scm) == MapSchema: - return self._get_map_scm(project, prop_name, scm) - elif type(scm) == ArraySchema: - return self._get_array_scm(project, prop_name, scm) - else: - return project.get_property(prop_name) - def get_project_config(self, project): - scm = self.project_config_schema - return self._get_one(project, scm.name, scm) + return project.config.get(self.project_config_schema.name, {}) _loaded_modules = {} @@ -XXX,XX +XXX,XX @@ function properties_save(btn) { $(btn).addClass("disabled"); $(btn).text("Saving..."); $(btn).parent().find(".save-message").remove(); - patchew_api_do("set-project-properties", + patchew_api_do("set-project-config", { project: "{{ project.name }}", - properties: props }) + config: props }) .done(function (data) { save_done(btn, true); }) @@ -XXX,XX +XXX,XX @@ function map_delete_item(btn) { if (!window.confirm("Really delete '" + name +"'?")) { return; } - $(btn).addClass("disabled"); - $(btn).text("Deleting..."); - $(btn).parent().find(".delete-message").remove(); - patchew_api_do("delete-project-properties-by-prefix", - { project: "{{ project.name }}", - prefix: prefix }) - .done(function (data) { - container = $(btn).parent().parent().parent(); - container.remove(); - }) - .fail(function (data, text, error) { - $(btn).removeClass("disabled"); - $(btn).text("Delete"); - info = $("<div class=\\"alert alert-danger delete-message\\"></div>"); - info.html("Error: " + error); - info.insertBefore($(btn)); - }); + container = $(btn).parent().parent().parent(); + container.remove(); } function enum_change(which) { val = $(which).val(); diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ class ApplierGetView(APILoginRequiredView): def handle(self, request, target_repo=None): q = Message.objects.filter(results__name="git", results__status="pending") if target_repo is not None and target_repo != '': - props = ProjectProperty.objects.filter(name='git.push_to') - # unfortunately startswith does not work with JSONFields, so we have to hack - # around it and look at the raw database representation. This would fail - # if we used PostgreSQL json fields! - target_repo_escaped = json.dumps(target_repo) - if target_repo[-1] != '/': - props = props.extra(where=["value = %s or value like %s"], - params=[target_repo_escaped, - target_repo_escaped[0:-1] + '/%']) - else: - props = props.extra(where=["value like %s"], - params=target_repo_escaped[0:-1] + '%') - q = q.filter(project__in=[prop.project for prop in props.all()]) + # Postgres could use JSON fields instead. Fortunately projects are + # few so this is cheap + def match_target_repo(config, target_repo): + push_to = config.get('git', {}).get('push_to') + if push_to is None: + return False + if target_repo[-1] != '/': + return push_to == target_repo or push_to.startswith(target_repo + '/') + else: + return push_to.startswith(target_repo) + + projects = Project.objects.values_list('id', 'config').all() + projects = [pid for pid, config in projects if match_target_repo(config, target_repo)] + q = q.filter(project__pk__in=projects) m = q.first() if not m: return None diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): html_log_url="URL to test log (HTML)", is_timeout="whether the test has timeout") register_handler("SetProperty", self.on_set_property) + register_handler("SetProjectConfig", self.on_set_config) register_handler("ResultUpdate", self.on_result_update) def on_set_property(self, evt, obj, name, value, old_value): if isinstance(obj, Project) and name == "git.head" \ and old_value != value: self.clear_and_start_testing(obj) - elif isinstance(obj, Project) and name.startswith("testing.tests.") \ - and old_value != value: - self.project_recalc_pending_tests(obj) + + def on_set_config(self, evt, obj): + self.project_recalc_pending_tests(obj) def get_msg_base_tags(self, msg): return [t for t in msg.tags if t.lower().startswith("based-on:")] diff --git a/tests/patchewtest.py b/tests/patchewtest.py index XXXXXXX..XXXXXXX 100644 --- a/tests/patchewtest.py +++ b/tests/patchewtest.py @@ -XXX,XX +XXX,XX @@ class PatchewTestCase(dj_test.LiveServerTestCase): def add_project(self, name, mailing_list="", git_repo=""): p = Project(name=name, mailing_list=mailing_list, git=git_repo or self.create_git_repo(name)) - p.save() push_repo = self.create_git_repo(name + "_push") - p.set_property("git.push_to", push_repo) - p.set_property("git.public_repo", push_repo) - p.set_property("git.url_template", push_repo) + p.config = { + "git": { + "push_to": push_repo, + "public_repo": push_repo, + "url_template": push_repo + } + } + p.save() return p def api_login(self): diff --git a/tests/test_git.py b/tests/test_git.py index XXXXXXX..XXXXXXX 100755 --- a/tests/test_git.py +++ b/tests/test_git.py @@ -XXX,XX +XXX,XX @@ class GitTest(PatchewTestCase): self.cli_login() self.repo = self.create_git_repo() self.p = self.add_project("QEMU", "qemu-devel@nongnu.org", git_repo=self.repo) - self.p.set_property("git.push_to", self.repo) - self.p.set_property("git.public_repo", self.repo) - self.p.set_property("git.url_template", self.repo + " %t") + self.p.config = { + "git": { + "push_to": self.repo, + "public_repo": self.repo, + "url_template": self.repo + " %t" + } + } + self.p.save() self.PROJECT_BASE = '%sprojects/%d/' % (self.REST_BASE, self.p.id) def cleanUp(self): diff --git a/tests/test_testing.py b/tests/test_testing.py index XXXXXXX..XXXXXXX 100755 --- a/tests/test_testing.py +++ b/tests/test_testing.py @@ -XXX,XX +XXX,XX @@ import subprocess from api.models import Message, Result from api.search import FLAG_TESTED +from event import emit_event from .patchewtest import PatchewTestCase, main def create_test(project, name, requirements="", script="#!/bin/bash\ntrue"): - prefix = "testing.tests." + name + "." - project.set_property(prefix + "timeout", 3600) - project.set_property(prefix + "enabled", True) - project.set_property(prefix + "script", script) - project.set_property(prefix + "requirements", requirements) + testing = project.config.setdefault('testing', {}) + tests = testing.setdefault('tests', {}) + tests[name] = { + 'timeout': 3600, + 'enabled': True, + 'script': script, + 'requirements': requirements + } + project.save() + emit_event("SetProjectConfig", obj=project) + + +def create_requirement(project, name, script="#!/bin/bash\ntrue"): + testing = project.config.setdefault('testing', {}) + requirements = testing.setdefault('requirements', {}) + requirements[name] = { + 'script': script, + } + project.save() class TestingTestCase(PatchewTestCase, metaclass=abc.ABCMeta): @@ -XXX,XX +XXX,XX @@ class TesterTest(PatchewTestCase): create_test(self.p2, "b") self.p3 = self.add_project("ALLOW", "qemu-devel@nongnu.org") - self.p3.set_property("testing.requirements.allow.script", - "#!/bin/sh\ntrue") + create_requirement(self.p3, 'allow', "#!/bin/sh\ntrue") create_test(self.p3, "c", "allow") self.p4 = self.add_project("DENY", "qemu-devel@nongnu.org") - self.p4.set_property("testing.requirements.deny.script", - "#!/bin/sh\nfalse") + create_requirement(self.p4, 'deny', "#!/bin/sh\nfalse") create_test(self.p4, "d", "deny") self.cli_login() @@ -XXX,XX +XXX,XX @@ class TestingDisableTest(PatchewTestCase): self.cli_login() self.cli_import('0013-foo-patch.mbox.gz') self.do_apply() - self.p1.set_property("testing.tests.a.enabled", False) + self.p1.config['testing']['tests']['a']['enabled'] = False + self.p1.save() + emit_event("SetProjectConfig", obj=self.p1) out, err = self.check_cli(["tester", "-p", "QEMU", "--no-wait"]) self.assertNotIn("Project: QEMU\n", out) self.cli_logout() -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel This is the first step towards storing properties in a single JSON dictionary, which is simpler now that properties are fewer and smaller than they used to be. The related name "properties" has to be freed up for the new field. --- api/migrations/0051_auto_20190418_1346.py | 33 +++++++++++++++++++++++ api/models.py | 9 ++++--- 2 files changed, 38 insertions(+), 4 deletions(-) create mode 100644 api/migrations/0051_auto_20190418_1346.py diff --git a/api/migrations/0051_auto_20190418_1346.py b/api/migrations/0051_auto_20190418_1346.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0051_auto_20190418_1346.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-04-18 13:46 +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion +import jsonfield.encoder +import jsonfield.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0050_populate_project_config'), + ] + + operations = [ + migrations.AddField( + model_name='message', + name='properties', + field=jsonfield.fields.JSONField(default={}, dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, load_kwargs={}), + ), + migrations.AddField( + model_name='project', + name='properties', + field=jsonfield.fields.JSONField(default={}, dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, load_kwargs={}), + ), + migrations.AlterField( + model_name='messageproperty', + name='message', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='api.Message'), + ), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class Project(models.Model): "parent_project=NULL")) maintainers = models.ManyToManyField(User, blank=True) config = jsonfield.JSONField(default={}) + properties = jsonfield.JSONField(default={}) def __str__(self): return self.name @@ -XXX,XX +XXX,XX @@ class MessageManager(models.Manager): return None else: q = super(MessageManager, self).get_queryset() - return q.filter(is_series_head=True).prefetch_related('properties', 'project') + return q.filter(is_series_head=True).prefetch_related('messageproperty_set', 'project') def find_series(self, message_id, project_name=None): heads = self.series_heads(project_name) @@ -XXX,XX +XXX,XX @@ class Message(models.Model): maintainers = jsonfield.JSONField(blank=True, default=[]) flags = models.CharField(max_length=64, blank=True, default="") + properties = jsonfield.JSONField(default={}) def save_mbox(self, mbox_blob): save_blob(mbox_blob, self.message_id) @@ -XXX,XX +XXX,XX @@ class Message(models.Model): # The prefetch cache is invalidated, query again all_props = MessageProperty.objects.filter(message=self) else: - all_props = self.properties.all() + all_props = self.messageproperty_set.all() r = {} for m in all_props: r[m.name] = m.value @@ -XXX,XX +XXX,XX @@ class MessageResult(Result): class MessageProperty(models.Model): - message = models.ForeignKey('Message', on_delete=models.CASCADE, - related_name='properties') + message = models.ForeignKey('Message', on_delete=models.CASCADE) name = models.CharField(max_length=256) value = jsonfield.JSONField() -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Fortunately, most accesses to the property tables were hidden behind accessors. Therefore, apart from the usual ugly migration (which in this case is more or less a copy of the config migration) it is almost enough to change the accessors to look into the JSON dictionary. We can also use the new API to access a whole subset of the properties, which simplifies the access to testing.check_in.*. --- .../0052_populate_property_fields.py | 67 +++++++++++ api/models.py | 109 ++++++++++-------- mods/testing.py | 6 +- 3 files changed, 128 insertions(+), 54 deletions(-) create mode 100644 api/migrations/0052_populate_property_fields.py diff --git a/api/migrations/0052_populate_property_fields.py b/api/migrations/0052_populate_property_fields.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0052_populate_property_fields.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations +from django.db.models import Q + +def do_properties_to_field(obj, propset): + properties = {} + props = propset.all() + for p in props: + *path, last = p.name.split('.') + parent = properties + for item in path: + parent = parent.setdefault(item, {}) + parent[last] = p.value + obj.properties = properties + #print(obj, properties) + obj.save() + props.delete() + +def properties_to_field(apps, schema_editor): + Project = apps.get_model('api', 'Project') + for po in Project.objects.all(): + do_properties_to_field(po, po.projectproperty_set) + Message = apps.get_model('api', 'Message') + for po in Message.objects.all(): + do_properties_to_field(po, po.messageproperty_set) + +def flatten_properties(source, prefix, result=None): + if result is None: + result = {} + for k, v in source.items(): + if isinstance(v, dict): + flatten_properties(v, prefix + k + '.', result) + else: + result[prefix + k] = v + return result + +def do_field_to_properties(source, propclass, **kwargs): + props = flatten_properties(source, '') + for k, v in props.items(): + #print(k, v) + new_prop = propclass(name=k, value=v, **kwargs) + new_prop.save() + +def field_to_properties(apps, schema_editor): + Project = apps.get_model('api', 'Project') + ProjectProperty = apps.get_model('api', 'ProjectProperty') + for po in Project.objects.all(): + do_field_to_properties(po.properties, ProjectProperty, project=po) + Message = apps.get_model('api', 'Message') + MessageProperty = apps.get_model('api', 'MessageProperty') + for m in Message.objects.all(): + do_field_to_properties(m.properties, MessageProperty, message=m) + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0051_auto_20190418_1346'), + ] + + operations = [ + migrations.RunPython(properties_to_field, + reverse_code=field_to_properties), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class Project(models.Model): return self.objects.filter(name=project).exists() def get_property(self, prop, default=None): - a = ProjectProperty.objects.filter(project=self, name=prop).first() - if a: - return a.value - else: - return default - - def get_properties(self): - r = {} - for m in ProjectProperty.objects.filter(project=self): - r[m.name] = m.value - return r - - def _do_set_property(self, prop, value): - if value is None: - ProjectProperty.objects.filter(project=self, name=prop).delete() + x = self.properties + *path, last = prop.split('.') + for item in path: + if not item in x: + return default + x = x[item] + return x.get(last, default) + + def delete_property(self, prop): + x = self.properties + *path, last = prop.split('.') + for item in path: + if not item in x: + return + x = x[item] + if not last in x: return - pp, created = ProjectProperty.objects.get_or_create(project=self, - name=prop) - pp.value = value - pp.save() + old_val = x[last] + del x[last] + self.save() + emit_event("SetProperty", obj=self, name=prop, value=None, + old_value=old_val) def set_property(self, prop, value): - old_val = self.get_property(prop) - self._do_set_property(prop, value) + if value is None: + self.delete_property(prop) + x = self.properties + *path, last = prop.split('.') + for item in path: + x = x.setdefault(item, {}) + old_val = x.get(last) + x[last] = value + self.save() emit_event("SetProperty", obj=self, name=prop, value=value, old_value=old_val) @@ -XXX,XX +XXX,XX @@ class Message(models.Model): self.save() def get_property(self, prop, default=None): - return self.get_properties().get(prop, default) - - def get_properties(self): - if hasattr(self, '_properties'): - if self._properties is not None: - return self._properties - else: - # The prefetch cache is invalidated, query again - all_props = MessageProperty.objects.filter(message=self) - else: - all_props = self.messageproperty_set.all() - r = {} - for m in all_props: - r[m.name] = m.value - self._properties = r - return r - - def _do_set_property(self, prop, value): - if value is None: - MessageProperty.objects.filter(message=self, name=prop).delete() + x = self.properties + *path, last = prop.split('.') + for item in path: + if not item in x: + return default + x = x[item] + return x.get(last, default) + + def delete_property(self, prop): + x = self.properties + *path, last = prop.split('.') + for item in path: + if not item in x: + return + x = x[item] + if not last in x: return - mp, created = MessageProperty.objects.get_or_create(message=self, - name=prop) - mp.value = value - mp.save() - # Invalidate cache - self._properties = None + old_val = x[last] + del x[last] + self.save() + emit_event("SetProperty", obj=self, name=prop, value=None, + old_value=old_val) def set_property(self, prop, value): - old_val = self.get_property(prop) - self._do_set_property(prop, value) + if value is None: + self.delete_property(prop) + x = self.properties + *path, last = prop.split('.') + for item in path: + x = x.setdefault(item, {}) + old_val = x.get(last) + x[last] = value + self.save() emit_event("SetProperty", obj=self, name=prop, value=value, old_value=old_val) diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): def check_active_testers(self, project): at = [] - for k, v in project.get_properties().items(): - prefix = "testing.check_in." - if not k.startswith(prefix): - continue + for tn, v in project.get_property('testing.check_in', {}).items(): age = time.time() - v if age > 10 * 60: continue - tn = k[len(prefix):] at.append("%s (%dmin)" % (tn, math.ceil(age / 60))) if not at: return -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel --- api/admin.py | 14 +-------- api/migrations/0053_auto_20190418_1357.py | 37 +++++++++++++++++++++++ api/models.py | 29 +----------------- api/search.py | 2 +- mods/git.py | 3 +- mods/testing.py | 3 +- 6 files changed, 42 insertions(+), 46 deletions(-) create mode 100644 api/migrations/0053_auto_20190418_1357.py diff --git a/api/admin.py b/api/admin.py index XXXXXXX..XXXXXXX 100644 --- a/api/admin.py +++ b/api/admin.py @@ -XXX,XX +XXX,XX @@ # http://opensource.org/licenses/MIT. from django.contrib import admin -from .models import Message, MessageProperty, Module, Project, ProjectProperty +from .models import Message, Module, Project from mod import get_module -class ProjectPropertyInline(admin.TabularInline): - model = ProjectProperty - extra = 0 - - class ProjectAdmin(admin.ModelAdmin): filter_horizontal = ('maintainers',) - inlines = [ProjectPropertyInline] - - -class MessagePropertyInline(admin.TabularInline): - model = MessageProperty - extra = 0 class MessageAdmin(admin.ModelAdmin): - inlines = [MessagePropertyInline] list_filter = [('is_series_head')] search_fields = [ 'message_id', diff --git a/api/migrations/0053_auto_20190418_1357.py b/api/migrations/0053_auto_20190418_1357.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0053_auto_20190418_1357.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-04-18 13:57 +from __future__ import unicode_literals + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0052_populate_property_fields'), + ] + + operations = [ + migrations.AlterUniqueTogether( + name='messageproperty', + unique_together=set([]), + ), + migrations.RemoveField( + model_name='messageproperty', + name='message', + ), + migrations.AlterUniqueTogether( + name='projectproperty', + unique_together=set([]), + ), + migrations.RemoveField( + model_name='projectproperty', + name='project', + ), + migrations.DeleteModel( + name='MessageProperty', + ), + migrations.DeleteModel( + name='ProjectProperty', + ), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class ProjectResult(Result): return self.project -class ProjectProperty(models.Model): - project = models.ForeignKey('Project', on_delete=models.CASCADE) - name = models.CharField(max_length=1024, db_index=True) - value = jsonfield.JSONField() - - class Meta: - unique_together = ('project', 'name',) - verbose_name_plural = "Project Properties" - - declare_event("SeriesComplete", project="project object", series="series instance that is marked complete") declare_event("SeriesMerged", project="project object", @@ -XXX,XX +XXX,XX @@ class MessageManager(models.Manager): return None else: q = super(MessageManager, self).get_queryset() - return q.filter(is_series_head=True).prefetch_related('messageproperty_set', 'project') + return q.filter(is_series_head=True).prefetch_related('project') def find_series(self, message_id, project_name=None): heads = self.series_heads(project_name) @@ -XXX,XX +XXX,XX @@ class MessageResult(Result): return self.message -class MessageProperty(models.Model): - message = models.ForeignKey('Message', on_delete=models.CASCADE) - name = models.CharField(max_length=256) - value = jsonfield.JSONField() - - def __str__(self): - if len(self.value) > 30: - val_prev = self.value[:30] + "..." - else: - val_prev = self.value - return "%s: %s = %s" % (self.message.subject, self.name, val_prev) - - class Meta: - unique_together = ('message', 'name',) - verbose_name_plural = "Message Properties" - - class Module(models.Model): """ Module information """ name = models.CharField(max_length=128, unique=True) diff --git a/api/search.py b/api/search.py index XXXXXXX..XXXXXXX 100644 --- a/api/search.py +++ b/api/search.py @@ -XXX,XX +XXX,XX @@ # This work is licensed under the MIT License. Please see the LICENSE file or # http://opensource.org/licenses/MIT. -from .models import Message, MessageProperty, MessageResult, Result, QueuedSeries +from .models import Message, MessageResult, Result, QueuedSeries from functools import reduce from django.db import connection diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ from django.utils.html import format_html from django.db.models import Q from mod import PatchewModule from event import declare_event, register_handler, emit_event -from api.models import (Message, MessageProperty, Project, - ProjectProperty, Result) +from api.models import (Message, Project, Result) from api.rest import PluginMethodField, reverse_detail from api.views import APILoginRequiredView, prepare_series from patchew.logviewer import LogView diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ import datetime import time import math from api.views import APILoginRequiredView -from api.models import (Message, MessageProperty, MessageResult, - Project, ProjectResult, Result) +from api.models import (Message, MessageResult, Project, ProjectResult, Result) from api.rest import PluginMethodField, reverse_detail from api.search import SearchEngine, FLAG_TESTED from event import emit_event, declare_event, register_handler -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Still RFCish, but this time a bit more tested. The JavaScript config editor part was fleshed out and split over 7 patches. I also added REST API support for reading and storing the config (for now as JSON only, not YAML). Paolo Paolo Bonzini (17): api, patchew-cli: remove commands to directly access properties git: do not return properties from applier-get api: do not blindly return all properties from get-projects mods: move epilog to a .js file mods: use classes instead of ids mods: rename project_property_schema to project_config_schema mods: refactor extraction of configuration into a dictionary mods: refactor construction of config editor HTML mods: place a single Save button at the end of the forms mods: do not allow creating duplicate map items mods: ask for confirmation after the user has added an item models: store plugin configuration in a single, separate JSONField rest: allow accessing the configuration via the REST API models: introduce flags into Messages models: add property fields to Message models: switch from property tables to field models: remove property tables api/admin.py | 14 +- api/migrations/0046_project_config.py | 22 ++ .../0047_populate_project_config.py | 56 ++++ api/migrations/0048_message_flags.py | 20 ++ api/migrations/0049_populate_message_flags.py | 42 +++ .../0050_message_flags_postgres_fts.py | 22 ++ api/migrations/0051_auto_20190418_1346.py | 33 +++ .../0052_populate_property_fields.py | 67 +++++ api/migrations/0053_auto_20190418_1357.py | 37 +++ api/models.py | 165 ++++++------ api/rest.py | 18 +- api/search.py | 15 +- api/views.py | 72 ++--- mod.py | 247 +++--------------- mods/email.py | 18 +- mods/git.py | 62 +++-- mods/tags.py | 5 +- mods/testing.py | 65 ++--- patchew-cli | 65 +---- static/js/config-editor.js | 131 ++++++++++ tests/patchewtest.py | 12 +- tests/test_git.py | 11 +- tests/test_rest.py | 30 +++ tests/test_testing.py | 41 ++- www/templates/project-detail.html | 8 + 25 files changed, 754 insertions(+), 524 deletions(-) create mode 100644 api/migrations/0046_project_config.py create mode 100644 api/migrations/0047_populate_project_config.py create mode 100644 api/migrations/0048_message_flags.py create mode 100644 api/migrations/0049_populate_message_flags.py create mode 100644 api/migrations/0050_message_flags_postgres_fts.py create mode 100644 api/migrations/0051_auto_20190418_1346.py create mode 100644 api/migrations/0052_populate_property_fields.py create mode 100644 api/migrations/0053_auto_20190418_1357.py create mode 100644 static/js/config-editor.js -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel set-properties was completely unused; get-project-properties was not needed for the Javascript interface, only for the "project property" command. The Django admin is enough if one needs that kind of fine-grained access. --- api/views.py | 23 -------------------- patchew-cli | 61 ---------------------------------------------------- 2 files changed, 84 deletions(-) diff --git a/api/views.py b/api/views.py index XXXXXXX..XXXXXXX 100644 --- a/api/views.py +++ b/api/views.py @@ -XXX,XX +XXX,XX @@ class AddProjectView(APILoginRequiredView): p.save() -class GetProjectPropertiesView(APILoginRequiredView): - name = "get-project-properties" - - def handle(self, request, project): - po = Project.objects.get(name=project) - if not po.maintained_by(request.user): - raise PermissionDenied("Access denied to this project") - return po.get_properties() - - class UpdateProjectHeadView(APILoginRequiredView): name = "update-project-head" allowed_groups = ["importers"] @@ -XXX,XX +XXX,XX @@ class UpdateProjectHeadView(APILoginRequiredView): return ret -class SetPropertyView(APILoginRequiredView): - name = "set-properties" - allowed_groups = ["importers"] - - def handle(self, request, project, message_id, properties): - mo = Message.objects.filter(project__name=project, - message_id=message_id).first() - if not mo: - raise Http404("Message not found") - for k, v in properties.items(): - mo.set_property(k, v) - - class SetProjectPropertiesView(APILoginRequiredView): name = "set-project-properties" allowed_groups = ["maintainers"] diff --git a/patchew-cli b/patchew-cli index XXXXXXX..XXXXXXX 100755 --- a/patchew-cli +++ b/patchew-cli @@ -XXX,XX +XXX,XX @@ class ProjectCommand(SubCommand): finally: shutil.rmtree(wd) - def project_property(self, argv): - parser = argparse.ArgumentParser() - parser.add_argument("name", help="Name of the project") - parser.add_argument("prop", nargs="?", help="Name of the property") - parser.add_argument("--delete", "-d", action="store_true", - help="""delete the property with the given name. - Must give a property name""") - parser.add_argument("value", nargs="?", help="Value of the property to set") - args = parser.parse_args(argv) - if not args.value and not args.delete: - # Get property and print them or the specified one - r = self.api_do("get-project-properties", - project=args.name) - if not args.prop: - for k, v in iter(r.items()) if r else []: - print(k, v) - else: - if r and args.prop in r: - print(args.prop, r[args.prop]) - else: - print("Property Not found:", args.prop) - if r and list(r.keys()): - print("There are:", ", ".join(list(r.keys()))) - else: - if args.delete: - args.value = None - # Set property - self.api_do("set-project-properties", project=args.name, - properties={args.prop: args.value}) - def do(self, args, argv): if argv: if argv[0] == "add": return self.add_project(argv[1:]) - elif argv[0] == "property": - return self.project_property(argv[1:]) elif argv[0] == "info": return self.show_project(argv[1:]) elif argv[0] == "update": @@ -XXX,XX +XXX,XX @@ class UntestCommand(SubCommand): self.api_do("untest", terms=args.term) return 0 -class SetPropertyCommand(SubCommand): - name = "set-property" - want_argv = True - - def arguments(self, parser): - parser.add_argument("--project", "-p", required=True, - help="Project name") - parser.add_argument("--message-id", "-m", required=True, - help="Project name") - parser.add_argument("--file", action="store_true", - help="Read property values from given file") - parser.add_argument("--json", action="store_true", - help="Read property values as json") - - def do(self, args, argv): - if not argv: - return 0 - if len(argv) % 2: - print("Name and value unpaired:", argv[-1]) - props = dict(list(zip(argv[::2], argv[1::2]))) - if args.file: - props = dict([(k, open(v, "r").read()) for k, v in props.items()]) - if args.json: - props = dict([(k, json.loads(v)) for k, v in props.items()]) - self.api_do("set-properties", project=args.project, - message_id=args.message_id, - properties=props) - return 0 - class TesterCommand(SubCommand): name = "tester" want_argv = True -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Properties that are of interest to the applier are already returned directly in the applier-get reply; the applier is only using properties to retrieve tags from old servers that do not have commit 32bddb4 ("models: convert tags from property to field", 2018-10-30). It is not needed anymore, remove it to hide properties from the API. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- api/views.py | 14 ++++---------- patchew-cli | 4 +--- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/api/views.py b/api/views.py index XXXXXXX..XXXXXXX 100644 --- a/api/views.py +++ b/api/views.py @@ -XXX,XX +XXX,XX @@ class SetProjectPropertiesView(APILoginRequiredView): po.set_property(k, v) -def get_properties(m): - r = m.get_properties() - # for compatibility with patchew-cli's applier mode - if m.tags: - r['tags'] = m.tags - return r - - class DeleteProjectPropertiesByPrefixView(APILoginRequiredView): name = "delete-project-properties-by-prefix" allowed_groups = ["maintainers"] @@ -XXX,XX +XXX,XX @@ def prepare_patch(p): r = {"subject": p.subject, "message-id": p.message_id, "mbox": p.get_mbox(), - "properties": get_properties(p) + # For backwards compatibility with old clients + "properties": {} } return r @@ -XXX,XX +XXX,XX @@ def prepare_series(request, s, fields=None): if want_field("patches"): r["patches"] = [prepare_patch(x) for x in s.get_patches()] if want_field("properties"): - r["properties"] = get_properties(s) + # For backwards compatibility with old clients + r["properties"] = {} if want_field("tags"): r["tags"] = s.tags if want_field("is_complete"): diff --git a/patchew-cli b/patchew-cli index XXXXXXX..XXXXXXX 100755 --- a/patchew-cli +++ b/patchew-cli @@ -XXX,XX +XXX,XX @@ class ApplyCommand(SubCommand): subprocess.check_output(["git", "log", "-n", "1", "--format=%b"], cwd=repo) \ .decode('utf-8').splitlines() - # old servers do not have "tags" directly in the response - for t in set(p["properties"].get("tags", []) + p.get("tags", []) + \ - s["properties"].get("tags", []) + s.get("tags", [])): + for t in set(p.get("tags", []) + s.get("tags", [])): if t in commit_message_lines: continue filter_cmd += "echo '%s';" % t -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Only return the two properties that patchew-cli actually needs, through a new hook. Do this before the configuration is moved out of the properties table, because these "properties" are actually configuration items. This will be cleaned up once patchew-cli switches to the REST API. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- api/views.py | 6 +++++- mods/git.py | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/api/views.py b/api/views.py index XXXXXXX..XXXXXXX 100644 --- a/api/views.py +++ b/api/views.py @@ -XXX,XX +XXX,XX @@ import json from .search import SearchEngine from django.views.decorators.csrf import csrf_exempt from django.utils.decorators import method_decorator +from mod import dispatch_module_hook class APIView(View): @@ -XXX,XX +XXX,XX @@ def prepare_project(p): "url": p.url, "git": p.git, "description": p.description, - "properties": p.get_properties(), + "properties": {}, } + dispatch_module_hook("get_projects_prepare_hook", project=p, + response=ret['properties']) + return ret diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): def rest_series_fields_hook(self, request, fields, detailed): fields['based_on'] = PluginMethodField(obj=self, required=False) + def get_projects_prepare_hook(self, project, response): + response["git.head"] = project.get_property("git.head") + response["git.push_to"] = project.get_property("git.push_to") + def prepare_message_hook(self, request, message, detailed): if not message.is_series_head: return -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Load the config editor Javascript via <script> from the template, instead of including it once per plugin. --- mod.py | 132 +----------------------------- static/js/config-editor.js | 127 ++++++++++++++++++++++++++++ www/templates/project-detail.html | 1 + 3 files changed, 129 insertions(+), 131 deletions(-) create mode 100644 static/js/config-editor.js diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): assert not isinstance(self.project_property_schema, StringSchema) assert not isinstance(self.project_property_schema, IntegerSchema) scm = self.project_property_schema - tmpl = self._build_one(request, project, scm.name + ".", scm) - tmpl += self._render_template(request, project, TMPL_END) - return tmpl + return self._build_one(request, project, scm.name + ".", scm) _loaded_modules = {} @@ -XXX,XX +XXX,XX @@ TMPL_MAP = """ </div> </div> """ - -TMPL_END = """ -<script type="text/javascript"> -function save_done(btn, succeeded, error) { - $(btn).text("Save"); - $(btn).removeClass("disabled"); - info = $("<div class=\\"alert save-message\\"></div>"); - if (succeeded) { - info.addClass("alert-success"); - info.html("Saved"); - } else { - info.addClass("alert-danger"); - info.html("Error: " + error); - } - info.insertBefore($(btn)); -} - -function collect_properties(btn, check_required) { - prefix = $(btn).parent().parent().find("#property-prefix").val(); - properties = {}; - $(btn).parent().parent().find(".project-property").each(function () { - if (check_required && this.required && !this.value) { - alert($(this).parent().find("label").html() + " is required!"); - $(this).focus(); - properties = false; - return false; - } - if (this.type == "number") { - val = parseInt(this.value); - if (isNaN(val)) { - alert("Invalid number for " + this.name); - $(this).focus(); - properties = false; - return false; - } - } else if (this.type == "checkbox") { - if (this.checked) { - val = true; - } else { - val = false; - } - } else { - val = this.value; - } - properties[prefix + this.name] = val; - }); - return properties; -} - -function properties_save(btn) { - if ($(btn).hasClass("disabled")) { - return; - } - props = collect_properties(btn, true); - if (!props) { - return; - } - $(btn).addClass("disabled"); - $(btn).text("Saving..."); - $(btn).parent().find(".save-message").remove(); - patchew_api_do("set-project-properties", - { project: "{{ project.name }}", - properties: props }) - .done(function (data) { - save_done(btn, true); - }) - .fail(function (data, text, error) { - save_done(btn, false, error); - }); -} - -function collect_items(btn) { - $(btn).parent().parent().find(".map-item"); - return {}; -} - -function map_add_item(btn) { - name = window.prompt("Please input a name"); - if (!name || name == 'null') { - return; - } - if (name in collect_items(btn)) { - alert(test_name + " already exists."); - return; - } - if (name.indexOf(".") >= 0) { - alert("Invalid name, no dot is allowed."); - return; - } - container = $(btn).parent().parent(); - tmpl = container.find("#item-template").html(); - nt = $(tmpl) - nt.find("#item-name").html(name); - old = nt.find("#property-prefix").val(); - nt.find("#property-prefix").val(old + name + "."); - container.find(".items").append(nt); -} -function map_delete_item(btn) { - name = $(btn).parent().parent().parent().find("#item-name").html(); - prefix = $(btn).parent().parent().parent().find("#prefix").val(); - if (!window.confirm("Really delete '" + name +"'?")) { - return; - } - $(btn).addClass("disabled"); - $(btn).text("Deleting..."); - $(btn).parent().find(".delete-message").remove(); - patchew_api_do("delete-project-properties-by-prefix", - { project: "{{ project.name }}", - prefix: prefix }) - .done(function (data) { - container = $(btn).parent().parent().parent(); - container.remove(); - }) - .fail(function (data, text, error) { - $(btn).removeClass("disabled"); - $(btn).text("Delete"); - info = $("<div class=\\"alert alert-danger delete-message\\"></div>"); - info.html("Error: " + error); - info.insertBefore($(btn)); - }); -} -function enum_change(which) { - val = $(which).val(); - desc = $(which).parent().find("#enum-desc-" + val).html(); - $(which).parent().find("#enum-desc").html(desc); -} -</script> -""" diff --git a/static/js/config-editor.js b/static/js/config-editor.js new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/static/js/config-editor.js @@ -XXX,XX +XXX,XX @@ +function current_project() { + return $('h2').text(); +} + +function save_done(btn, succeeded, error) { + $(btn).text("Save"); + $(btn).removeClass("disabled"); + info = $("<div class=\"alert save-message\"></div>"); + if (succeeded) { + info.addClass("alert-success"); + info.html("Saved"); + } else { + info.addClass("alert-danger"); + info.html("Error: " + error); + } + info.insertBefore($(btn)); +} + +function collect_properties(btn, check_required) { + prefix = $(btn).parent().parent().find("#property-prefix").val(); + properties = {}; + $(btn).parent().parent().find(".project-property").each(function () { + if (check_required && this.required && !this.value) { + alert($(this).parent().find("label").html() + " is required!"); + $(this).focus(); + properties = false; + return false; + } + if (this.type == "number") { + val = parseInt(this.value); + if (isNaN(val)) { + alert("Invalid number for " + this.name); + $(this).focus(); + properties = false; + return false; + } + } else if (this.type == "checkbox") { + if (this.checked) { + val = true; + } else { + val = false; + } + } else { + val = this.value; + } + properties[prefix + this.name] = val; + }); + return properties; +} + +function properties_save(btn) { + if ($(btn).hasClass("disabled")) { + return; + } + props = collect_properties(btn, true); + if (!props) { + return; + } + $(btn).addClass("disabled"); + $(btn).text("Saving..."); + $(btn).parent().find(".save-message").remove(); + patchew_api_do("set-project-properties", + { project: current_project(), + properties: props }) + .done(function (data) { + save_done(btn, true); + }) + .fail(function (data, text, error) { + save_done(btn, false, error); + }); +} + +function collect_items(btn) { + $(btn).parent().parent().find(".map-item"); + return {}; +} + +function map_add_item(btn) { + name = window.prompt("Please input a name"); + if (!name || name == 'null') { + return; + } + if (name in collect_items(btn)) { + alert(test_name + " already exists."); + return; + } + if (name.indexOf(".") >= 0) { + alert("Invalid name, no dot is allowed."); + return; + } + container = $(btn).parent().parent(); + tmpl = container.find("#item-template").html(); + nt = $(tmpl) + nt.find("#item-name").html(name); + old = nt.find("#property-prefix").val(); + nt.find("#property-prefix").val(old + name + "."); + container.find(".items").append(nt); +} +function map_delete_item(btn) { + name = $(btn).parent().parent().parent().find("#item-name").html(); + prefix = $(btn).parent().parent().parent().find("#prefix").val(); + if (!window.confirm("Really delete '" + name +"'?")) { + return; + } + $(btn).addClass("disabled"); + $(btn).text("Deleting..."); + $(btn).parent().find(".delete-message").remove(); + patchew_api_do("delete-project-properties-by-prefix", + { project: current_project(), + prefix: prefix }) + .done(function (data) { + container = $(btn).parent().parent().parent(); + container.remove(); + }) + .fail(function (data, text, error) { + $(btn).removeClass("disabled"); + $(btn).text("Delete"); + info = $("<div class=\"alert alert-danger delete-message\"></div>"); + info.html("Error: " + error); + info.insertBefore($(btn)); + }); +} +function enum_change(which) { + val = $(which).val(); + desc = $(which).parent().find("#enum-desc-" + val).html(); + $(which).parent().find("#enum-desc").html(desc); +} diff --git a/www/templates/project-detail.html b/www/templates/project-detail.html index XXXXXXX..XXXXXXX 100644 --- a/www/templates/project-detail.html +++ b/www/templates/project-detail.html @@ -XXX,XX +XXX,XX @@ <link rel="stylesheet" href="/static/css/project-detail.css"> <link rel="stylesheet" href="/static/css/colorbox.css"> <script src="/static/js/jquery.colorbox-min.js"></script> +<script src="/static/js/config-editor.js"></script> {% endblock %} {% block content %} -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel ids should be unique, avoid duplicating them. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- mod.py | 12 ++++++------ static/js/config-editor.js | 14 +++++++------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ TMPL_ENUM = """ {% endif %} {% endfor %} </select> - <div class="form-group" id="enum-desc"> + <div class="form-group enum-desc"> {% for opt, desc in schema.enums.items %} {% if opt == value %} """ + TMPL_ENUM_DESC + """ @@ -XXX,XX +XXX,XX @@ TMPL_ENUM = """ {% endfor %} </div> {% for opt, desc in schema.enums.items %} - <div class="hidden" id="enum-desc-{{ opt }}"> + <div class="hidden enum-desc-{{ opt }}"> """ + TMPL_ENUM_DESC + """ </div> {% endfor %} @@ -XXX,XX +XXX,XX @@ TMPL_ENUM = """ """ TMPL_ARRAY = """ -<input type="hidden" name="property-prefix" id="property-prefix" value="{{ prefix }}"> +<input type="hidden" name="property-prefix" class="property-prefix" value="{{ prefix }}"> {% for schema in members %} {{ schema }} {% endfor %} @@ -XXX,XX +XXX,XX @@ TMPL_MAP_ITEM = """ <div class="item panel panel-default"> <div class="panel-heading panel-toggler" onclick="patchew_toggler_onclick(this)"> {{ item_schema.title }} - <strong id="item-name">{{ item.name }}</strong> - <input type="hidden" value="{{ prefix }}{{ item.name }}." id="prefix" /> + <strong class="item-name">{{ item.name }}</strong> + <input type="hidden" value="{{ prefix }}{{ item.name }}." class="prefix" /> </div> <div class="panel-body panel-collapse collapse"> {{ item.html }} @@ -XXX,XX +XXX,XX @@ TMPL_MAP_ITEM = """ TMPL_MAP = """ <div id="{{ schema.name }}-container"> - <script id="item-template" type="text/x-custom-template"> + <script class="item-template" type="text/x-custom-template"> """ + TMPL_MAP_ITEM + """ </script> <div class="items"> diff --git a/static/js/config-editor.js b/static/js/config-editor.js index XXXXXXX..XXXXXXX 100644 --- a/static/js/config-editor.js +++ b/static/js/config-editor.js @@ -XXX,XX +XXX,XX @@ function save_done(btn, succeeded, error) { } function collect_properties(btn, check_required) { - prefix = $(btn).parent().parent().find("#property-prefix").val(); + prefix = $(btn).parent().parent().find(".property-prefix").val(); properties = {}; $(btn).parent().parent().find(".project-property").each(function () { if (check_required && this.required && !this.value) { @@ -XXX,XX +XXX,XX @@ function map_add_item(btn) { return; } container = $(btn).parent().parent(); - tmpl = container.find("#item-template").html(); + tmpl = container.find(".item-template").html(); nt = $(tmpl) - nt.find("#item-name").html(name); - old = nt.find("#property-prefix").val(); - nt.find("#property-prefix").val(old + name + "."); + nt.find(".item-name").html(name); + old = nt.find(".property-prefix").val(); + nt.find(".property-prefix").val(old + name + "."); container.find(".items").append(nt); } function map_delete_item(btn) { - name = $(btn).parent().parent().parent().find("#item-name").html(); - prefix = $(btn).parent().parent().parent().find("#prefix").val(); + name = $(btn).parent().parent().parent().find(".item-name").html(); + prefix = $(btn).parent().parent().parent().find(".prefix").val(); if (!window.confirm("Really delete '" + name +"'?")) { return; } -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel We would like to split properties from configurations, and place them in two separate fields of the project. As a start, do not use "property" when referring to the module schemas. The fact that configuration is stored as a property is (more or less) an implementation detail. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- mod.py | 8 ++++---- mods/email.py | 2 +- mods/git.py | 2 +- mods/testing.py | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): """ Module base class """ name = None # The name of the module, must be unique default_config = "" # The default config string - project_property_schema = None + project_config_schema = None def get_model(self): # ALways read from DB to accept configuration update in-flight @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): assert False def build_config_html(self, request, project): - assert not isinstance(self.project_property_schema, StringSchema) - assert not isinstance(self.project_property_schema, IntegerSchema) - scm = self.project_property_schema + assert not isinstance(self.project_config_schema, StringSchema) + assert not isinstance(self.project_config_schema, IntegerSchema) + scm = self.project_config_schema return self._build_one(request, project, scm.name + ".", scm) _loaded_modules = {} diff --git a/mods/email.py b/mods/email.py index XXXXXXX..XXXXXXX 100644 --- a/mods/email.py +++ b/mods/email.py @@ -XXX,XX +XXX,XX @@ Email information is configured in "INI" style: required=True), ]) - project_property_schema = \ + project_config_schema = \ ArraySchema("email", desc="Configuration for email module", members=[ MapSchema("notifications", "Email notifications", diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): allowed_groups = ('importers', ) result_data_serializer_class = ResultDataSerializer - project_property_schema = \ + project_config_schema = \ ArraySchema("git", desc="Configuration for git module", members=[ StringSchema("push_to", "Push remote", diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): required=True), ]) - project_property_schema = \ + project_config_schema = \ ArraySchema("testing", desc="Configuration for testing module", members=[ MapSchema("tests", "Tests", -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Both the email and testing modules have code to extract the configuration from project properties into a dictionary. Generalize that code, using the project_config_schema to drive the conversion, and use it in the git plugin as well This matches the way configuration will be stored in the database when we move away from project properties. In fact, all this nice visitor code will disappear very soon... Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- mod.py | 37 ++++++++++++++++++++++++++++++++++++- mods/email.py | 16 ++-------------- mods/git.py | 28 ++++++++++++++++------------ mods/testing.py | 25 ++++--------------------- 4 files changed, 58 insertions(+), 48 deletions(-) diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): prefix = prefix + scm.name + "." def _build_map_items(): r = {} - for p, v in project.get_properties().items(): + for p in project.get_properties().keys(): if not p.startswith(prefix): continue name = p[len(prefix):] @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): scm = self.project_config_schema return self._build_one(request, project, scm.name + ".", scm) + def _get_map_scm(self, project, prop_name, scm): + prefix = prop_name + "." + result = {} + for p in project.get_properties().keys(): + if not p.startswith(prefix): + continue + name = p[len(prefix):] + name = name[:name.rfind(".")] + if name in result: + continue + assert scm.item.name == '{name}' + value = self._get_one(project, prefix + name, scm.item) + result[name] = value + return result + + def _get_array_scm(self, project, prop_name, scm): + prefix = prop_name + "." + result = {} + for i in scm.members: + assert i.name != '{name}' + result[i.name] = self._get_one(project, prefix + i.name, i) + return result + + def _get_one(self, project, prop_name, scm): + if type(scm) == MapSchema: + return self._get_map_scm(project, prop_name, scm) + elif type(scm) == ArraySchema: + return self._get_array_scm(project, prop_name, scm) + else: + return project.get_property(prop_name) + + def get_project_config(self, project): + scm = self.project_config_schema + return self._get_one(project, scm.name, scm) + _loaded_modules = {} def _module_init_config(cls): diff --git a/mods/email.py b/mods/email.py index XXXXXXX..XXXXXXX 100644 --- a/mods/email.py +++ b/mods/email.py @@ -XXX,XX +XXX,XX @@ Email information is configured in "INI" style: default_config = _default_config email_schema = \ - ArraySchema("email_notification", "Email Notification", + ArraySchema("{name}", "Email Notification", desc="Email notification", members=[ EnumSchema("event", "Event", @@ -XXX,XX +XXX,XX @@ Email information is configured in "INI" style: return "<%s@patchew.org>" % uuid.uuid1() def get_notifications(self, project): - ret = {} - for k, v in project.get_properties().items(): - if not k.startswith("email.notifications."): - continue - tn = k[len("email.notifications."):] - if "." not in tn: - continue - an = tn[tn.find(".") + 1:] - tn = tn[:tn.find(".")] - ret.setdefault(tn, {}) - ret[tn][an] = v - ret[tn]["name"] = tn - return ret + return self.get_project_config(project).get("notifications", {}) def on_event(self, event, **params): class EmailCancelled(Exception): diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): if series.is_complete: self.mark_as_pending_apply(series) - def get_project_config(self, project, what): - return project.get_property("git." + what) - def _is_repo(self, path): if not os.path.isdir(path): return False @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): def get_mirror(self, po, request, format): response = {} - for key, prop in (("head", "git.head"), - ("pushurl", "git.push_to"), - ("url", "git.public_repo")): - response[key] = po.get_property(prop) or None + config = self.get_project_config(po) + if "push_to" in config: + response["pushurl"] = config["push_to"] + if "public_repo" in config: + response["url"] = config["public_repo"] + head = po.get_property("git.head") + if head: + response["head"] = head return response def rest_project_fields_hook(self, request, fields): @@ -XXX,XX +XXX,XX @@ class GitModule(PatchewModule): def get_projects_prepare_hook(self, project, response): response["git.head"] = project.get_property("git.head") - response["git.push_to"] = project.get_property("git.push_to") + config = self.get_project_config(project) + if "push_to" in config: + response["git.push_to"] = config["push_to"] def prepare_message_hook(self, request, message, detailed): if not message.is_series_head: @@ -XXX,XX +XXX,XX @@ class ApplierGetView(APILoginRequiredView): "properties", "tags"]) po = m.project - for prop in ["git.push_to", "git.public_repo", "git.url_template"]: - if po.get_property(prop): - response[prop] = po.get_property(prop) + config = _instance.get_project_config(po) + for k, v in config.items(): + response["git." + k] = v base = _instance.get_base(m) if base: response["git.repo"] = base.data["repo"] @@ -XXX,XX +XXX,XX @@ class ApplierReportView(APILoginRequiredView): if url: data['url'] = url elif tag: - url_template = p.get_property("git.url_template") + config = _instance.get_project_config(po) + url_template = config.get("url_template") if url_template: data['url'] = url_template.replace("%t", tag) if base: diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): ret = {} if isinstance(obj, Message): obj = obj.project - for k, v in obj.get_properties().items(): - if not k.startswith("testing.tests."): - continue - tn = k[len("testing.tests."):] - if "." not in tn: - continue - an = tn[tn.find(".") + 1:] - tn = tn[:tn.find(".")] - ret.setdefault(tn, {}) - ret[tn][an] = v - ret[tn]["name"] = tn - return ret + return self.get_project_config(obj).get("tests", {}) def _build_reset_ops(self, obj): if isinstance(obj, Message): @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): project.extra_ops += self._build_reset_ops(project) def get_capability_probes(self, project): - ret = {} - for k, v in project.get_properties().items(): - prefix = "testing.requirements." - if not k.startswith(prefix): - continue - name = k[len(prefix):] - name = name[:name.find(".")] - ret[name] = v - return ret + props = self.get_project_config(project).get('requirements', {}) + return {k: v['script'] for k, v in props.items()} def get_testing_probes(self, project, request, format): return self.get_capability_probes(project) @@ -XXX,XX +XXX,XX @@ class TestingGetView(APILoginRequiredView): if req not in capabilities: break else: + t["name"] = tn yield r, t def _find_project_test(self, request, po, tester, capabilities): -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Refactor the Javascript config editor to work on the properties dictionary that is retrieved by get_project_config. The _build_*_scm functions therefore no longer need to understand how properties are stored in the database. --- mod.py | 77 ++++++++++++++++++++-------------------------------------- 1 file changed, 27 insertions(+), 50 deletions(-) diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): data["module"] = self return Template(tmpl).render(Context(data)) - def _build_map_scm(self, request, project, prefix, scm): - prefix = prefix + scm.name + "." - def _build_map_items(): - r = {} - for p in project.get_properties().keys(): - if not p.startswith(prefix): - continue - name = p[len(prefix):] - name = name[:name.rfind(".")] - if name in r: - continue - pref = prefix + name + "." - r[name] = { - "name": name, - "html": self._build_one(request, project, - pref, scm.item) - } - return list(r.values()) - - schema_html = self._build_one(request, project, prefix, - scm.item) + def _build_map_scm(self, request, project, prefix, config, scm): + schema_html = self._build_one(request, project, "", {}, scm.item) item = {"html": schema_html} - items = _build_map_items() + items = [{ + "name": name, + "html": self._build_one(request, project, prefix + "." + name, + value, scm.item)} for name, value in config.items()] return self._render_template(request, project, TMPL_MAP, schema=scm, item_schema=scm.item, @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): items=items, item=item) - def _build_array_scm(self, request, project, prefix, scm): + def _build_array_scm(self, request, project, prefix, config, scm): members = [self._build_one(request, project, - prefix, x) for x in scm.members] + prefix + "." + x.name, + config.get(x.name), x) for x in scm.members] show_save_button = False for m in scm.members: if type(m) == StringSchema: @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): show_save_button=show_save_button, prefix=prefix) - def _build_string_scm(self, request, project, prefix, scm): - prop_name = prefix + scm.name - prop_value = project.get_property(prop_name) + def _build_string_scm(self, request, project, prefix, config, scm): return self._render_template(request, project, TMPL_STRING, schema=scm, name=scm.name, prefix=prefix, - value=prop_value) + value=config or '') - def _build_integer_scm(self, request, project, prefix, scm): - prop_name = prefix + scm.name - prop_value = project.get_property(prop_name) + def _build_integer_scm(self, request, project, prefix, config, scm): return self._render_template(request, project, TMPL_INTEGER, schema=scm, name=scm.name, prefix=prefix, - value=prop_value) + value=config or 0) - def _build_boolean_scm(self, request, project, prefix, scm): - prop_name = prefix + scm.name - prop_value = project.get_property(prop_name) + def _build_boolean_scm(self, request, project, prefix, config, scm): return self._render_template(request, project, TMPL_BOOLEAN, schema=scm, name=scm.name, prefix=prefix, - value=prop_value) + value=config or False) - def _build_enum_scm(self, request, project, prefix, scm): - prop_name = prefix + scm.name - prop_value = project.get_property(prop_name) + def _build_enum_scm(self, request, project, prefix, config, scm): return self._render_template(request, project, TMPL_ENUM, schema=scm, name=scm.name, prefix=prefix, - value=prop_value) + value=config) - def _build_one(self, request, project, prefix, scm): + def _build_one(self, request, project, prefix, config, scm): if type(scm) == MapSchema: - return self._build_map_scm(request, project, prefix, scm) + return self._build_map_scm(request, project, prefix, config, scm) elif type(scm) == StringSchema: - return self._build_string_scm(request, project, prefix, scm) + return self._build_string_scm(request, project, prefix, config, scm) elif type(scm) == IntegerSchema: - return self._build_integer_scm(request, project, prefix, scm) + return self._build_integer_scm(request, project, prefix, config, scm) elif type(scm) == BooleanSchema: - return self._build_boolean_scm(request, project, prefix, scm) + return self._build_boolean_scm(request, project, prefix, config, scm) elif type(scm) == EnumSchema: - return self._build_enum_scm(request, project, prefix, scm) + return self._build_enum_scm(request, project, prefix, config, scm) elif type(scm) == ArraySchema: - return self._build_array_scm(request, project, prefix, scm) + return self._build_array_scm(request, project, prefix, config, scm) assert False def build_config_html(self, request, project): - assert not isinstance(self.project_config_schema, StringSchema) - assert not isinstance(self.project_config_schema, IntegerSchema) + assert isinstance(self.project_config_schema, ArraySchema) scm = self.project_config_schema - return self._build_one(request, project, scm.name + ".", scm) + config = self.get_project_config(project) + return self._build_one(request, project, scm.name, config, scm) def _get_map_scm(self, project, prop_name, scm): prefix = prop_name + "." -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Because the save button now acts on the whole config, we cannot anymore rely on finding the property prefix from the save button's parents. Instead, each property control has a data-property-path attribute and building the configuration dictionary is as simple as looking all of them up. Likewise, each map item has a data-property-prefix attribute which simplifies deletion and is also used to build the data-property-path when a new map item is added. --- mod.py | 25 +++++++------------------ static/js/config-editor.js | 30 ++++++++++++++++++------------ www/templates/project-detail.html | 7 +++++++ 3 files changed, 32 insertions(+), 30 deletions(-) diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): members = [self._build_one(request, project, prefix + "." + x.name, config.get(x.name), x) for x in scm.members] - show_save_button = False - for m in scm.members: - if type(m) == StringSchema: - show_save_button = True - break return self._render_template(request, project, TMPL_ARRAY, schema=scm, members=members, - show_save_button=show_save_button, prefix=prefix) def _build_string_scm(self, request, project, prefix, config, scm): @@ -XXX,XX +XXX,XX @@ TMPL_STRING = """ <input {% endif %} type="text" class="form-control project-property" + data-property-path="{{ prefix }}" id="{{ module.name }}-input-{{ schema.name }}" {% if schema.required %}required{%endif%} name="{{ name }}" placeholder="{{ schema.desc }}" {% if schema.multiline %} @@ -XXX,XX +XXX,XX @@ TMPL_INTEGER = """ <div class="form-group"> <label for="{{ module.name }}-input-{{ schema.name }}">{{ schema.title }}</label> <input type="number" class="form-control project-property" + data-property-path="{{ prefix }}" id="{{ module.name }}-input-{{ schema.name }}" {% if schema.required %}required{%endif%} name="{{ name }}" placeholder="{{ schema.desc }}" {% if schema.multiline %} @@ -XXX,XX +XXX,XX @@ TMPL_BOOLEAN = """ <div class="checkbox"> <label> <input class="project-property" type="checkbox" name="{{ name }}" + data-property-path="{{ prefix }}" {% if value == None %} {% if schema.default %} checked @@ -XXX,XX +XXX,XX @@ TMPL_ENUM = """ <label for="{{ module.name }}-input-{{ schema.name }}">{{ schema.title }}</label> <select class="form-control project-property" id="{{ module.name }}-input-{{ schema.name }}" + data-property-path="{{ prefix }}" onchange="enum_change(this)" {% if schema.required %}required{%endif%} name="{{ name }}"> @@ -XXX,XX +XXX,XX @@ TMPL_ENUM = """ """ TMPL_ARRAY = """ -<input type="hidden" name="property-prefix" class="property-prefix" value="{{ prefix }}"> {% for schema in members %} {{ schema }} {% endfor %} -{% if show_save_button %} - <div class="form-group"> - <button type="button" class="btn btn-info" onclick="properties_save(this)"> - Save - </button> - </div> -{% endif %} """ TMPL_MAP_ITEM = """ -<div class="item panel panel-default"> - <div class="panel-heading panel-toggler" onclick="patchew_toggler_onclick(this)"> +<div class="item panel panel-default" data-property-prefix="{{ prefix }}{% if item.name %}.{{ item.name }}{% endif %}"> + <div class="item-heading panel-heading panel-toggler" onclick="patchew_toggler_onclick(this)"> {{ item_schema.title }} <strong class="item-name">{{ item.name }}</strong> - <input type="hidden" value="{{ prefix }}{{ item.name }}." class="prefix" /> </div> <div class="panel-body panel-collapse collapse"> {{ item.html }} @@ -XXX,XX +XXX,XX @@ TMPL_MAP_ITEM = """ """ TMPL_MAP = """ -<div id="{{ schema.name }}-container"> +<div> <script class="item-template" type="text/x-custom-template"> """ + TMPL_MAP_ITEM + """ </script> diff --git a/static/js/config-editor.js b/static/js/config-editor.js index XXXXXXX..XXXXXXX 100644 --- a/static/js/config-editor.js +++ b/static/js/config-editor.js @@ -XXX,XX +XXX,XX @@ function save_done(btn, succeeded, error) { } function collect_properties(btn, check_required) { - prefix = $(btn).parent().parent().find(".property-prefix").val(); properties = {}; $(btn).parent().parent().find(".project-property").each(function () { + path = $(this).data('property-path'); if (check_required && this.required && !this.value) { alert($(this).parent().find("label").html() + " is required!"); $(this).focus(); @@ -XXX,XX +XXX,XX @@ function collect_properties(btn, check_required) { } else { val = this.value; } - properties[prefix + this.name] = val; + properties[path] = val; }); return properties; } @@ -XXX,XX +XXX,XX @@ function map_add_item(btn) { if (!name || name == 'null') { return; } + container = $(btn).parent().parent(); if (name in collect_items(btn)) { alert(test_name + " already exists."); return; @@ -XXX,XX +XXX,XX @@ function map_add_item(btn) { alert("Invalid name, no dot is allowed."); return; } - container = $(btn).parent().parent(); tmpl = container.find(".item-template").html(); - nt = $(tmpl) - nt.find(".item-name").html(name); - old = nt.find(".property-prefix").val(); - nt.find(".property-prefix").val(old + name + "."); - container.find(".items").append(nt); + nt = $(tmpl); + nt.find(".item-name").text(name); + prefix = nt.data('property-prefix') + '.' + name; + nt.data('property-prefix', prefix); + nt.find(".project-property").each(function() { + old = $(this).data('property-path'); + $(this).data('property-path', prefix + old); + }); + nt.find(".panel-collapse").collapse("show"); + container.find("> .items").append(nt); } + function map_delete_item(btn) { - name = $(btn).parent().parent().parent().find(".item-name").html(); - prefix = $(btn).parent().parent().parent().find(".prefix").val(); + item = $(btn).parent().parent().parent(); + name = item.find(".item-name").text(); + prefix = item.data('property-prefix') + "."; if (!window.confirm("Really delete '" + name +"'?")) { return; } @@ -XXX,XX +XXX,XX @@ function map_delete_item(btn) { { project: current_project(), prefix: prefix }) .done(function (data) { - container = $(btn).parent().parent().parent(); - container.remove(); + item.remove(); }) .fail(function (data, text, error) { $(btn).removeClass("disabled"); diff --git a/www/templates/project-detail.html b/www/templates/project-detail.html index XXXXXXX..XXXXXXX 100644 --- a/www/templates/project-detail.html +++ b/www/templates/project-detail.html @@ -XXX,XX +XXX,XX @@ </div> </div> {% endfor %} + {% if request.user.is_authenticated %} + <div class="form-group"> + <button type="button" class="btn btn-info" onclick="properties_save(this)"> + Save + </button> + </div> + {% endif %} </div> <script type="text/javascript"> -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel The code for that was there but more or less stubbed out, fix it. --- static/js/config-editor.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/static/js/config-editor.js b/static/js/config-editor.js index XXXXXXX..XXXXXXX 100644 --- a/static/js/config-editor.js +++ b/static/js/config-editor.js @@ -XXX,XX +XXX,XX @@ function properties_save(btn) { }); } -function collect_items(btn) { - $(btn).parent().parent().find(".map-item"); - return {}; +function collect_items(container) { + return container.find("> .items > .item > .item-heading > .item-name").map(function() { + return $(this).text(); + }).get(); } function map_add_item(btn) { @@ -XXX,XX +XXX,XX @@ function map_add_item(btn) { return; } container = $(btn).parent().parent(); - if (name in collect_items(btn)) { - alert(test_name + " already exists."); + if (collect_items(container).includes(name)) { + alert(name + " already exists."); return; } if (name.indexOf(".") >= 0) { -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel --- static/js/config-editor.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/static/js/config-editor.js b/static/js/config-editor.js index XXXXXXX..XXXXXXX 100644 --- a/static/js/config-editor.js +++ b/static/js/config-editor.js @@ -XXX,XX +XXX,XX @@ function current_project() { return $('h2').text(); } +function confirm_leaving_page(enable) { + if (enable) { + window.onbeforeunload = function() { + return "You have modified the configuration but have not saved it yet."; + }; + } else { + window.onbeforeunload = null; + } +} + function save_done(btn, succeeded, error) { $(btn).text("Save"); $(btn).removeClass("disabled"); @@ -XXX,XX +XXX,XX @@ function save_done(btn, succeeded, error) { if (succeeded) { info.addClass("alert-success"); info.html("Saved"); + confirm_leaving_page(false); } else { info.addClass("alert-danger"); info.html("Error: " + error); @@ -XXX,XX +XXX,XX @@ function map_add_item(btn) { }); nt.find(".panel-collapse").collapse("show"); container.find("> .items").append(nt); + confirm_leaving_page(true); } function map_delete_item(btn) { -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel This is the next step in the splitting the properties table into logically separate components. This time it's the turn of configuration data coming from the plugin schemas, which is extracted into a new JSON field. The configuration editor is simplified a bit because all the properties are sent to the server in a single shot and all configuration is overwritten. As a result, we no longer need the deletion API delete-project-properties-by-prefix. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- api/migrations/0046_project_config.py | 22 ++++++++ .../0047_populate_project_config.py | 56 +++++++++++++++++++ api/models.py | 11 ++++ api/views.py | 29 ++++------ mod.py | 34 +---------- mods/git.py | 27 ++++----- mods/testing.py | 7 ++- static/js/config-editor.js | 23 ++------ tests/patchewtest.py | 12 ++-- tests/test_git.py | 11 +++- tests/test_testing.py | 32 +++++++---- 11 files changed, 162 insertions(+), 102 deletions(-) create mode 100644 api/migrations/0046_project_config.py create mode 100644 api/migrations/0047_populate_project_config.py diff --git a/api/migrations/0046_project_config.py b/api/migrations/0046_project_config.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0046_project_config.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-04-18 12:56 +from __future__ import unicode_literals + +from django.db import migrations +import jsonfield.encoder +import jsonfield.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0045_message_maintainers'), + ] + + operations = [ + migrations.AddField( + model_name='project', + name='config', + field=jsonfield.fields.JSONField(default={}, dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, load_kwargs={}), + ), + ] diff --git a/api/migrations/0047_populate_project_config.py b/api/migrations/0047_populate_project_config.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0047_populate_project_config.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations +from django.db.models import Q + +def properties_to_config(apps, schema_editor): + Project = apps.get_model('api', 'Project') + for po in Project.objects.all(): + q = Q(name__startswith='testing.tests.') | \ + Q(name__startswith='testing.requirements.') | \ + Q(name__startswith='email.notifications.') | \ + Q(name__in=('git.push_to', 'git.url_template', 'git.public_repo')) + props = po.projectproperty_set.filter(q) + config = {} + for p in props: + *path, last = p.name.split('.') + parent = config + for item in path: + parent = parent.setdefault(item, {}) + parent[last] = p.value + #print(po, config) + po.config = config + po.save() + props.delete() + +def flatten_properties(source, prefix, result=None): + if result is None: + result = {} + for k, v in source.items(): + if isinstance(v, dict): + flatten_properties(v, prefix + k + '.', result) + else: + result[prefix + k] = v + return result + +def config_to_properties(apps, schema_editor): + Project = apps.get_model('api', 'Project') + ProjectProperty = apps.get_model('api', 'ProjectProperty') + for po in Project.objects.all(): + props = flatten_properties(po.config, '') + for k, v in props.items(): + new_prop = ProjectProperty(project=po, name=k, value=v) + new_prop.save() + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0046_project_config'), + ] + + operations = [ + migrations.RunPython(properties_to_config, + reverse_code=config_to_properties), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class Project(models.Model): "top project which has " "parent_project=NULL")) maintainers = models.ManyToManyField(User, blank=True) + config = jsonfield.JSONField(default={}) def __str__(self): return self.name @@ -XXX,XX +XXX,XX @@ class Project(models.Model): def has_project(self, project): return self.objects.filter(name=project).exists() + def save(self, *args, **kwargs): + old_project = Project.objects.filter(pk=self.pk).first() + old_config = old_project.config if old_project else None + super(Project, self).save(*args, **kwargs) + if old_config != self.config: + emit_event("SetProjectConfig", obj=self) + def get_property(self, prop, default=None): a = ProjectProperty.objects.filter(project=self, name=prop).first() if a: @@ -XXX,XX +XXX,XX @@ declare_event("SeriesMerged", project="project object", declare_event("MessageAdded", message="message object that is added") +declare_event("SetProjectConfig", obj="project whose configuration was updated") + + declare_event("SetProperty", obj="object to set the property", name="name of the property", value="value of the property", diff --git a/api/views.py b/api/views.py index XXXXXXX..XXXXXXX 100644 --- a/api/views.py +++ b/api/views.py @@ -XXX,XX +XXX,XX @@ class UpdateProjectHeadView(APILoginRequiredView): return ret -class SetProjectPropertiesView(APILoginRequiredView): - name = "set-project-properties" +class SetProjectConfigView(APILoginRequiredView): + name = "set-project-config" allowed_groups = ["maintainers"] - def handle(self, request, project, properties): + def handle(self, request, project, config): po = Project.objects.get(name=project) if not po.maintained_by(request.user): raise PermissionDenied("Access denied to this project") - for k, v in properties.items(): - po.set_property(k, v) - - -class DeleteProjectPropertiesByPrefixView(APILoginRequiredView): - name = "delete-project-properties-by-prefix" - allowed_groups = ["maintainers"] - - def handle(self, request, project, prefix): - po = Project.objects.get(name=project) - if not po.maintained_by(request.user): - raise PermissionDenied("Access denied to this project") - for k in [x for x in po.get_properties().keys() if x.startswith(prefix)]: - po.set_property(k, None) + new_config = {} + for k, v in config.items(): + *path, last = k.split('.') + parent = new_config + for item in path: + parent = parent.setdefault(item, {}) + parent[last] = v + po.config = new_config + po.save() def prepare_patch(p): diff --git a/mod.py b/mod.py index XXXXXXX..XXXXXXX 100644 --- a/mod.py +++ b/mod.py @@ -XXX,XX +XXX,XX @@ class PatchewModule(object): config = self.get_project_config(project) return self._build_one(request, project, scm.name, config, scm) - def _get_map_scm(self, project, prop_name, scm): - prefix = prop_name + "." - result = {} - for p in project.get_properties().keys(): - if not p.startswith(prefix): - continue - name = p[len(prefix):] - name = name[:name.rfind(".")] - if name in result: - continue - assert scm.item.name == '{name}' - value = self._get_one(project, prefix + name, scm.item) - result[name] = value - return result - - def _get_array_scm(self, project, prop_name, scm): - prefix = prop_name + "." - result = {} - for i in scm.members: - assert i.name != '{name}' - result[i.name] = self._get_one(project, prefix + i.name, i) - return result - - def _get_one(self, project, prop_name, scm): - if type(scm) == MapSchema: - return self._get_map_scm(project, prop_name, scm) - elif type(scm) == ArraySchema: - return self._get_array_scm(project, prop_name, scm) - else: - return project.get_property(prop_name) - def get_project_config(self, project): - scm = self.project_config_schema - return self._get_one(project, scm.name, scm) + return project.config.get(self.project_config_schema.name, {}) _loaded_modules = {} diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ class ApplierGetView(APILoginRequiredView): def handle(self, request, target_repo=None): q = Message.objects.filter(results__name="git", results__status="pending") if target_repo is not None and target_repo != '': - props = ProjectProperty.objects.filter(name='git.push_to') - # unfortunately startswith does not work with JSONFields, so we have to hack - # around it and look at the raw database representation. This would fail - # if we used PostgreSQL json fields! - target_repo_escaped = json.dumps(target_repo) - if target_repo[-1] != '/': - props = props.extra(where=["value = %s or value like %s"], - params=[target_repo_escaped, - target_repo_escaped[0:-1] + '/%']) - else: - props = props.extra(where=["value like %s"], - params=target_repo_escaped[0:-1] + '%') - q = q.filter(project__in=[prop.project for prop in props.all()]) + # Postgres could use JSON fields instead. Fortunately projects are + # few so this is cheap + def match_target_repo(config, target_repo): + push_to = config.get('git', {}).get('push_to') + if push_to is None: + return False + if target_repo[-1] != '/': + return push_to == target_repo or push_to.startswith(target_repo + '/') + else: + return push_to.startswith(target_repo) + + projects = Project.objects.values_list('id', 'config').all() + projects = [pid for pid, config in projects if match_target_repo(config, target_repo)] + q = q.filter(project__pk__in=projects) m = q.first() if not m: return None diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): html_log_url="URL to test log (HTML)", is_timeout="whether the test has timeout") register_handler("SetProperty", self.on_set_property) + register_handler("SetProjectConfig", self.on_set_config) register_handler("ResultUpdate", self.on_result_update) def on_set_property(self, evt, obj, name, value, old_value): if isinstance(obj, Project) and name == "git.head" \ and old_value != value: self.clear_and_start_testing(obj) - elif isinstance(obj, Project) and name.startswith("testing.tests.") \ - and old_value != value: - self.project_recalc_pending_tests(obj) + + def on_set_config(self, evt, obj): + self.project_recalc_pending_tests(obj) def get_msg_base_tags(self, msg): return [t for t in msg.tags if t.lower().startswith("based-on:")] diff --git a/static/js/config-editor.js b/static/js/config-editor.js index XXXXXXX..XXXXXXX 100644 --- a/static/js/config-editor.js +++ b/static/js/config-editor.js @@ -XXX,XX +XXX,XX @@ function properties_save(btn) { $(btn).addClass("disabled"); $(btn).text("Saving..."); $(btn).parent().find(".save-message").remove(); - patchew_api_do("set-project-properties", + patchew_api_do("set-project-config", { project: current_project(), - properties: props }) + config: props }) .done(function (data) { save_done(btn, true); }) @@ -XXX,XX +XXX,XX @@ function map_add_item(btn) { function map_delete_item(btn) { item = $(btn).parent().parent().parent(); name = item.find(".item-name").text(); - prefix = item.data('property-prefix') + "."; if (!window.confirm("Really delete '" + name +"'?")) { return; } - $(btn).addClass("disabled"); - $(btn).text("Deleting..."); - $(btn).parent().find(".delete-message").remove(); - patchew_api_do("delete-project-properties-by-prefix", - { project: current_project(), - prefix: prefix }) - .done(function (data) { - item.remove(); - }) - .fail(function (data, text, error) { - $(btn).removeClass("disabled"); - $(btn).text("Delete"); - info = $("<div class=\"alert alert-danger delete-message\"></div>"); - info.html("Error: " + error); - info.insertBefore($(btn)); - }); + item.remove(); + confirm_leaving_page(true); } function enum_change(which) { val = $(which).val(); diff --git a/tests/patchewtest.py b/tests/patchewtest.py index XXXXXXX..XXXXXXX 100644 --- a/tests/patchewtest.py +++ b/tests/patchewtest.py @@ -XXX,XX +XXX,XX @@ class PatchewTestCase(dj_test.LiveServerTestCase): def add_project(self, name, mailing_list="", git_repo=""): p = Project(name=name, mailing_list=mailing_list, git=git_repo or self.create_git_repo(name)) - p.save() push_repo = self.create_git_repo(name + "_push") - p.set_property("git.push_to", push_repo) - p.set_property("git.public_repo", push_repo) - p.set_property("git.url_template", push_repo) + p.config = { + "git": { + "push_to": push_repo, + "public_repo": push_repo, + "url_template": push_repo + } + } + p.save() return p def api_login(self): diff --git a/tests/test_git.py b/tests/test_git.py index XXXXXXX..XXXXXXX 100755 --- a/tests/test_git.py +++ b/tests/test_git.py @@ -XXX,XX +XXX,XX @@ class GitTest(PatchewTestCase): self.cli_login() self.repo = self.create_git_repo() self.p = self.add_project("QEMU", "qemu-devel@nongnu.org", git_repo=self.repo) - self.p.set_property("git.push_to", self.repo) - self.p.set_property("git.public_repo", self.repo) - self.p.set_property("git.url_template", self.repo + " %t") + self.p.config = { + "git": { + "push_to": self.repo, + "public_repo": self.repo, + "url_template": self.repo + " %t" + } + } + self.p.save() self.PROJECT_BASE = '%sprojects/%d/' % (self.REST_BASE, self.p.id) def cleanUp(self): diff --git a/tests/test_testing.py b/tests/test_testing.py index XXXXXXX..XXXXXXX 100755 --- a/tests/test_testing.py +++ b/tests/test_testing.py @@ -XXX,XX +XXX,XX @@ from .patchewtest import PatchewTestCase, main def create_test(project, name, requirements="", script="#!/bin/bash\ntrue"): - prefix = "testing.tests." + name + "." - project.set_property(prefix + "timeout", 3600) - project.set_property(prefix + "enabled", True) - project.set_property(prefix + "script", script) - project.set_property(prefix + "requirements", requirements) + testing = project.config.setdefault('testing', {}) + tests = testing.setdefault('tests', {}) + tests[name] = { + 'timeout': 3600, + 'enabled': True, + 'script': script, + 'requirements': requirements + } + project.save() + + +def create_requirement(project, name, script="#!/bin/bash\ntrue"): + testing = project.config.setdefault('testing', {}) + requirements = testing.setdefault('requirements', {}) + requirements[name] = { + 'script': script, + } + project.save() class TestingTestCase(PatchewTestCase, metaclass=abc.ABCMeta): @@ -XXX,XX +XXX,XX @@ class TesterTest(PatchewTestCase): create_test(self.p2, "b") self.p3 = self.add_project("ALLOW", "qemu-devel@nongnu.org") - self.p3.set_property("testing.requirements.allow.script", - "#!/bin/sh\ntrue") + create_requirement(self.p3, 'allow', "#!/bin/sh\ntrue") create_test(self.p3, "c", "allow") self.p4 = self.add_project("DENY", "qemu-devel@nongnu.org") - self.p4.set_property("testing.requirements.deny.script", - "#!/bin/sh\nfalse") + create_requirement(self.p4, 'deny', "#!/bin/sh\nfalse") create_test(self.p4, "d", "deny") self.cli_login() @@ -XXX,XX +XXX,XX @@ class TestingDisableTest(PatchewTestCase): self.cli_login() self.cli_import('0013-foo-patch.mbox.gz') self.do_apply() - self.p1.set_property("testing.tests.a.enabled", False) + self.p1.config['testing']['tests']['a']['enabled'] = False + self.p1.save() out, err = self.check_cli(["tester", "-p", "QEMU", "--no-wait"]) self.assertNotIn("Project: QEMU\n", out) self.cli_logout() -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Add a new endpoint /projects/ID/config/ that allows retrieving and setting the project's module configuration. The endpoint is only accessible to maintainers, because it could contain secrets. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- api/rest.py | 18 +++++++++++++++++- tests/test_rest.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/api/rest.py b/api/rest.py index XXXXXXX..XXXXXXX 100644 --- a/api/rest.py +++ b/api/rest.py @@ -XXX,XX +XXX,XX @@ class PatchewPermission(permissions.BasePermission): def is_superuser(self, request): return request.user and request.user.is_superuser + def is_safe_method(self, request): + return request.method in permissions.SAFE_METHODS + def has_project_permission(self, request, view, obj): return obj.maintained_by(request.user) @@ -XXX,XX +XXX,XX @@ class PatchewPermission(permissions.BasePermission): return False def has_generic_permission(self, request, view): - return (request.method in permissions.SAFE_METHODS) or \ + return self.is_safe_method(request) or \ self.is_superuser(request) or \ self.has_group_permission(request, view, self.allowed_groups) @@ -XXX,XX +XXX,XX @@ class PatchewPermission(permissions.BasePermission): self.has_project_permission(request, view, obj) +class MaintainerPermission(PatchewPermission): + def is_safe_method(self, request): + return False + + class ImportPermission(PatchewPermission): allowed_groups = ('importers',) @@ -XXX,XX +XXX,XX @@ class ProjectsViewSet(viewsets.ModelViewSet): serializer_class = ProjectSerializer permission_classes = (PatchewPermission,) + @action(methods=['get', 'put'], detail=True, permission_classes=[MaintainerPermission]) + def config(self, request, pk=None): + project = self.get_object() + if request.method == 'PUT': + project.config = request.data + project.save() + return Response(project.config) + @action(methods=['post'], detail=True, permission_classes=[ImportPermission]) def update_project_head(self, request, pk=None): """ diff --git a/tests/test_rest.py b/tests/test_rest.py index XXXXXXX..XXXXXXX 100755 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -XXX,XX +XXX,XX @@ class RestTest(PatchewTestCase): self.assertEquals(resp.data['name'], "QEMU") self.assertEquals(resp.data['mailing_list'], "qemu-devel@nongnu.org") + def test_project_config_get(self): + self.p.config = { + "git": { + "push_to": "/tmp/aaa" + } + } + self.p.save() + resp = self.api_client.get(self.PROJECT_BASE + 'config/') + self.assertEquals(resp.status_code, 403) + self.api_client.login(username=self.user, password=self.password) + resp = self.api_client.get(self.PROJECT_BASE + 'config/') + self.assertEquals(resp.status_code, 200) + self.assertEquals(resp.data['git']['push_to'], '/tmp/aaa') + + def test_project_config_put(self): + new_config = { + "git": { + "push_to": "/tmp/bbb" + } + } + resp = self.api_client.put(self.PROJECT_BASE + 'config/', new_config, format='json') + self.assertEquals(resp.status_code, 403) + self.api_client.login(username=self.user, password=self.password) + resp = self.api_client.put(self.PROJECT_BASE + 'config/', new_config, format='json') + self.assertEquals(resp.status_code, 200) + self.assertEquals(resp.data['git']['push_to'], '/tmp/bbb') + resp = self.api_client.get(self.PROJECT_BASE + 'config/') + self.assertEquals(resp.status_code, 200) + self.assertEquals(resp.data['git']['push_to'], '/tmp/bbb') + def test_update_project_head(self): resp = self.apply_and_retrieve('0001-simple-patch.mbox.gz', self.p.id, '20160628014747.20971-1-famz@redhat.com') -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Flags are used as a replacement for Boolean properties that are used by search queries. They can be searched efficiently using a trigram index. --- api/migrations/0048_message_flags.py | 20 +++++++++ api/migrations/0049_populate_message_flags.py | 42 +++++++++++++++++++ .../0050_message_flags_postgres_fts.py | 22 ++++++++++ api/models.py | 13 ++++++ api/search.py | 13 +++--- mods/tags.py | 5 ++- mods/testing.py | 22 +++++----- tests/test_testing.py | 9 ++-- 8 files changed, 124 insertions(+), 22 deletions(-) create mode 100644 api/migrations/0048_message_flags.py create mode 100644 api/migrations/0049_populate_message_flags.py create mode 100644 api/migrations/0050_message_flags_postgres_fts.py diff --git a/api/migrations/0048_message_flags.py b/api/migrations/0048_message_flags.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0048_message_flags.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-04-18 14:30 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0047_populate_project_config'), + ] + + operations = [ + migrations.AddField( + model_name='message', + name='flags', + field=models.CharField(blank=True, default='', max_length=64), + ), + ] diff --git a/api/migrations/0049_populate_message_flags.py b/api/migrations/0049_populate_message_flags.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0049_populate_message_flags.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations + +from api.search import FLAG_TESTED, FLAG_REVIEWED, FLAG_OBSOLETE + +def property_to_flags(apps, schema_editor): + MessageProperty = apps.get_model('api', 'MessageProperty') + for p in MessageProperty.objects.filter(name='obsoleted-by'): + p.message.flags += FLAG_OBSOLETE + p.message.save() + for p in MessageProperty.objects.filter(name='reviewed'): + p.message.flags += FLAG_REVIEWED + p.message.save() + for p in MessageProperty.objects.filter(name='testing.done'): + p.message.flags += FLAG_TESTED + p.message.save() + MessageProperty.objects.filter(name='reviewed').delete() + MessageProperty.objects.filter(name='testing.done').delete() + +def flags_to_property(apps, schema_editor): + Message = apps.get_model('api', 'Message') + for m in Message.objects.exclude(flags=''): + if '[reviewed]' in m.flags: + new_prop = MessageProperty(message=p.message, name='reviewed', value=True) + new_prop.save() + if FLAG_TESTED in m.flags: + new_prop = MessageProperty(message=p.message, name='testing.done', value=True) + new_prop.save() + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0048_message_flags'), + ] + + operations = [ + migrations.RunPython(property_to_flags, + reverse_code=flags_to_property), + ] diff --git a/api/migrations/0050_message_flags_postgres_fts.py b/api/migrations/0050_message_flags_postgres_fts.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0050_message_flags_postgres_fts.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.16 on 2018-11-07 15:28 +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion +from django.contrib.postgres.operations import TrigramExtension + +from api.migrations import PostgresOnlyMigration + + +class Migration(PostgresOnlyMigration): + + dependencies = [ + ('api', '0049_populate_message_flags'), + ] + + operations = [ + TrigramExtension(), + migrations.RunSQL("create index api_message_flags_gin on api_message using gin(flags gin_trgm_ops);", + "drop index api_message_flags_gin"), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class Message(models.Model): objects = MessageManager() maintainers = jsonfield.JSONField(blank=True, default=[]) + flags = models.CharField(max_length=64, blank=True, default="") def save_mbox(self, mbox_blob): save_blob(mbox_blob, self.message_id) @@ -XXX,XX +XXX,XX @@ class Message(models.Model): self.refresh_num_patches() return self.num_patches + def add_flag(self, flag): + assert flag[0] == '[' and flag[-1] == ']' + if not flag in self.flags: + self.flags += flag + self.save() + + def remove_flag(self, flag): + assert flag[0] == '[' and flag[-1] == ']' + if flag in self.flags: + self.flags = self.flags.replace(flag, '') + self.save() + def get_property(self, prop, default=None): return self.get_properties().get(prop, default) diff --git a/api/search.py b/api/search.py index XXXXXXX..XXXXXXX 100644 --- a/api/search.py +++ b/api/search.py @@ -XXX,XX +XXX,XX @@ from django.contrib.postgres.search import SearchQuery, SearchVector, SearchVect from django.db.models import Lookup from django.db.models.fields import Field +# These are used by migrations. Do not change them! +FLAG_REVIEWED = '[reviewed]' +FLAG_OBSOLETE = '[obsolete]' +FLAG_TESTED = '[tested]' @Field.register_lookup class NotEqual(Lookup): @@ -XXX,XX +XXX,XX @@ Search text keyword in the email message. Example: self._add_to_keywords('PULL') return Q(subject__contains='[PULL') | Q(subject__contains='[GIT PULL') elif cond == "reviewed": - return self._make_filter_subquery(MessageProperty, Q(name="reviewed", value=True)) + return Q(flags__contains=FLAG_REVIEWED) elif cond in ("obsoleted", "old"): - return self._make_filter_subquery( - MessageProperty, - Q(name="obsoleted-by", value__isnull=False) & ~Q(name="obsoleted-by", value__iexact='') - ) + return Q(flags__contains=FLAG_OBSOLETE) elif cond == "applied": return self._make_filter_subquery(MessageResult, Q(name="git", status=Result.SUCCESS)) elif cond == "tested": - return self._make_filter_subquery(MessageProperty, Q(name="testing.done", value=True)) + return Q(flags__contains=FLAG_TESTED) elif cond == "merged": return Q(is_merged=True) return None diff --git a/mods/tags.py b/mods/tags.py index XXXXXXX..XXXXXXX 100644 --- a/mods/tags.py +++ b/mods/tags.py @@ -XXX,XX +XXX,XX @@ from mbox import addr_db_to_rest, parse_address from event import register_handler, emit_event, declare_event from api.models import Message from api.rest import PluginMethodField +from api.search import FLAG_OBSOLETE, FLAG_REVIEWED import rest_framework REV_BY_PREFIX = "Reviewed-by:" @@ -XXX,XX +XXX,XX @@ series cover letter, patch mail body and their replies. return m1.version > m2.version and m1.date >= m2.date for m in series.get_alternative_revisions(): if newer_than(m, series): + series.add_flag(FLAG_OBSOLETE) series.set_property("obsoleted-by", m.message_id) elif newer_than(series, m): + m.add_flag(FLAG_OBSOLETE) m.set_property("obsoleted-by", series.message_id) updated = self.update_tags(series) @@ -XXX,XX +XXX,XX @@ series cover letter, patch mail body and their replies. series_reviewers = _find_reviewers(series) reviewers = reviewers.union(series_reviewers) if num_reviewed == series.get_num()[1] or series_reviewers: - series.set_property("reviewed", True) + series.add_flag(FLAG_REVIEWED) series.set_property("reviewers", list(reviewers)) if updated: emit_event("TagsUpdate", series=series) diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ from api.views import APILoginRequiredView from api.models import (Message, MessageProperty, MessageResult, Project, ProjectResult, Result) from api.rest import PluginMethodField, reverse_detail -from api.search import SearchEngine +from api.search import SearchEngine, FLAG_TESTED from event import emit_event, declare_event, register_handler from patchew.logviewer import LogView from schema import * @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): _instance.tester_check_in(po, result.data['tester']) if not self.get_testing_results(obj, status__in=(Result.PENDING, Result.RUNNING)).exists(): - obj.set_property("testing.done", True) obj.set_property("testing.tested-head", result.data["head"]) if isinstance(obj, Message): + obj.add_flag(FLAG_TESTED) obj.set_property("testing.tested-base", self.get_msg_base_tags(obj)) if isinstance(obj, Project): @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): for tn in all_tests: if not tn in done_tests: obj.create_result(name='testing.' + tn, status=Result.PENDING).save() - if len(done_tests) < len(all_tests): - obj.set_property("testing.done", None) - return - obj.set_property("testing.done", True) + if isinstance(obj, Message): + if len(all_tests) and len(done_tests) == len(all_tests): + obj.add_flag(FLAG_TESTED) + else: + obj.remove_flag(FLAG_TESTED) def project_recalc_pending_tests(self, project): self.recalc_pending_tests(project) @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): self.recalc_pending_tests(obj) def clear_and_start_testing(self, obj, test=""): - for k in list(obj.get_properties().keys()): - if k == "testing.done" or \ - k == "testing.tested-head": - obj.set_property(k, None) + obj.set_property("testing.tested-head", None) + if isinstance(obj, Message): + obj.remove_flag(FLAG_TESTED) if test: r = self.get_testing_result(obj, test) if r: @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): "type": "danger", "char": "T", }) - elif message.get_property("testing.done"): + elif FLAG_TESTED in message.flags: message.status_tags.append({ "title": "Testing passed", "url": reverse("series_detail", diff --git a/tests/test_testing.py b/tests/test_testing.py index XXXXXXX..XXXXXXX 100755 --- a/tests/test_testing.py +++ b/tests/test_testing.py @@ -XXX,XX +XXX,XX @@ import abc import subprocess from api.models import Message, Result +from api.search import FLAG_TESTED from .patchewtest import PatchewTestCase, main @@ -XXX,XX +XXX,XX @@ class TestingTestCase(PatchewTestCase, metaclass=abc.ABCMeta): if 'status' not in kwargs: kwargs['status'] = Result.SUCCESS self.modify_test_result(obj, **kwargs) - obj.set_property("testing.done", True) def do_testing_report(self, **report): self.api_login() @@ -XXX,XX +XXX,XX @@ class MessageTestingTest(TestingTestCase): def do_testing_done(self, **kwargs): self._do_testing_done(self.msg, **kwargs) + self.msg.add_flag(FLAG_TESTED) def do_testing_report(self, **report): r = super(MessageTestingTest, self).do_testing_report(**report) @@ -XXX,XX +XXX,XX @@ class TestingResetTest(PatchewTestCase): "testing.a": Result.SUCCESS, "testing.b": Result.SUCCESS, "testing.c": Result.FAILURE}) - self.assertTrue(msg.get_property("testing.done")) + self.assertIn(FLAG_TESTED, msg.flags) self.api_login() self.client.post('/login/', {'username': self.user, 'password': self.password}) @@ -XXX,XX +XXX,XX @@ class TestingResetTest(PatchewTestCase): "testing.a": Result.PENDING, "testing.b": Result.SUCCESS, "testing.c": Result.FAILURE}) - self.assertFalse(msg.get_property("testing.done")) + self.assertNotIn(FLAG_TESTED, msg.flags) self.client.get('/testing-reset/%s/?type=message&test=b' % msg.message_id) self.client.get('/testing-reset/%s/?type=message&test=c' % msg.message_id) @@ -XXX,XX +XXX,XX @@ class TestingResetTest(PatchewTestCase): "testing.a": Result.PENDING, "testing.b": Result.PENDING, "testing.c": Result.PENDING}) - self.assertFalse(msg.get_property("testing.done")) + self.assertNotIn(FLAG_TESTED, msg.flags) class TestingDisableTest(PatchewTestCase): -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel This is the first step towards storing properties in a single JSON dictionary, which is simpler now that properties are fewer and smaller than they used to be. The related name "properties" has to be freed up for the new field. --- api/migrations/0051_auto_20190418_1346.py | 33 +++++++++++++++++++++++ api/models.py | 9 ++++--- 2 files changed, 38 insertions(+), 4 deletions(-) create mode 100644 api/migrations/0051_auto_20190418_1346.py diff --git a/api/migrations/0051_auto_20190418_1346.py b/api/migrations/0051_auto_20190418_1346.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0051_auto_20190418_1346.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-04-18 13:46 +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion +import jsonfield.encoder +import jsonfield.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0050_message_flags_postgres_fts'), + ] + + operations = [ + migrations.AddField( + model_name='message', + name='properties', + field=jsonfield.fields.JSONField(default={}, dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, load_kwargs={}), + ), + migrations.AddField( + model_name='project', + name='properties', + field=jsonfield.fields.JSONField(default={}, dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, load_kwargs={}), + ), + migrations.AlterField( + model_name='messageproperty', + name='message', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='api.Message'), + ), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class Project(models.Model): "parent_project=NULL")) maintainers = models.ManyToManyField(User, blank=True) config = jsonfield.JSONField(default={}) + properties = jsonfield.JSONField(default={}) def __str__(self): return self.name @@ -XXX,XX +XXX,XX @@ class MessageManager(models.Manager): return None else: q = super(MessageManager, self).get_queryset() - return q.filter(is_series_head=True).prefetch_related('properties', 'project') + return q.filter(is_series_head=True).prefetch_related('messageproperty_set', 'project') def find_series(self, message_id, project_name=None): heads = self.series_heads(project_name) @@ -XXX,XX +XXX,XX @@ class Message(models.Model): maintainers = jsonfield.JSONField(blank=True, default=[]) flags = models.CharField(max_length=64, blank=True, default="") + properties = jsonfield.JSONField(default={}) def save_mbox(self, mbox_blob): save_blob(mbox_blob, self.message_id) @@ -XXX,XX +XXX,XX @@ class Message(models.Model): # The prefetch cache is invalidated, query again all_props = MessageProperty.objects.filter(message=self) else: - all_props = self.properties.all() + all_props = self.messageproperty_set.all() r = {} for m in all_props: r[m.name] = m.value @@ -XXX,XX +XXX,XX @@ class MessageResult(Result): class MessageProperty(models.Model): - message = models.ForeignKey('Message', on_delete=models.CASCADE, - related_name='properties') + message = models.ForeignKey('Message', on_delete=models.CASCADE) name = models.CharField(max_length=256) value = jsonfield.JSONField() -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel Fortunately, most accesses to the property tables were hidden behind accessors. Therefore, apart from the usual ugly migration (which in this case is more or less a copy of the config migration) it is almost enough to change the accessors to look into the JSON dictionary. We can also use the new API to access a whole subset of the properties, which simplifies the access to testing.check_in.*. --- .../0052_populate_property_fields.py | 67 +++++++++++ api/models.py | 111 ++++++++++-------- mods/testing.py | 6 +- 3 files changed, 130 insertions(+), 54 deletions(-) create mode 100644 api/migrations/0052_populate_property_fields.py diff --git a/api/migrations/0052_populate_property_fields.py b/api/migrations/0052_populate_property_fields.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0052_populate_property_fields.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations +from django.db.models import Q + +def do_properties_to_field(obj, propset): + properties = {} + props = propset.all() + for p in props: + *path, last = p.name.split('.') + parent = properties + for item in path: + parent = parent.setdefault(item, {}) + parent[last] = p.value + obj.properties = properties + #print(obj, properties) + obj.save() + props.delete() + +def properties_to_field(apps, schema_editor): + Project = apps.get_model('api', 'Project') + for po in Project.objects.all(): + do_properties_to_field(po, po.projectproperty_set) + Message = apps.get_model('api', 'Message') + for po in Message.objects.all(): + do_properties_to_field(po, po.messageproperty_set) + +def flatten_properties(source, prefix, result=None): + if result is None: + result = {} + for k, v in source.items(): + if isinstance(v, dict): + flatten_properties(v, prefix + k + '.', result) + else: + result[prefix + k] = v + return result + +def do_field_to_properties(source, propclass, **kwargs): + props = flatten_properties(source, '') + for k, v in props.items(): + #print(k, v) + new_prop = propclass(name=k, value=v, **kwargs) + new_prop.save() + +def field_to_properties(apps, schema_editor): + Project = apps.get_model('api', 'Project') + ProjectProperty = apps.get_model('api', 'ProjectProperty') + for po in Project.objects.all(): + do_field_to_properties(po.properties, ProjectProperty, project=po) + Message = apps.get_model('api', 'Message') + MessageProperty = apps.get_model('api', 'MessageProperty') + for m in Message.objects.all(): + do_field_to_properties(m.properties, MessageProperty, message=m) + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0051_auto_20190418_1346'), + ] + + operations = [ + migrations.RunPython(properties_to_field, + reverse_code=field_to_properties), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class Project(models.Model): emit_event("SetProjectConfig", obj=self) def get_property(self, prop, default=None): - a = ProjectProperty.objects.filter(project=self, name=prop).first() - if a: - return a.value - else: - return default - - def get_properties(self): - r = {} - for m in ProjectProperty.objects.filter(project=self): - r[m.name] = m.value - return r - - def _do_set_property(self, prop, value): - if value is None: - ProjectProperty.objects.filter(project=self, name=prop).delete() + x = self.properties + *path, last = prop.split('.') + for item in path: + if not item in x: + return default + x = x[item] + return x.get(last, default) + + def delete_property(self, prop): + x = self.properties + *path, last = prop.split('.') + for item in path: + if not item in x: + return + x = x[item] + if not last in x: return - pp, created = ProjectProperty.objects.get_or_create(project=self, - name=prop) - pp.value = value - pp.save() + old_val = x[last] + del x[last] + self.save() + emit_event("SetProperty", obj=self, name=prop, value=None, + old_value=old_val) def set_property(self, prop, value): - old_val = self.get_property(prop) - self._do_set_property(prop, value) + if value is None: + self.delete_property(prop) + return + x = self.properties + *path, last = prop.split('.') + for item in path: + x = x.setdefault(item, {}) + old_val = x.get(last) + x[last] = value + self.save() emit_event("SetProperty", obj=self, name=prop, value=value, old_value=old_val) @@ -XXX,XX +XXX,XX @@ class Message(models.Model): self.save() def get_property(self, prop, default=None): - return self.get_properties().get(prop, default) - - def get_properties(self): - if hasattr(self, '_properties'): - if self._properties is not None: - return self._properties - else: - # The prefetch cache is invalidated, query again - all_props = MessageProperty.objects.filter(message=self) - else: - all_props = self.messageproperty_set.all() - r = {} - for m in all_props: - r[m.name] = m.value - self._properties = r - return r - - def _do_set_property(self, prop, value): - if value is None: - MessageProperty.objects.filter(message=self, name=prop).delete() + x = self.properties + *path, last = prop.split('.') + for item in path: + if not item in x: + return default + x = x[item] + return x.get(last, default) + + def delete_property(self, prop): + x = self.properties + *path, last = prop.split('.') + for item in path: + if not item in x: + return + x = x[item] + if not last in x: return - mp, created = MessageProperty.objects.get_or_create(message=self, - name=prop) - mp.value = value - mp.save() - # Invalidate cache - self._properties = None + old_val = x[last] + del x[last] + self.save() + emit_event("SetProperty", obj=self, name=prop, value=None, + old_value=old_val) def set_property(self, prop, value): - old_val = self.get_property(prop) - self._do_set_property(prop, value) + if value is None: + self.delete_property(prop) + return + x = self.properties + *path, last = prop.split('.') + for item in path: + x = x.setdefault(item, {}) + old_val = x.get(last) + x[last] = value + self.save() emit_event("SetProperty", obj=self, name=prop, value=value, old_value=old_val) diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ class TestingModule(PatchewModule): def check_active_testers(self, project): at = [] - for k, v in project.get_properties().items(): - prefix = "testing.check_in." - if not k.startswith(prefix): - continue + for tn, v in project.get_property('testing.check_in', {}).items(): age = time.time() - v if age > 10 * 60: continue - tn = k[len(prefix):] at.append("%s (%dmin)" % (tn, math.ceil(age / 60))) if not at: return -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel --- api/admin.py | 14 +-------- api/migrations/0053_auto_20190418_1357.py | 37 +++++++++++++++++++++++ api/models.py | 29 +----------------- api/search.py | 2 +- mods/git.py | 3 +- mods/testing.py | 3 +- 6 files changed, 42 insertions(+), 46 deletions(-) create mode 100644 api/migrations/0053_auto_20190418_1357.py diff --git a/api/admin.py b/api/admin.py index XXXXXXX..XXXXXXX 100644 --- a/api/admin.py +++ b/api/admin.py @@ -XXX,XX +XXX,XX @@ # http://opensource.org/licenses/MIT. from django.contrib import admin -from .models import Message, MessageProperty, Module, Project, ProjectProperty +from .models import Message, Module, Project from mod import get_module -class ProjectPropertyInline(admin.TabularInline): - model = ProjectProperty - extra = 0 - - class ProjectAdmin(admin.ModelAdmin): filter_horizontal = ('maintainers',) - inlines = [ProjectPropertyInline] - - -class MessagePropertyInline(admin.TabularInline): - model = MessageProperty - extra = 0 class MessageAdmin(admin.ModelAdmin): - inlines = [MessagePropertyInline] list_filter = [('is_series_head')] search_fields = [ 'message_id', diff --git a/api/migrations/0053_auto_20190418_1357.py b/api/migrations/0053_auto_20190418_1357.py new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/api/migrations/0053_auto_20190418_1357.py @@ -XXX,XX +XXX,XX @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-04-18 13:57 +from __future__ import unicode_literals + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0052_populate_property_fields'), + ] + + operations = [ + migrations.AlterUniqueTogether( + name='messageproperty', + unique_together=set([]), + ), + migrations.RemoveField( + model_name='messageproperty', + name='message', + ), + migrations.AlterUniqueTogether( + name='projectproperty', + unique_together=set([]), + ), + migrations.RemoveField( + model_name='projectproperty', + name='project', + ), + migrations.DeleteModel( + name='MessageProperty', + ), + migrations.DeleteModel( + name='ProjectProperty', + ), + ] diff --git a/api/models.py b/api/models.py index XXXXXXX..XXXXXXX 100644 --- a/api/models.py +++ b/api/models.py @@ -XXX,XX +XXX,XX @@ class ProjectResult(Result): return self.project -class ProjectProperty(models.Model): - project = models.ForeignKey('Project', on_delete=models.CASCADE) - name = models.CharField(max_length=1024, db_index=True) - value = jsonfield.JSONField() - - class Meta: - unique_together = ('project', 'name',) - verbose_name_plural = "Project Properties" - - declare_event("SeriesComplete", project="project object", series="series instance that is marked complete") declare_event("SeriesMerged", project="project object", @@ -XXX,XX +XXX,XX @@ class MessageManager(models.Manager): return None else: q = super(MessageManager, self).get_queryset() - return q.filter(is_series_head=True).prefetch_related('messageproperty_set', 'project') + return q.filter(is_series_head=True).prefetch_related('project') def find_series(self, message_id, project_name=None): heads = self.series_heads(project_name) @@ -XXX,XX +XXX,XX @@ class MessageResult(Result): return self.message -class MessageProperty(models.Model): - message = models.ForeignKey('Message', on_delete=models.CASCADE) - name = models.CharField(max_length=256) - value = jsonfield.JSONField() - - def __str__(self): - if len(self.value) > 30: - val_prev = self.value[:30] + "..." - else: - val_prev = self.value - return "%s: %s = %s" % (self.message.subject, self.name, val_prev) - - class Meta: - unique_together = ('message', 'name',) - verbose_name_plural = "Message Properties" - - class Module(models.Model): """ Module information """ name = models.CharField(max_length=128, unique=True) diff --git a/api/search.py b/api/search.py index XXXXXXX..XXXXXXX 100644 --- a/api/search.py +++ b/api/search.py @@ -XXX,XX +XXX,XX @@ # This work is licensed under the MIT License. Please see the LICENSE file or # http://opensource.org/licenses/MIT. -from .models import Message, MessageProperty, MessageResult, Result, QueuedSeries +from .models import Message, MessageResult, Result, QueuedSeries from functools import reduce from django.db import connection diff --git a/mods/git.py b/mods/git.py index XXXXXXX..XXXXXXX 100644 --- a/mods/git.py +++ b/mods/git.py @@ -XXX,XX +XXX,XX @@ from django.utils.html import format_html from django.db.models import Q from mod import PatchewModule from event import declare_event, register_handler, emit_event -from api.models import (Message, MessageProperty, Project, - ProjectProperty, Result) +from api.models import (Message, Project, Result) from api.rest import PluginMethodField, reverse_detail from api.views import APILoginRequiredView, prepare_series from patchew.logviewer import LogView diff --git a/mods/testing.py b/mods/testing.py index XXXXXXX..XXXXXXX 100644 --- a/mods/testing.py +++ b/mods/testing.py @@ -XXX,XX +XXX,XX @@ import datetime import time import math from api.views import APILoginRequiredView -from api.models import (Message, MessageProperty, MessageResult, - Project, ProjectResult, Result) +from api.models import (Message, MessageResult, Project, ProjectResult, Result) from api.rest import PluginMethodField, reverse_detail from api.search import SearchEngine, FLAG_TESTED from event import emit_event, declare_event, register_handler -- 2.21.0 _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel