From b5814975e81701373543647c7be08851c1458c3f Mon Sep 17 00:00:00 2001 From: Mirek Simek Date: Sun, 11 Aug 2024 20:48:32 +0200 Subject: [PATCH 1/3] Adding IfRequestedBy, making requests compatible with workflow rename (workflow_id => workflow) --- .gitignore | 2 + oarepo_requests/errors.py | 11 +++ oarepo_requests/receiver.py | 19 +++-- oarepo_requests/services/oarepo/service.py | 3 +- .../services/permissions/generators.py | 77 +++++++++++-------- oarepo_requests/utils.py | 10 ++- run-tests.sh | 10 ++- setup.cfg | 2 +- tests/conftest.py | 59 +++++++++++++- .../test_conditional_recipient.py | 59 ++++++++++++++ tests/thesis_workflows.yaml | 32 +------- 11 files changed, 208 insertions(+), 76 deletions(-) create mode 100644 tests/test_requests/test_conditional_recipient.py diff --git a/.gitignore b/.gitignore index 5eb451ad..2748eca6 100644 --- a/.gitignore +++ b/.gitignore @@ -96,3 +96,5 @@ node_modules dummy-record.js jsconfig.json + +forked_install.sh \ No newline at end of file diff --git a/oarepo_requests/errors.py b/oarepo_requests/errors.py index 11fe4697..1d35062b 100644 --- a/oarepo_requests/errors.py +++ b/oarepo_requests/errors.py @@ -19,3 +19,14 @@ def __init__(self, request_type): def description(self): """Exception's description.""" return f"Unknown request type {self.request_type}." + + +class RequestTypeNotInWorkflow(Exception): + def __init__(self, request_type, workflow): + self.request_type = request_type + self.workflow = workflow + + @property + def description(self): + """Exception's description.""" + return f"Request type {self.request_type} not in workflow {self.workflow}." \ No newline at end of file diff --git a/oarepo_requests/receiver.py b/oarepo_requests/receiver.py index a89bc08d..04299245 100644 --- a/oarepo_requests/receiver.py +++ b/oarepo_requests/receiver.py @@ -1,16 +1,23 @@ +from oarepo_requests.errors import RequestTypeNotInWorkflow + + def default_workflow_receiver_function(record=None, request_type=None, **kwargs): from oarepo_workflows.proxies import current_oarepo_workflows workflow_id = current_oarepo_workflows.get_workflow_from_record(record) + if not workflow_id: + return None + try: request = getattr( current_oarepo_workflows.record_workflows[workflow_id].requests(), request_type.type_id, ) - receiver = request.reference_receivers( - record=record, request_type=request_type, **kwargs - ) - return receiver - except (KeyError, AttributeError): - return None + except AttributeError: + raise RequestTypeNotInWorkflow(request_type.type_id, workflow_id) + + receiver = request.reference_receivers( + record=record, request_type=request_type, **kwargs + ) + return receiver diff --git a/oarepo_requests/services/oarepo/service.py b/oarepo_requests/services/oarepo/service.py index 23b2b4af..566aec86 100644 --- a/oarepo_requests/services/oarepo/service.py +++ b/oarepo_requests/services/oarepo/service.py @@ -28,8 +28,9 @@ def create( raise UnknownRequestType(request_type) if receiver is None: + # if explicit creator is not passed, use current identity - this is in sync with invenio_requests receiver = current_oarepo_requests.default_request_receiver( - identity, type_, topic, creator, data + identity, type_, topic, creator or identity, data ) if data is None: diff --git a/oarepo_requests/services/permissions/generators.py b/oarepo_requests/services/permissions/generators.py index f0385b22..39c32a56 100644 --- a/oarepo_requests/services/permissions/generators.py +++ b/oarepo_requests/services/permissions/generators.py @@ -1,7 +1,12 @@ -from invenio_records_permissions.generators import Generator +from invenio_records_permissions.generators import Generator, ConditionalGenerator from invenio_search.engine import dsl from oarepo_requests.services.permissions.identity import request_active +from oarepo_workflows.requests.policy import RecipientGeneratorMixin +from flask_principal import Identity + +from invenio_requests.proxies import current_requests +from invenio_records_resources.references.entity_resolvers import EntityProxy class RequestActive(Generator): @@ -57,33 +62,43 @@ def query_filter(self, record=None, request_type=None, **kwargs): pass -""" -#if needed, have to implement filter -class RecordRequestsReceivers(Generator): - def needs(self, record=None, **kwargs): - service = get_requests_service_for_records_service( - get_record_service_for_record(record) - ) - reader = ( - service.search_requests_for_draft - if getattr(record, "is_draft", False) - else service.search_requests_for_record - ) - requests = list(reader(system_identity, record["id"]).hits) - needs = set() - for request in requests: - request_type_id = request["type"] - type_ = current_request_type_registry.lookup(request_type_id, quiet=True) - if not type_: - raise UnknownRequestType(request_type_id) - workflow_request = current_oarepo_workflows.get_workflow(record).requests()[ - request_type_id - ] - request_needs = { - need - for generator in workflow_request.recipients - for need in generator.needs(**kwargs) - } - needs |= request_needs - return needs -""" +class IfRequestedBy(RecipientGeneratorMixin, ConditionalGenerator): + + def __init__(self, conditions, then_, else_): + super().__init__(then_, else_) + if not isinstance(conditions, (list, tuple)): + conditions = [conditions] + self._conditions = conditions + + def _condition(self, *, request_type, creator, **kwargs): + """Condition to choose generators set.""" + # get needs + if isinstance(creator, Identity): + needs = creator.provides + else: + if not isinstance(creator, EntityProxy): + # convert to entityproxy + creator = current_requests.entity_resolvers_registry.reference_entity(creator) + needs = creator.get_needs() + + for condition in self._conditions: + condition_needs = set(condition.needs(request_type=request_type, creator=creator, **kwargs)) + condition_excludes = set(condition.excludes(request_type=request_type, creator=creator, **kwargs)) + + if not condition_needs.intersection(needs): + continue + if condition_excludes and condition_excludes.intersection(needs): + continue + return True + return False + + def reference_receivers(self, record=None, request_type=None, **kwargs): + ret = [] + for gen in self._generators(record=record, request_type=request_type, **kwargs): + if isinstance(gen, RecipientGeneratorMixin): + ret.extend(gen.reference_receivers(record=record, request_type=request_type, **kwargs)) + return ret + + def query_filter(self, **kwargs): + """Search filters.""" + raise NotImplementedError("Please use IfRequestedBy only in recipients, not elsewhere.") diff --git a/oarepo_requests/utils.py b/oarepo_requests/utils.py index 2452d516..57a4f352 100644 --- a/oarepo_requests/utils.py +++ b/oarepo_requests/utils.py @@ -8,8 +8,14 @@ def allowed_request_types_for_record(record): try: from oarepo_workflows.proxies import current_oarepo_workflows - - workflow_requests = current_oarepo_workflows.get_workflow(record).requests() + from oarepo_workflows.errors import MissingWorkflowError + try: + workflow_requests = current_oarepo_workflows.get_workflow(record).requests() + except MissingWorkflowError: + # workflow not defined on the record, probably not a workflow-enabled record + # so returning all matching request types + # TODO: is this correct? + workflow_requests = None except ImportError: workflow_requests = None request_types = current_request_type_registry._registered_types diff --git a/run-tests.sh b/run-tests.sh index 0bf429d0..4e4585a1 100755 --- a/run-tests.sh +++ b/run-tests.sh @@ -9,6 +9,8 @@ BUILDER_VENV=.venv-builder BUILD_TEST_DIR="tests" CODE_TEST_DIR="tests" +curl -L -o forked_install.sh https://github.com/oarepo/nrp-devtools/raw/main/tests/forked_install.sh + if test -d $BUILDER_VENV ; then rm -rf $BUILDER_VENV fi @@ -16,8 +18,12 @@ fi python3 -m venv $BUILDER_VENV . $BUILDER_VENV/bin/activate pip install -U setuptools pip wheel -pip install -U oarepo-model-builder-tests oarepo-model-builder-requests oarepo-model-builder-drafts -curl -L -o forked_install.sh https://github.com/oarepo/nrp-devtools/raw/main/tests/forked_install.sh +pip install -U oarepo-model-builder \ + oarepo-model-builder-tests \ + oarepo-model-builder-requests \ + oarepo-model-builder-drafts \ + oarepo-model-builder-workflows + if test -d ./$BUILD_TEST_DIR/$MODEL; then rm -rf ./$BUILD_TEST_DIR/$MODEL fi diff --git a/setup.cfg b/setup.cfg index 8d45e6e7..097769c5 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,6 +1,6 @@ [metadata] name = oarepo-requests -version = 2.0.1 +version = 2.0.2 description = authors = Ronald Krist readme = README.md diff --git a/tests/conftest.py b/tests/conftest.py index acd11ed8..d0a1a1d3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -25,6 +25,8 @@ ) from oarepo_workflows.base import Workflow from oarepo_workflows.requests import RecipientGeneratorMixin + +from oarepo_requests.services.permissions.generators import IfRequestedBy from thesis.proxies import current_service from thesis.records.api import ThesisDraft @@ -70,6 +72,17 @@ class DefaultRequests(WorkflowRequestPolicy): ) +class UserGenerator(RecipientGeneratorMixin, Generator): + def __init__(self, user_id): + self.user_id = user_id + + def needs(self, **kwargs): + return [UserNeed(self.user_id)] + + def reference_receivers(self, **kwargs): + return [{"user": str(self.user_id)}] + + class RequestsWithApprove(WorkflowRequestPolicy): publish_draft = WorkflowRequest( requesters=[IfInState("approved", [AutoRequest()])], @@ -99,6 +112,17 @@ class RequestsWithApprove(WorkflowRequestPolicy): ) +class RequestsWithCT(WorkflowRequestPolicy): + conditional_recipient_rt = WorkflowRequest( + requesters=[AnyUser()], + recipients=[IfRequestedBy( + UserGenerator(1), + [UserGenerator(2)], + [UserGenerator(3)] + )], + ) + + class ApproveRequestType(NonDuplicableOARepoRequestType): type_id = "approve_draft" name = _("Approve draft") @@ -114,6 +138,21 @@ class ApproveRequestType(NonDuplicableOARepoRequestType): allowed_topic_ref_types = ModelRefTypes(published=False, draft=True) +class ConditionalRecipientRequestType(NonDuplicableOARepoRequestType): + type_id = "conditional_recipient_rt" + name = _("Request type to test conditional recipients") + + available_actions = { + **NonDuplicableOARepoRequestType.available_actions, + "accept": OARepoAcceptAction, + "submit": OARepoSubmitAction, + "decline": OARepoDeclineAction, + } + description = _("A no-op request to check conditional recipients") + receiver_can_be_none = False + allowed_topic_ref_types = ModelRefTypes(published=False, draft=True) + + class TestWorkflowPolicy(DefaultWithRequestsWorkflowPermissionPolicy): can_read = [ IfInState("draft", [RecordOwners()]), @@ -144,6 +183,11 @@ class WithApprovalPermissionPolicy(DefaultWithRequestsWorkflowPermissionPolicy): permission_policy_cls=WithApprovalPermissionPolicy, request_policy_cls=RequestsWithApprove, ), + "with_ct": Workflow( + label=_("Workflow with approval process"), + permission_policy_cls=WithApprovalPermissionPolicy, + request_policy_cls=RequestsWithCT, + ), } @@ -183,6 +227,15 @@ def ret_data(record_id): return ret_data +@pytest.fixture() +def conditional_recipient_request_data_function(): + def ret_data(record_id): + return { + "request_type": "conditional_recipient_rt", + "topic": {"thesis_draft": record_id}, + } + + return ret_data @pytest.fixture() def edit_record_data_function(): @@ -305,7 +358,7 @@ def app_config(app_config): app_config["OAREPO_REQUESTS_DEFAULT_RECEIVER"] = default_workflow_receiver_function app_config["WORKFLOWS"] = WORKFLOWS - app_config["REQUESTS_REGISTERED_TYPES"] = [ApproveRequestType()] + app_config["REQUESTS_REGISTERED_TYPES"] = [ApproveRequestType(), ConditionalRecipientRequestType()] return app_config @@ -461,7 +514,7 @@ def _create_draft( ): json = copy.deepcopy(default_workflow_json) if custom_workflow: - json["parent"]["workflow_id"] = custom_workflow + json["parent"]["workflow"] = custom_workflow if additional_data: json |= additional_data url = urls["BASE_URL"] + "?expand=true" if expand else urls["BASE_URL"] @@ -515,4 +568,4 @@ def role_ui_serialization(): @pytest.fixture() def default_workflow_json(): - return {"parent": {"workflow_id": "default"}} + return {"parent": {"workflow": "default"}} diff --git a/tests/test_requests/test_conditional_recipient.py b/tests/test_requests/test_conditional_recipient.py new file mode 100644 index 00000000..249134a5 --- /dev/null +++ b/tests/test_requests/test_conditional_recipient.py @@ -0,0 +1,59 @@ +import pytest + +from oarepo_requests.errors import OpenRequestAlreadyExists + +from .utils import link_api2testclient + + +def test_conditional_receiver_creator_matches( + logged_client, + users, + urls, + conditional_recipient_request_data_function, + create_draft_via_resource, + search_clear, +): + # user[0] is creator, user[1] is receiver + # user[0] is not a creator, user[2] is receiver + + creator = users[0] + assert creator.id == '1' + + creator_client = logged_client(creator) + + draft1 = create_draft_via_resource(creator_client, custom_workflow="with_ct") + + resp_request_create = creator_client.post( + urls["BASE_URL_REQUESTS"], + json=conditional_recipient_request_data_function(draft1.json["id"]), + ) + + assert resp_request_create.status_code == 201 + assert resp_request_create.json["receiver"] == {"user": '2'} + + +def test_conditional_receiver_creator_does_not_match( + logged_client, + users, + urls, + conditional_recipient_request_data_function, + create_draft_via_resource, + search_clear, +): + # user[0] is creator, user[1] is receiver + # user[0] is not a creator, user[2] is receiver + + creator = users[1] + assert creator.id != 1 + + creator_client = logged_client(creator) + + draft1 = create_draft_via_resource(creator_client, custom_workflow="with_ct") + + resp_request_create = creator_client.post( + urls["BASE_URL_REQUESTS"], + json=conditional_recipient_request_data_function(draft1.json["id"]), + ) + + assert resp_request_create.status_code == 201 + assert resp_request_create.json["receiver"] == {"user": '3'} diff --git a/tests/thesis_workflows.yaml b/tests/thesis_workflows.yaml index 57ca158f..6b16d15f 100644 --- a/tests/thesis_workflows.yaml +++ b/tests/thesis_workflows.yaml @@ -3,36 +3,6 @@ record: - invenio module: qualified: thesis - permissions: - presets: [ 'workflow' ] - - record: - fields: - state: "{{oarepo_workflows.records.systemfields.state.RecordStateField}}(initial='published')" - - draft: - record: - fields: - state: "{{oarepo_workflows.records.systemfields.state.RecordStateField}}()" - - draft-parent-record: - fields: - workflow: "{{oarepo_workflows.records.systemfields.workflow.WorkflowField}}()" - - service-config: - components: - - "{{oarepo_workflows.services.components.workflow.WorkflowComponent}}" - - parent-record-marshmallow: - base-classes: - - oarepo_workflows.services.records.schema.WorkflowParentSchema - - - draft-parent-record-metadata: - base-classes: - - oarepo_workflows.records.models.RecordWorkflowParentModelMixin - - invenio_db.db{db.Model} - - invenio_records.models.RecordMetadataBase properties: metadata: @@ -44,6 +14,8 @@ record: contributors[]: type: keyword + draft: {} + settings: schema-server: 'local://' profiles: From 6a0f0f8661c277035cd0116da2288081a3317feb Mon Sep 17 00:00:00 2001 From: Mirek Simek Date: Sun, 11 Aug 2024 20:56:11 +0200 Subject: [PATCH 2/3] Renamed condition to if_ --- oarepo_requests/services/permissions/generators.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/oarepo_requests/services/permissions/generators.py b/oarepo_requests/services/permissions/generators.py index 39c32a56..4ae3cb61 100644 --- a/oarepo_requests/services/permissions/generators.py +++ b/oarepo_requests/services/permissions/generators.py @@ -64,11 +64,11 @@ def query_filter(self, record=None, request_type=None, **kwargs): class IfRequestedBy(RecipientGeneratorMixin, ConditionalGenerator): - def __init__(self, conditions, then_, else_): + def __init__(self, if_, then_, else_): super().__init__(then_, else_) - if not isinstance(conditions, (list, tuple)): - conditions = [conditions] - self._conditions = conditions + if not isinstance(if_, (list, tuple)): + if_ = [if_] + self.if_ = if_ def _condition(self, *, request_type, creator, **kwargs): """Condition to choose generators set.""" @@ -81,7 +81,7 @@ def _condition(self, *, request_type, creator, **kwargs): creator = current_requests.entity_resolvers_registry.reference_entity(creator) needs = creator.get_needs() - for condition in self._conditions: + for condition in self.if_: condition_needs = set(condition.needs(request_type=request_type, creator=creator, **kwargs)) condition_excludes = set(condition.excludes(request_type=request_type, creator=creator, **kwargs)) From d996a7ce2cd039fba4f228b7b49723239998d541 Mon Sep 17 00:00:00 2001 From: Mirek Simek Date: Sun, 11 Aug 2024 20:58:47 +0200 Subject: [PATCH 3/3] renamed if_ to requesters --- oarepo_requests/services/permissions/generators.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/oarepo_requests/services/permissions/generators.py b/oarepo_requests/services/permissions/generators.py index 4ae3cb61..4dacd523 100644 --- a/oarepo_requests/services/permissions/generators.py +++ b/oarepo_requests/services/permissions/generators.py @@ -64,11 +64,11 @@ def query_filter(self, record=None, request_type=None, **kwargs): class IfRequestedBy(RecipientGeneratorMixin, ConditionalGenerator): - def __init__(self, if_, then_, else_): + def __init__(self, requesters, then_, else_): super().__init__(then_, else_) - if not isinstance(if_, (list, tuple)): - if_ = [if_] - self.if_ = if_ + if not isinstance(requesters, (list, tuple)): + requesters = [requesters] + self.requesters = requesters def _condition(self, *, request_type, creator, **kwargs): """Condition to choose generators set.""" @@ -81,7 +81,7 @@ def _condition(self, *, request_type, creator, **kwargs): creator = current_requests.entity_resolvers_registry.reference_entity(creator) needs = creator.get_needs() - for condition in self.if_: + for condition in self.requesters: condition_needs = set(condition.needs(request_type=request_type, creator=creator, **kwargs)) condition_excludes = set(condition.excludes(request_type=request_type, creator=creator, **kwargs))