Re: Introduce contributor badges - v14

The v14 patch series addresses Dave's review, Daniels 150x150 comment
and includes Magnus feedback:

Re: Dave Page
> 1. **Removing a holder purges nothing for that person.** `BadgeForm.save`
> calls `badge.holders.remove()` (`pgweb/contributors/forms.py`). That
> deletes the through-table rows without any `post_save`, and pgweb only
> hooks `post_save` (`pgweb/util/signals.py:169`). The badge page still gets
> purged, because `simple_form` saves the badge afterwards. The removed
> person's `/community/people/<username>/` page keeps showing the badge.

This is v14-0007-Purge-badge-pages-and-holder-profiles-through-xk.patch

> 2. **Deletes never purge.** This applies to deleting a `Badge`,
> `Badgeholder` or `Contributor` in the admin. The rest of pgweb has the same
> gap, but it matters more here: a deleted contributor's public profile page,
> which can include their email, stays cached. If someone asks for their
> profile to be removed, it isn't really gone.

v14-0008-Purge-on-delete-as-well-as-on-save.patch

> 3. **Approving or editing a badge doesn't purge its holders' profile
> pages.** `Badge.purge_urls` says profiles "get refreshed when their holder
> rows are edited", but approval, unapproval, a title change and an image
> added or removed all leave the profiles stale. After unapproval, profiles
> keep linking to `/community/badge/<slug>/image/`, which now returns a 404
> that Varnish may also cache. The fix is xkeys, which the docs already
> describe as the better option: tag the badge, people and profile responses
> with something like `pgbadge_<id>` and give `Badge` a `purge_xkeys`. That
> also replaces the per-holder URL loops.

Part of v14-0007-Purge-badge-pages-and-holder-profiles-through-xk.patch

> 4. **The image URL never changes when the image does.**
> `/community/badge/<slug>/image/` has no file extension and no version in
> it. The docs say Varnish applies a longer policy to images "because they
> are considered never to change". Varnish itself is purged, but if the
> Varnish configuration (VCL) also sends long browser cache headers for
> images, browsers will keep showing the old image. The VCL isn't in this
> repo, so I can't check which behaviour applies. Adding a short hash of the
> image to the URL, as `?v=` or in the path, avoids the question.

We rejected that in the other subthread.

> ## Database and app server
>
> 5. **The `has_image` annotation reads every image out of TOAST**,
> PostgreSQL's out-of-line storage for large values
> (`BadgeManager.with_has_image`). The comment says the value is "never
> transferred", which is true for the network, but `imagedata > ''` compares
> the full bytea, so PostgreSQL has to fetch and reassemble the whole value.
> A test of 200 rows of about 96 KB each on the local database showed the
> comparison reading about 3,000 buffers in 31 ms, whilst `octet_length(img)
> > 0` read 2 buffers in 0.04 ms, because it only reads the TOAST header.
> Switching to `Length('imagedata') > 0` is a one-line fix. It runs on every
> cache miss of the people page (up to 200 badges) and on every account
> profile page load, which is never cached.

v14-0002-Determine-has-image-without-full-detoasting.patch

> 6. **Oversized images are decoded in full (a "decompression bomb").**
> `ImageBinaryField.to_python` decodes the entire image with Pillow before
> rescaling. A PNG under 1 MB can still be about 170 megapixels before Pillow
> refuses it, which is several hundred MB of RAM plus a bicubic resize on the
> app server. `ModelForm` full-clean also re-parses the stored image on every
> badge edit, even when the image hasn't changed. Only managers of an
> approved organisation can upload, so the risk is moderate, but checking
> `img.size` before decoding would be cheap.

v14-0003-Reject-images-with-wrong-size-before-loading-the.patch
v14-0004-Don-t-re-parse-the-stored-badge-image-on-every-e.patch

> 7. **Minor, and only on cache misses:**
> - `badge.html` uses `holders|length` and then `holders.all`, which runs
> the holder query and its two prefetches a second time.

v14-0005-Don-t-run-the-badge-holder-query-twice.patch

> - `select_related` would do better than `prefetch_related` for the
> foreign-key lookups in `peoplelist` and `badge_view`.

v14-0006-Use-select_related-for-the-badge-holder-and-peop.patch

> - `badge_image` fetches the image and computes its MD5 before it checks
> `If-None-Match`; storing the hash would avoid both.

v14-0009-Store-the-badge-image-md5-in-the-database.patch

Since 0009 is touching the database, we can now pull up the staged
extended badge title length:
v14-0010-Extend-badge-title-to-255-chars.patch

Both together share a migration file that is part of
v14-0011-Migrate-imagehash-and-serve-badge-images-from-it.patch

If we decide we don't want the MD5 change, we can postpone these
three.

> - Every account profile save that has a contributor record purges
> `/community/contributors/`, `/community/people/` and all of that user's
> badge pages, even when nothing changed.

v14-0012-Don-t-re-purge-contributor-pages-on-a-no-op-acco.patch

Re: Daniel Gustafsson
> +1 on restricting the upload to 150x150 and make the user responsible for the
> badge image looking good rather than rescaling.

That's v14-0001-Require-user-supplied-badges-to-be-150x150.patch

v14-0010-Extend-badge-title-to-255-chars.patch

Christoph