Stephen Finucane [Fri, 16 Nov 2018 15:47:43 +0000 (16:47 +0100)]
REST: Handle unset series
This was introduced in the recent "convert series from N:M to 1:N"
series. We take the opportunity to make the 'create_patch' and
'create_cover' utility methods a little smarter, in that series will
automatically be created for the patch/cover letter unless told not to.
This requires some related changes to other modules.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Sun, 28 Oct 2018 14:43:31 +0000 (14:43 +0000)]
tests: Add 'store_samples' decorator to 'test_bundle'
Add the decorator to the 'test_bundle' test class. This involves
splitting up the test cases so that each test case we care about makes
only a single request. We also add a missing test to ensure private
bundles cannot be shown by anyone but the owner.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Sun, 28 Oct 2018 14:40:31 +0000 (14:40 +0000)]
tests: Add 'store_samples' decorator
We want to start including sample API requests and responses in our
documentation. Given that these may get out of date over time, we
should really generate these things dynamically. Create a decorator that
will allow us to do just that.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Sat, 20 Oct 2018 22:20:52 +0000 (23:20 +0100)]
REST: Add additional documentation
As noted in the Django REST Framework docs [1], views that support
multiple methods can and should split their documentation using
'method:' style delimiters. Do just this.
Stephen Finucane [Tue, 30 Oct 2018 22:04:23 +0000 (22:04 +0000)]
README: Note latest version of requirements
These were missed previously
Signed-off-by: Stephen Finucane <stephen@that.guru> Fixes: 8eb3719a ("Add support for django-filter 2.0") Fixes: fab5571e ("Add support for Django REST Framework 3.9")
Stephen Finucane [Tue, 30 Oct 2018 11:35:41 +0000 (11:35 +0000)]
travis: Stop testing PostgreSQL 11
Travis seems to be doing something really weird with PG11 since it was
released a few weeks ago. This is currently breaking our CI and can't
continue. It doesn't seem possible to mark this as an expected failure
so simply remove it. We can re-add this once Travis gains proper support
for this version via their addons.
Signed-off-by: Stephen Finucane <stephen@that.guru> Suggested-by: Daniel Black <daniel@linux.ibm.com> Acked-by: Daniel Axtens <dja@axtens.net>
Stephen Finucane [Fri, 26 Oct 2018 09:34:34 +0000 (10:34 +0100)]
trivial: Use implicit string concatenation
While pycodestyle's W504 ("line break after binary operator") error was
recently disabled, it did highlight a number of areas where explicit
string concatenation was being used despite it not be necessary. Fix
these while we're aware of them.
Stephen Finucane [Fri, 26 Oct 2018 09:39:29 +0000 (10:39 +0100)]
tox: Disable W504 ("line break after binary operator")
This was introduced in a recent version of 'pycodestyle'. The
documentation notes [1] that it is mutually exclusive with W503, which
we do enforce, suggesting that we disable one or the other. Avoid the
churn and stick to the older rule, which I personally prefer.
Stephen Finucane [Sun, 21 Oct 2018 11:16:15 +0000 (12:16 +0100)]
README: Use pyup.io badge instead of requires.io
requires.io has an open issue whereby it doesn't consider Python version
in assessing the latest version of a package available. pyup.io also has
the same issue but we've already worked around this by ignoring the
offending lines. Seeing as we're already using that elsewhere, double
down on it.
Signed-off-by: Stephen Finucane <stephen@that.guru>
The 'SeriesPatch' object was recently removed, but the
'create_series_patch' was retained in order to minimize the changes
necessary. This can now be removed and the logic moved to the
'create_patch' and 'create_cover' functions instead.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Sat, 19 May 2018 02:42:18 +0000 (03:42 +0100)]
models: Convert Series-Patch relationship to 1:N
Late in the development of the series feature, it was decided that there
were advantages to allowing an N:M relationship between series and
patches. This would allow us to do things like create complete series
where a sole vN patch was sent to a list rather than the full series.
After some time using series in the wild, it's apparent that such
features are very difficult to implement correctly and will likely never
be implemented. As such, it's time to start cleaning up the mess, paving
the way for things like an improved tagging feature.
There are some significant changes to the model required:
1. - Add 'Patch.series_alt' and 'Patch.number' fields.
2. - Populate the 'Patch.series_alt' and 'Patch.number' fields from
their 'SeriesPatch' equivalents.
3. - Remove the 'SeriesPatch' model.
- Rename 'Patch.series_alt' to 'Patch.series'.
- Change 'Series.cover_letter' to a 'OneToOneField' since a cover
letter can no longer be assigned to multiple series.
Note that the migrations have to be split into multiple parts as the
combined migration raises an OperationalError as below.
(1072, "Key column 'series_alt_id' doesn't exist in table")
This is due to Django's penchant for creating indexes for newly
created fields, as noted here: https://stackoverflow.com/q/35158530/
Aside from the model changes, there are numerous other changes required:
- admin.py
Reflect model changes for the 'PatchInline' inline used by
'SeriesAdmin'
- api/cover.py, api/patch.py
Update the 'series' field for the cover letter and patch resources to
reflect the model changes. A 'to_representation' function is added in
both cases to post-process this field and make it look like a list
again. This is necessary to avoid breaking clients.
- parser.py
Update to reflect the replacement of 'SeriesPatch' with 'Patch'.
- signals.py
Update to filter on changes to 'Patch' instead of 'SeriesPatch'. This
requires some reworking due to how we set these fields now, as we can
no longer receive on 'post_save' signals for 'SeriesPatch' and must
instead watch for 'pre_save' on 'Patch', which is what we do for
delegate and state changes on same.
- templates/patchwork/*.html
Remove logic that handled multiple series in favour of the (simpler)
single series logic.
- tests/*
Modify the 'create_series_patch' helper to reflect the removal of the
'SeriesPatch' model. This entire helper will be removed in a future
change. Improve some tests to cover edge cases that were highlighted
during development
Unfortunately, all of the above changes must go in at the same time,
otherwise we end up with either (a) broken views, API etc. or (b) split
brain because we need to keep the new single-series fields alongside the
older multi-series fields and models while we rework the views. It's
unfortunate but there's not much to be done here.
Signed-off-by: Stephen Finucane <stephen@that.guru> Tested-by: Daniel Axtens <dja@axtens.net>
Stephen Finucane [Thu, 11 Oct 2018 10:47:56 +0000 (11:47 +0100)]
REST: Allow setting of values using embedded serializers
Unfortunately, the use of embedded serializers for some fields breaks
the ability to update these fields, either via the HTML interface (where
the widget is totally busted) or via a client like 'git-pw'. What we
actually want is to be able to update these fields like normal primary
key but show them using the embedded serializer. We do just this by
using a modified variant of the PrimaryKeyRelatedField and using the
serializers simply for displaying.
Signed-off-by: Stephen Finucane <stephen@that.guru> Closes: #216
Stephen Finucane [Thu, 11 Oct 2018 13:53:25 +0000 (14:53 +0100)]
REST: Validate patch delegate
At present, only users who are maintainers of projects can be delegated
a project. Validate this. This is currently broken due to #216 but that
will be fixed in a future change.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Sat, 29 Sep 2018 20:11:22 +0000 (21:11 +0100)]
filters: Pre-populate delegate, submitter filters
This appears to have got lost in the transition to 'selectize.js'. In
brief, this will ensure that a previously selected delegate or submitter
is still enabled post-filtering. For more information, see [1].
Stephen Finucane [Wed, 10 Oct 2018 10:20:21 +0000 (11:20 +0100)]
settings: Do configure logging for parsearchive (but differently)
In commit aa266a28, we removed the logging configuration for the
'parsearchive' tool because it was using the same configuration as
'parsemail' and could send an email to administrators for ERROR logs.
While we shouldn't be doing that, we should be configuring _some_ kind
of logging.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Wed, 10 Oct 2018 09:33:50 +0000 (10:33 +0100)]
requirements: Add sqlparse to test requirements
This was previously installed as part of 'django-debug-toolbar' [1] but
as this dependency is no longer installed for tests, the dependency is
now missing. Fix this by manually specifying it.
Stephen Finucane [Thu, 13 Sep 2018 18:30:34 +0000 (12:30 -0600)]
doc: Cleanup development installation guide
Use literals where possible, don't leave a space between a term and
definition, and note that Python 3.4, not 3.3, is now the minimum
supported version of Python 3.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Thu, 13 Sep 2018 18:13:35 +0000 (12:13 -0600)]
Add support for 'django-dbbackup'
'parsemail' and 'parsearchive' are slow. When messing with models and
migrations, it can be very useful to backup the database in its current
state and restore it if/when you mess up. Currently, we've documented
how to do this via some commands run in the shell of the container, but
we can do things easier via an application designed for this purpose:
'django-dbshell'.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Thu, 13 Sep 2018 22:08:31 +0000 (16:08 -0600)]
Add support for django-filter 2.0
This is necessary for Django 2.1 support. We retain support for
django-filter 1.0 and 1.1 as 2.0 is Python 3-only. Thankfully there is
essentially zero cost in doing so for now.
Signed-off-by: Stephen Finucane <stephen@that.guru>
We had registered an event handler on a checkbox in table header which
would call a function, 'checkboxes', on all checkboxes within that
table. This function, in turn, causes does its work and then triggers
event handlers for all modified checkboxes which include the original
table header checkbox. This resulted in the original event calling
itself recursively.
Resolve this by only modifying the checkboxes in the table body.
Signed-off-by: Stephen Finucane <stephen@that.guru> Fixes: 44fe7bae ("js: Allow shift-select of checkboxes")
This makes things a little easier to parse. A couple of templates are
renamed and the 'register.mail' template, which appears to be unused
since commit f1e089f7, is removed.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Fri, 21 Sep 2018 16:59:48 +0000 (17:59 +0100)]
settings: Don't configure logging for parsearchive
A recent change added additional ERROR level logs to the 'parsearchive'
tool. This highlighted an issue, whereby we had configured all modules
in 'patchwork.management.command' to email administrators on ERROR logs.
We clearly shouldn't be doing this for the 'parsearchive' command or for
anything other than 'parsemail', so fix this.
Along the way, we remove a now-unnecessary 'extra' argument to one of
the logging calls in 'parsearchive' and resolve a pep8 issue.
Signed-off-by: Stephen Finucane <stephen@that.guru> Fixes: 133091da ("parsearchive: Fix logging") Cc: Daniel Axtens <dja@axtens.net>
Stephen Finucane [Wed, 29 Aug 2018 10:01:41 +0000 (11:01 +0100)]
requirements: Start using fixed versions
Given that 'tox' doesn't actually read any of these, there's no reason
to use ranges of requirements. Instead, use the latest and greatest for
live instances and rely on tox to validate behavior with older versions.
The selenium dependency, which is no longer required since commit bab2895f, is removed. The psycopg2 dependency is updated to use
psycopg2-binary, as this avoids the need for the libpg library and
removes a deprecation warning.
Signed-off-by: Stephen Finucane <stephen@that.guru> Signed-off-by: Daniel Axtens <dja@axtens.net>
Stephen Finucane [Wed, 19 Sep 2018 20:03:33 +0000 (21:03 +0100)]
parser: Handle IntegrityError for cover letters, comments
This was already done for patches but cover letters and comments were
not handled correctly, resulting in errors while parsing archives. While
we're here, we slightly modify how these exceptions are handle. Rather
than simply ignoring them, as we were doing, we raise a custom
exception. This allows us to specifically identify these types of
exceptions, print a log and still skip them (which we want, as seen in
commit d2eb1f6d2).
While we're here, we change from separate create-save calls to a
combined create-save call for both created CoverLetter and Comment
objects. We were already doing this for patches.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Wed, 19 Sep 2018 18:50:55 +0000 (19:50 +0100)]
parsearchive: Fix logging
We should use a counter normally, avoid using the counter and emit logs
when more detailed output is requested, and emit nothing when no output
is requested.
In addition, the default logging level for the parser module is set to
'WARNING' to make it less chatty.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stephen Finucane [Mon, 17 Sep 2018 17:14:05 +0000 (18:14 +0100)]
docs: Note new requirement to include a SPDX line
Add some wording around the requirement to include this line instead of
the license header. Also note the requirement that all code be licensed
using the 'GPL-2.0-or-later' license and add a CONTRIBUTING document,
which GitHub likes.
Signed-off-by: Stephen Finucane <stephen@that.guru> Reviewed-by: Veronika Kabatova <vkabatov@redhat.com>
Stephen Finucane [Mon, 17 Sep 2018 10:59:56 +0000 (11:59 +0100)]
Update license header
The FSF has a new address since 2005 that hasn't been noted in any file
except the COPYING file. Rather than fix these, simply remove the
headers in favour of a SPDX license header. IANAL but the combination of
the header and the COPYING file in source should resolve this issue.
Note that copyright notices are retained.
Signed-off-by: Stephen Finucane <stephen@that.guru> Closes: #210 Reviewed-by: Veronika Kabatova <vkabatov@redhat.com>
Stephen Finucane [Tue, 18 Sep 2018 15:48:01 +0000 (16:48 +0100)]
tox: Specify doctree directory
Sphinx 1.8 has a change in where it places 'doctree' directories. Rather
than ignore this directory via gitignore, specify where this directory
should go. This will ensure future changes in Sphinx's behavior won't
affect us.
Signed-off-by: Stephen Finucane <stephen@that.guru>
This is only used in a single view (where it probably shouldn't be used)
and some tests. It's an anti-pattern that makes it too easy to shoot
yourself in the foot. Remove it.
Signed-off-by: Stephen Finucane <stephen@that.guru>
Stewart Smith [Fri, 10 Aug 2018 08:01:06 +0000 (18:01 +1000)]
Fetch maintainer information in one query
Viewing the /project/ page lists maintainers. Prior to this patch,
this was done in one query to fetch the maintainer IDs, and then one
query per mainatiner to get the name/email address.
Now, with this patch, it's all in one query (yay joins) and saves
a few ms of database queries for displaying the page.
Realistically, this doesn't save us too much time as counting how many
patches are there takes 99% of the database time for this page.
Signed-off-by: Stewart Smith <stewart@linux.ibm.com> Reviewed-by: Stephen Finucane <stephen@that.guru>
Stewart Smith [Fri, 10 Aug 2018 08:01:05 +0000 (18:01 +1000)]
Be sensible computing project patch counts
Django actively fights constructing a query that isn't insane.
So, let's go and just execute a raw one. This is all very standard
SQL so should execute everywhere without a problem.
With the dataset of patchwork.ozlabs.org, looking at the /project/
page for qemu-devel would take 13 queries and 1500ms,
with this patch it's down to 11 queries in ~250ms.
For the dataset of the netdev list, it's down to 440ms from 1500ms.
Signed-off-by: Stewart Smith <stewart@linux.ibm.com>
[stephenfin: Handle projects that have no archived patches and those
with all patches archived] Signed-off-by: Stephen Finucane <stephen@that.guru>
Stewart Smith [Fri, 10 Aug 2018 08:01:04 +0000 (18:01 +1000)]
Optimise fetching checks when displaying a patch
Prior to this patch, a typical /patch// query for linuxppc-dev
(which has about half a dozen checks per patch) took around 20 queries
and 16.5ms in the database. About half of those queries were fetching
the checks and who did the check.
We can just do one query to get all that needed information, so we do
that. This brings a page load down to 10 queries in 12ms.
Signed-off-by: Stewart Smith <stewart@linux.ibm.com> Reviewed-by: Stephen Finucane <stephen@that.guru>
Stewart Smith [Fri, 10 Aug 2018 08:01:03 +0000 (18:01 +1000)]
Add covering index to patchwork_submissions for /list/ queries
This gets PostgreSQL to generate *much* better query plans, gaining us
about two orders of magnitude in performance on the /list/ query for the
worst state project on the patchwork.ozlabs.org instance (qemu-devel).
Signed-off-by: Stewart Smith <stewart@linux.ibm.com>
[stephenfin: Regenerate migrations per addition of 0027 in earlier
patch] Signed-off-by: Stephen Finucane <stephen@that.guru>
Stewart Smith [Fri, 10 Aug 2018 08:01:01 +0000 (18:01 +1000)]
check distinct(user) based on just user_id
The logic to display the Check(s) on the patch list wants to really do a
DISTINCT(user_id,context) ORDER BY DATE query, but with Django that is
currently a bit Too Hard (at least for me).
But what we can do is from python just use the user_id rather than the
user object itself. Same functionality, no join or prefetching users.
This saves a couple of hundred queries on the linuxppc/list/ view and
makes loading it about 4x faster in terms of time spent in the db.
Signed-off-by: Stewart Smith <stewart@linux.ibm.com> Reviewed-by: Stephen Finucane <stephen@that.guru>
Stewart Smith [Fri, 10 Aug 2018 08:00:59 +0000 (18:00 +1000)]
Add covering index for /list/ query
In constructing the list of patches for a project, there are two
main queries that are executed:
1) get a count() of how many patches there are
2) Get the page of results being displayed
In a test dataset of ~11500 LKML patches and ~4000 others, the existing
code would take around 585ms and 858ms with a cold cache and 28ms and
198ms for a warm cache.
By adding a covering index, we get down to 4ms and 255ms for a cold
cache, and 4ms and 143ms for a warm cache!
Additionally, when there's a lot of archived or accepted patches
(I used ~11000 archived out of the 15000 total in my test set)
the query time goes from 28ms and 72ms down to 2ms and 33-40ms!
Signed-off-by: Stewart Smith <stewart@linux.ibm.com>
[stephenfin: Regenerate migrations per addition of 0027 in earlier
patch] Signed-off-by: Stephen Finucane <stephen@that.guru>
Stewart Smith [Fri, 10 Aug 2018 08:00:59 +0000 (18:00 +1000)]
Fetch all series for patch/cover viewing
e.g. a 10 comment patch goes from 26 queries in 17-20ms down to 20
queries in 12ms.
A 67 comment cover letter goes from 14 queries in 16ms down to 8 queries
in 8ms.
So, effectively, a near 2x perf improvement.
Previously, at several points we were asking for the latest series and
then asking for all the series. Since there just usually aren't *that*
many series, fetch them all and take the first one if we need to.
Signed-off-by: Stewart Smith <stewart@linux.ibm.com>
[stephenfin: Fix typos in the template and ensure patches from all
series are shown] Signed-off-by: Stephen Finucane <stephen@that.guru>
Stewart Smith [Fri, 10 Aug 2018 08:00:58 +0000 (18:00 +1000)]
Add index for patchwork_comment (submission_id,date)
This (at least theoretically) should speed up displaying comments
on patches/cover letters. It's an index that will return rows
in-order for the query that we always do ("give me the comments
on this submission in date order"), rather than having to have
the database server do a sort for us.
I haven't been able to benchmark something locally that shows
this is an actual improvement, but I don't have as large data
set as various production instances. The query plan does look
a bit nicer though. Although the benefit of index maintenance
versus how long it takes to sort things is a good question.
Signed-off-by: Stewart Smith <stewart@linux.ibm.com>
[stephenfin: Regenerate migrations per addition of 0027 in earlier
patch] Signed-off-by: Stephen Finucane <stephen@that.guru>
projects / thirdparty / patchwork.git / log
