Don't treat permission name as a machine readable field, use slug instead #115

Merged
Oleg-Komarov merged 2 commits from permissions-use-slug into main 2024-05-06 17:40:07 +02:00
3 changed files with 32 additions and 27 deletions
Showing only changes of commit 0e1ab3a921 - Show all commits

View File

@ -202,8 +202,13 @@ class TagAdmin(admin.ModelAdmin):
return () return ()
class VersionPermissionAdmin(admin.ModelAdmin):
list_display = ('name', 'slug')
admin.site.register(models.Extension, ExtensionAdmin) admin.site.register(models.Extension, ExtensionAdmin)
admin.site.register(models.Version, VersionAdmin) admin.site.register(models.Version, VersionAdmin)
admin.site.register(models.Maintainer, MaintainerAdmin) admin.site.register(models.Maintainer, MaintainerAdmin)
admin.site.register(models.License, LicenseAdmin) admin.site.register(models.License, LicenseAdmin)
admin.site.register(models.Tag, TagAdmin) admin.site.register(models.Tag, TagAdmin)
admin.site.register(models.VersionPermission, VersionPermissionAdmin)

View File

@ -0,0 +1,23 @@
# Generated by Django 4.2.11 on 2024-05-06 12:10
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('extensions', '0027_unique_preview_files'),
]
operations = [
migrations.AlterField(
model_name='license',
name='slug',
field=models.SlugField(help_text='Should be taken from https://spdx.org/licenses/', unique=True),
),
migrations.AlterField(
model_name='versionpermission',
name='slug',
field=models.SlugField(help_text='Permissions add-ons are expected to need.', unique=True),
),
]

View File

@ -19,8 +19,6 @@ from constants.base import (
EXTENSION_TYPE_SLUGS, EXTENSION_TYPE_SLUGS,
FILE_STATUS_CHOICES, FILE_STATUS_CHOICES,
) )
from constants.licenses import ALL_LICENSES
from constants.version_permissions import ALL_VERSION_PERMISSIONS
import common.help_texts import common.help_texts
import extensions.fields import extensions.fields
@ -90,22 +88,13 @@ class License(CreatedModifiedMixin, models.Model):
blank=False, blank=False,
null=False, null=False,
help_text='Should be taken from https://spdx.org/licenses/', help_text='Should be taken from https://spdx.org/licenses/',
unique=True,
) )
url = models.URLField(blank=False, null=False) url = models.URLField(blank=False, null=False)
def __str__(self) -> str: def __str__(self) -> str:
return f'{self.name}' return f'{self.name}'
@classmethod
def generate(cls):
"""Generate License records from constants."""
licenses = [cls(id=li.id, name=li.name, slug=li.slug, url=li.url) for li in ALL_LICENSES]
cls.objects.bulk_create(licenses)
@classmethod
def get_by_name(cls, name: str):
return cls.objects.filter(name__startswith=name).first()
@classmethod @classmethod
def get_by_slug(cls, slug: str): def get_by_slug(cls, slug: str):
return cls.objects.filter(slug__startswith=slug).first() return cls.objects.filter(slug__startswith=slug).first()
@ -386,28 +375,16 @@ class VersionPermission(CreatedModifiedMixin, models.Model):
blank=False, blank=False,
null=False, null=False,
help_text='Permissions add-ons are expected to need.', help_text='Permissions add-ons are expected to need.',
unique=True,
) )
help = models.CharField(max_length=128, null=False, blank=False, unique=True) help = models.CharField(max_length=128, null=False, blank=False, unique=True)
def __str__(self) -> str: def __str__(self) -> str:
return f'{self.name}' return f'{self.name}'
@classmethod
def generate(cls):
"""Generate Permission records from constants."""
permissions = [
cls(id=li.id, name=li.name, slug=li.slug, help=li.help)
for li in ALL_VERSION_PERMISSIONS
]
cls.objects.bulk_create(permissions)
@classmethod
def get_by_name(cls, name: str):
return cls.objects.filter(name__startswith=name).first()
@classmethod @classmethod
def get_by_slug(cls, slug: str): def get_by_slug(cls, slug: str):
return cls.objects.filter(slug__startswith=slug).first() return cls.objects.filter(slug=slug).first()
Oleg-Komarov marked this conversation as resolved Outdated

I'm replacing this with an exact match. Was there any usecase to do a prefix match?

I'm replacing this with an exact match. Was there any usecase to do a prefix match?

I don't recall, if it wasn't explained on the corresponding commit than just assume an exact match should be fine. I think I copied this over from the license and other code snippets

I don't recall, if it wasn't explained on the corresponding commit than just assume an exact match should be fine. I think I copied this over from the license and other code snippets
class Tag(CreatedModifiedMixin, models.Model): class Tag(CreatedModifiedMixin, models.Model):
@ -537,7 +514,7 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Model
return return
for permission_name in _permissions: for permission_name in _permissions:
permission = VersionPermission.get_by_name(permission_name) permission = VersionPermission.get_by_slug(permission_name)
# Just ignore versions that are incompatible. # Just ignore versions that are incompatible.
if not permission: if not permission:
Oleg-Komarov marked this conversation as resolved Outdated

@dfelinto here we explicitly allow to submit a manifest with incorrect permissions names. is it intentional? shouldn't we return a validation error instead if somebody mistypes a permission slug?

@dfelinto here we explicitly allow to submit a manifest with incorrect permissions names. is it intentional? shouldn't we return a validation error instead if somebody mistypes a permission slug?

Right, it should be an error indeed. Maybe already is (caught early on by the validator)?

Right, it should be an error indeed. Maybe already is (caught early on by the validator)?