From 65da818c59622934e667fd43808b992c6a1e9a27 Mon Sep 17 00:00:00 2001 From: alejandromumo Date: Wed, 6 Mar 2024 12:50:45 +0100 Subject: [PATCH 1/2] records: add status field to community records * added "status" field to community records * renamed "is_verified" property to "is_safelisted" * updated mappings to support "is_safelisted" * added tests for the new field * added tests for safelisting feature --- .../communities/records/api.py | 10 +++--- .../communities/communities-v1.0.0.json | 8 +++++ .../os-v1/communities/communities-v1.0.0.json | 5 ++- .../os-v2/communities/communities-v1.0.0.json | 5 ++- .../v7/communities/communities-v1.0.0.json | 5 ++- .../communities/records/models.py | 30 +++++++++++++++- .../{is_verified.py => is_safelisted.py} | 23 +++++++----- invenio_communities/communities/schema.py | 9 +++-- .../communities/services/sort.py | 2 +- invenio_communities/permissions.py | 2 +- tests/communities/test_safelist.py | 35 +++++++++++++++++++ tests/communities/test_services.py | 24 +++++++++++++ tests/conftest.py | 16 ++++++++- 13 files changed, 151 insertions(+), 23 deletions(-) rename invenio_communities/communities/records/systemfields/{is_verified.py => is_safelisted.py} (52%) create mode 100644 tests/communities/test_safelist.py diff --git a/invenio_communities/communities/records/api.py b/invenio_communities/communities/records/api.py index 0b5d35521..05901fd18 100644 --- a/invenio_communities/communities/records/api.py +++ b/invenio_communities/communities/records/api.py @@ -27,8 +27,8 @@ from invenio_vocabularies.records.systemfields.relations import CustomFieldsRelation from invenio_communities.communities.records.systemfields.children import ChildrenField -from invenio_communities.communities.records.systemfields.is_verified import ( - IsVerifiedField, +from invenio_communities.communities.records.systemfields.is_safelisted import ( + IsSafelistedField, ) from ..dumpers.featured import FeaturedDumperExt @@ -64,7 +64,7 @@ class Community(Record): extensions=[ FeaturedDumperExt("featured"), RelationDumperExt("relations"), - CalculatedFieldDumperExt("is_verified"), + CalculatedFieldDumperExt("is_safelisted"), ] ) @@ -121,7 +121,9 @@ class Community(Record): custom=CustomFieldsRelation("COMMUNITIES_CUSTOM_FIELDS"), ) - is_verified = IsVerifiedField("is_verified") + status = ModelField("status", dump_type=str) + + is_safelisted = IsSafelistedField("is_safelisted") deletion_status = CommunityDeletionStatusField() diff --git a/invenio_communities/communities/records/jsonschemas/communities/communities-v1.0.0.json b/invenio_communities/communities/records/jsonschemas/communities/communities-v1.0.0.json index 0dcd55d4e..0867b5107 100644 --- a/invenio_communities/communities/records/jsonschemas/communities/communities-v1.0.0.json +++ b/invenio_communities/communities/records/jsonschemas/communities/communities-v1.0.0.json @@ -220,6 +220,14 @@ "description": "Whether or not the tombstone page is publicly visible." } } + }, + "status": { + "type": "string", + "enum": [ + "new", + "moderated", + "verified" + ] } } } diff --git a/invenio_communities/communities/records/mappings/os-v1/communities/communities-v1.0.0.json b/invenio_communities/communities/records/mappings/os-v1/communities/communities-v1.0.0.json index 04dad161f..192259868 100644 --- a/invenio_communities/communities/records/mappings/os-v1/communities/communities-v1.0.0.json +++ b/invenio_communities/communities/records/mappings/os-v1/communities/communities-v1.0.0.json @@ -38,9 +38,12 @@ "id": { "type": "keyword" }, - "is_verified": { + "is_safelisted": { "type": "boolean" }, + "status": { + "type": "keyword" + }, "slug": { "type": "keyword" }, diff --git a/invenio_communities/communities/records/mappings/os-v2/communities/communities-v1.0.0.json b/invenio_communities/communities/records/mappings/os-v2/communities/communities-v1.0.0.json index babe67e2a..655a85148 100644 --- a/invenio_communities/communities/records/mappings/os-v2/communities/communities-v1.0.0.json +++ b/invenio_communities/communities/records/mappings/os-v2/communities/communities-v1.0.0.json @@ -38,9 +38,12 @@ "id": { "type": "keyword" }, - "is_verified": { + "is_safelisted": { "type": "boolean" }, + "status": { + "type": "keyword" + }, "slug": { "type": "keyword" }, diff --git a/invenio_communities/communities/records/mappings/v7/communities/communities-v1.0.0.json b/invenio_communities/communities/records/mappings/v7/communities/communities-v1.0.0.json index 5c31afa89..294867241 100644 --- a/invenio_communities/communities/records/mappings/v7/communities/communities-v1.0.0.json +++ b/invenio_communities/communities/records/mappings/v7/communities/communities-v1.0.0.json @@ -38,7 +38,7 @@ "id": { "type": "keyword" }, - "is_verified": { + "is_safelisted": { "type": "boolean" }, "slug": { @@ -47,6 +47,9 @@ "deletion_status": { "type": "keyword" }, + "status": { + "type": "keyword" + }, "is_deleted": { "type": "boolean" }, diff --git a/invenio_communities/communities/records/models.py b/invenio_communities/communities/records/models.py index f4ad14d99..00fb03b75 100644 --- a/invenio_communities/communities/records/models.py +++ b/invenio_communities/communities/records/models.py @@ -8,7 +8,7 @@ # under the terms of the MIT License; see LICENSE file for more details. """Community database models.""" - +import enum from datetime import datetime from invenio_db import db @@ -22,6 +22,28 @@ from .systemfields.deletion_status import CommunityDeletionStatusEnum +class CommunityStatusEnum(enum.Enum): + """Community status enum.""" + + NEW = "N" + VERIFIED = "V" + MODERATED = "M" + + def __str__(self): + """Return its value.""" + return self.value + + def __eq__(self, __value) -> bool: + """Check if the value is equal to the enum value. + + Supports comparison with string values. + """ + if isinstance(__value, str): + return self.value == __value + + return super().__eq__(__value) + + class CommunityMetadata(db.Model, RecordMetadataBase): """Represent a community.""" @@ -39,6 +61,12 @@ class CommunityMetadata(db.Model, RecordMetadataBase): default=CommunityDeletionStatusEnum.PUBLISHED.value, ) + status = db.Column( + ChoiceType(CommunityStatusEnum, impl=db.String(1)), + nullable=False, + default=CommunityStatusEnum.NEW.value, + ) + class CommunityFileMetadata(db.Model, RecordMetadataBase, FileRecordModelMixin): """File associated with a community.""" diff --git a/invenio_communities/communities/records/systemfields/is_verified.py b/invenio_communities/communities/records/systemfields/is_safelisted.py similarity index 52% rename from invenio_communities/communities/records/systemfields/is_verified.py rename to invenio_communities/communities/records/systemfields/is_safelisted.py index 2c2043347..dca121748 100644 --- a/invenio_communities/communities/records/systemfields/is_verified.py +++ b/invenio_communities/communities/records/systemfields/is_safelisted.py @@ -8,8 +8,10 @@ from invenio_records_resources.records.systemfields.calculated import CalculatedField +from ..models import CommunityStatusEnum -class IsVerifiedField(CalculatedField): + +class IsSafelistedField(CalculatedField): """Systemfield for calculating whether or not the request is expired.""" def __init__(self, key=None): @@ -17,15 +19,18 @@ def __init__(self, key=None): super().__init__(key=key, use_cache=False) def calculate(self, record): - """Calculate the ``is_verified`` property of the record.""" + """Calculate the ``is_safelisted`` property of the record.""" # import here due to circular import from invenio_communities.members.records.api import Member - community_verified = False - owners = [m.dumps() for m in Member.get_members(record.id) if m.role == "owner"] - for owner in owners: - # community is considered verified if at least one owner is verified - if owner["user"]["verified_at"] is not None: - community_verified = True - break + community_verified = record.status == CommunityStatusEnum.VERIFIED + if not community_verified: + owners = [ + m.dumps() for m in Member.get_members(record.id) if m.role == "owner" + ] + for owner in owners: + # community is considered verified if at least one owner is verified + if owner["user"]["verified_at"] is not None: + community_verified = True + break return community_verified diff --git a/invenio_communities/communities/schema.py b/invenio_communities/communities/schema.py index a29b39b85..21165adff 100644 --- a/invenio_communities/communities/schema.py +++ b/invenio_communities/communities/schema.py @@ -205,8 +205,9 @@ class Meta: unknown = EXCLUDE field_dump_permissions = { - # hide 'is_verified' behind a permission - "is_verified": "moderate", + # hide 'is_safelisted' behind a permission + "is_safelisted": "moderate", + "status": "moderate", } id = fields.String(dump_only=True) @@ -229,7 +230,9 @@ class Meta: partial(CustomFieldsSchema, fields_var="COMMUNITIES_CUSTOM_FIELDS") ) - is_verified = fields.Boolean(dump_only=True) + is_safelisted = fields.Boolean(dump_only=True) + + status = fields.String(dump_only=True) theme = fields.Nested(CommunityThemeSchema, allow_none=True) diff --git a/invenio_communities/communities/services/sort.py b/invenio_communities/communities/services/sort.py index d936d2c48..a5a8e00a9 100644 --- a/invenio_communities/communities/services/sort.py +++ b/invenio_communities/communities/services/sort.py @@ -30,6 +30,6 @@ def apply(self, identity, search, params): if current_app.config["COMMUNITIES_SEARCH_SORT_BY_VERIFIED"]: fields = self._compute_sort_fields(params) - return search.sort(*["-is_verified", *fields]) + return search.sort(*["-is_safelisted", *fields]) return super(CommunitiesSortParam, self).apply(identity, search, params) diff --git a/invenio_communities/permissions.py b/invenio_communities/permissions.py index 71e6cc258..d9144a3f1 100644 --- a/invenio_communities/permissions.py +++ b/invenio_communities/permissions.py @@ -168,7 +168,7 @@ class CommunityPermissionPolicy(BasePermissionPolicy): can_featured_update = [Administration(), SystemProcess()] can_featured_delete = [Administration(), SystemProcess()] - # Used to hide at the moment the `is_verified` field. It should be set to + # Used to hide at the moment the `is_safelisted` field. It should be set to # correct permissions based on which the field will be exposed only to moderators can_moderate = [Disable()] diff --git a/tests/communities/test_safelist.py b/tests/communities/test_safelist.py new file mode 100644 index 000000000..c24e1d6ea --- /dev/null +++ b/tests/communities/test_safelist.py @@ -0,0 +1,35 @@ +# -*- coding: utf-8 -*- +# +# Copyright (C) 2024 CERN. +# +# Invenio-communities is free software; you can redistribute it and/or modify +# it under the terms of the MIT License; see LICENSE file for more details. +"""Test safelist feature for communities.""" + +from copy import deepcopy + +from invenio_db import db + +from invenio_communities.communities.records.models import CommunityStatusEnum + + +def test_safelist_computed_by_verified_status( + community_service, minimal_community, location, es_clear, unverified_user +): + """ + Test that the safelist feature for communities is computed correctly based on the verified status. + """ + # Create a comunity + # Flag it as "verified" + # Validate that the computed field "is_verified" is set to "True" + c_data = deepcopy(minimal_community) + c_data["slug"] = "test_status_perms" + c_item = community_service.create(unverified_user.identity, data=c_data) + assert c_item._record.status == CommunityStatusEnum.NEW + assert c_item._record.is_safelisted is False + community = community_service.record_cls.pid.resolve(c_item.id) + community.status = CommunityStatusEnum.VERIFIED + community.commit() + db.session.commit() + c_item = community_service.read(unverified_user.identity, c_item.id) + assert c_item._record.is_safelisted is True diff --git a/tests/communities/test_services.py b/tests/communities/test_services.py index 65827447b..1597687c1 100644 --- a/tests/communities/test_services.py +++ b/tests/communities/test_services.py @@ -21,6 +21,7 @@ from invenio_records_resources.services.errors import PermissionDeniedError from marshmallow import ValidationError +from invenio_communities.communities.records.models import CommunityStatusEnum from invenio_communities.communities.records.systemfields.deletion_status import ( CommunityDeletionStatusEnum, ) @@ -762,3 +763,26 @@ def test_bulk_update_parent_overwrite( for c_id in children: c_comm = community_service.record_cls.pid.resolve(c_id) assert str(c_comm.parent.id) == str(parent_community.id) + + +def test_status_new(community_service, minimal_community, location, es_clear, owner): + c_data = deepcopy(minimal_community) + c_data["slug"] = "test_status_new" + co = community_service.create(data=c_data, identity=owner.identity) + assert co._record.status == CommunityStatusEnum.NEW + + +def test_status_permissions( + community_service, minimal_community, users, location, es_clear, owner +): + """Test that search does not return the 'status' field to any user.""" + c_data = deepcopy(minimal_community) + c_data["slug"] = "test_status_perms" + co = community_service.create(data=c_data, identity=owner.identity) + community_service.record_cls.index.refresh() + assert co._record.status == CommunityStatusEnum.NEW + + for uname, u in users.items(): + search = community_service.search(u.identity) + assert search.total == 1 + assert not any("status" in hit for hit in search.hits) diff --git a/tests/conftest.py b/tests/conftest.py index 33e71b64f..c82293453 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,7 +12,7 @@ from copy import deepcopy import pytest -from flask_principal import AnonymousIdentity +from flask_principal import AnonymousIdentity, Need from invenio_access.models import ActionRoles from invenio_access.permissions import any_user as any_user_need from invenio_access.permissions import superuser_access @@ -251,6 +251,20 @@ def owner(users): return users["owner"] +@pytest.fixture() +def unverified_user(UserFixture, app, db): + """User meant to test 'verified' property of records.""" + u = UserFixture( + email="unverified@inveniosoftware.org", + password="testuser", + ) + u.create(app, db) + u.user.verified_at = None + # Dumping `is_verified` requires authenticated user in tests + u.identity.provides.add(Need(method="system_role", value="authenticated_user")) + return u + + @pytest.fixture(scope="module") def any_user(UserFixture, app, database): """A user without privileges or memberships.""" From 9dac8a56a64e6dcc0cffc975138167c46c329742 Mon Sep 17 00:00:00 2001 From: alejandromumo Date: Thu, 7 Mar 2024 15:45:25 +0100 Subject: [PATCH 2/2] records: refactored 'status' as a systemfield * 'status' is not a db column anymore, instead it is a systemfield --- .../communities/records/api.py | 3 +- .../communities/records/models.py | 30 +----- .../records/systemfields/community_status.py | 99 +++++++++++++++++++ .../records/systemfields/is_safelisted.py | 2 +- tests/communities/test_safelist.py | 8 +- tests/communities/test_services.py | 5 +- 6 files changed, 111 insertions(+), 36 deletions(-) create mode 100644 invenio_communities/communities/records/systemfields/community_status.py diff --git a/invenio_communities/communities/records/api.py b/invenio_communities/communities/records/api.py index 05901fd18..647aa5ed3 100644 --- a/invenio_communities/communities/records/api.py +++ b/invenio_communities/communities/records/api.py @@ -34,6 +34,7 @@ from ..dumpers.featured import FeaturedDumperExt from . import models from .systemfields.access import CommunityAccessField +from .systemfields.community_status import CommunityStatusField from .systemfields.deletion_status import CommunityDeletionStatusField from .systemfields.parent_community import ParentCommunityField from .systemfields.pidslug import PIDSlugField @@ -121,7 +122,7 @@ class Community(Record): custom=CustomFieldsRelation("COMMUNITIES_CUSTOM_FIELDS"), ) - status = ModelField("status", dump_type=str) + status = CommunityStatusField("status") is_safelisted = IsSafelistedField("is_safelisted") diff --git a/invenio_communities/communities/records/models.py b/invenio_communities/communities/records/models.py index 00fb03b75..f4ad14d99 100644 --- a/invenio_communities/communities/records/models.py +++ b/invenio_communities/communities/records/models.py @@ -8,7 +8,7 @@ # under the terms of the MIT License; see LICENSE file for more details. """Community database models.""" -import enum + from datetime import datetime from invenio_db import db @@ -22,28 +22,6 @@ from .systemfields.deletion_status import CommunityDeletionStatusEnum -class CommunityStatusEnum(enum.Enum): - """Community status enum.""" - - NEW = "N" - VERIFIED = "V" - MODERATED = "M" - - def __str__(self): - """Return its value.""" - return self.value - - def __eq__(self, __value) -> bool: - """Check if the value is equal to the enum value. - - Supports comparison with string values. - """ - if isinstance(__value, str): - return self.value == __value - - return super().__eq__(__value) - - class CommunityMetadata(db.Model, RecordMetadataBase): """Represent a community.""" @@ -61,12 +39,6 @@ class CommunityMetadata(db.Model, RecordMetadataBase): default=CommunityDeletionStatusEnum.PUBLISHED.value, ) - status = db.Column( - ChoiceType(CommunityStatusEnum, impl=db.String(1)), - nullable=False, - default=CommunityStatusEnum.NEW.value, - ) - class CommunityFileMetadata(db.Model, RecordMetadataBase, FileRecordModelMixin): """File associated with a community.""" diff --git a/invenio_communities/communities/records/systemfields/community_status.py b/invenio_communities/communities/records/systemfields/community_status.py new file mode 100644 index 000000000..d4b990e55 --- /dev/null +++ b/invenio_communities/communities/records/systemfields/community_status.py @@ -0,0 +1,99 @@ +# -*- coding: utf-8 -*- +# +# Copyright (C) 2024 CERN. +# +# Invenio-communities is free software; you can redistribute it and/or modify +# it under the terms of the MIT License; see LICENSE file for more details. +"""Community status system field.""" + +import enum + +from invenio_records.systemfields import SystemField + + +class CommunityStatusEnum(enum.Enum): + """Community status enum.""" + + NEW = "new" + VERIFIED = "verified" + MODERATED = "moderated" + + +class CommunityStatus: + """The community status of the community.""" + + def __init__(self, status): + """Initialize the community status.""" + self.status = status + + @property + def status(self): + """Get the community status.""" + return self._status.value + + @status.setter + def status(self, value): + """Set the community status.""" + if value is None: + self._status = CommunityStatusEnum.NEW + + elif isinstance(value, str): + self._status = CommunityStatusEnum(value) + + elif isinstance(value, CommunityStatusEnum): + self._status = value + + else: + raise ValueError(f"Invalid value for community community status: {value}") + + def __repr__(self): + """Return repr(self).""" + return f"" + + def __str__(self): + """Return str(self).""" + return self.status + + def __eq__(self, other): + """Check if self and other are equal. + + This allows checking against other instances of the same type, strings, + and ``CommunityStatusEnum`` values. + """ + if isinstance(other, type(self)): + return self.status == other.status + + elif isinstance(other, CommunityStatusEnum): + return self.status == other.value + + elif isinstance(other, str): + return self.status == other + + return False + + +class CommunityStatusField(SystemField): + """System field for the community status.""" + + # + # Data descriptor methods (i.e. attribute access) + # + def __get__(self, record, owner=None): + """Get the status of the community.""" + if record is None: + return self # returns the field itself. + + status = self._get_cache(record) or CommunityStatus(record.get("status")) + + self._set_cache(record, status) + return status + + def __set__(self, record, value): + """Set the status of the community.""" + status = CommunityStatus(value) + self._set_cache(record, status) + + def pre_commit(self, record): + """Dump the deletion status to the community before committing.""" + status = self._get_cache(record) or CommunityStatus(None) + record[self.key] = status.status diff --git a/invenio_communities/communities/records/systemfields/is_safelisted.py b/invenio_communities/communities/records/systemfields/is_safelisted.py index dca121748..310e9d592 100644 --- a/invenio_communities/communities/records/systemfields/is_safelisted.py +++ b/invenio_communities/communities/records/systemfields/is_safelisted.py @@ -8,7 +8,7 @@ from invenio_records_resources.records.systemfields.calculated import CalculatedField -from ..models import CommunityStatusEnum +from ..systemfields.community_status import CommunityStatusEnum class IsSafelistedField(CalculatedField): diff --git a/tests/communities/test_safelist.py b/tests/communities/test_safelist.py index c24e1d6ea..5512b683c 100644 --- a/tests/communities/test_safelist.py +++ b/tests/communities/test_safelist.py @@ -10,15 +10,15 @@ from invenio_db import db -from invenio_communities.communities.records.models import CommunityStatusEnum +from invenio_communities.communities.records.systemfields.community_status import ( + CommunityStatusEnum, +) def test_safelist_computed_by_verified_status( community_service, minimal_community, location, es_clear, unverified_user ): - """ - Test that the safelist feature for communities is computed correctly based on the verified status. - """ + """Test that the safelist feature for communities is computed correctly based on the verified status.""" # Create a comunity # Flag it as "verified" # Validate that the computed field "is_verified" is set to "True" diff --git a/tests/communities/test_services.py b/tests/communities/test_services.py index 1597687c1..60a55681a 100644 --- a/tests/communities/test_services.py +++ b/tests/communities/test_services.py @@ -21,7 +21,9 @@ from invenio_records_resources.services.errors import PermissionDeniedError from marshmallow import ValidationError -from invenio_communities.communities.records.models import CommunityStatusEnum +from invenio_communities.communities.records.systemfields.community_status import ( + CommunityStatusEnum, +) from invenio_communities.communities.records.systemfields.deletion_status import ( CommunityDeletionStatusEnum, ) @@ -766,6 +768,7 @@ def test_bulk_update_parent_overwrite( def test_status_new(community_service, minimal_community, location, es_clear, owner): + """Test the status of a new community.""" c_data = deepcopy(minimal_community) c_data["slug"] = "test_status_new" co = community_service.create(data=c_data, identity=owner.identity)