diff --git a/oarepo_requests/actions/edit_topic.py b/oarepo_requests/actions/edit_topic.py index 1002113..4740230 100644 --- a/oarepo_requests/actions/edit_topic.py +++ b/oarepo_requests/actions/edit_topic.py @@ -13,7 +13,6 @@ from oarepo_runtime.datastreams.utils import get_record_service_for_record -from .cascade_events import update_topic from .generic import AddTopicLinksOnPayloadMixin, OARepoAcceptAction if TYPE_CHECKING: @@ -44,5 +43,4 @@ def apply( if not topic_service: raise KeyError(f"topic {topic} service not found") edit_topic = topic_service.edit(identity, topic["id"], uow=uow) - update_topic(self.request, topic, edit_topic._record, uow) super().apply(identity, request_type, edit_topic, uow, *args, **kwargs) diff --git a/oarepo_requests/actions/generic.py b/oarepo_requests/actions/generic.py index 3c2cd3e..43f5083 100644 --- a/oarepo_requests/actions/generic.py +++ b/oarepo_requests/actions/generic.py @@ -52,7 +52,6 @@ def apply( **kwargs: Any, ) -> None: """Apply the action to the topic.""" - pass def _execute_with_components( self, @@ -127,8 +126,13 @@ def apply( # invenio does not allow non-string values in the payload, so using colon notation here # client will need to handle this and convert to links structure # can not use dot notation as marshmallow tries to be too smart and does not serialize dotted keys - request["payload"][self.self_link] = topic_dict["links"]["self"] - request["payload"][self.self_html_link] = topic_dict["links"]["self_html"] + if ( + "self" in topic_dict["links"] + ): # todo consider - this happens if receiver doesn't have read rights to the topic, like after a draft is created after edit + # if it's needed in all cases, we could do a system identity call here + request["payload"][self.self_link] = topic_dict["links"]["self"] + if "self_html" in topic_dict["links"]: + request["payload"][self.self_html_link] = topic_dict["links"]["self_html"] return topic._record diff --git a/oarepo_requests/actions/new_version.py b/oarepo_requests/actions/new_version.py index 8df194c..3d7c28c 100644 --- a/oarepo_requests/actions/new_version.py +++ b/oarepo_requests/actions/new_version.py @@ -13,7 +13,6 @@ from oarepo_runtime.datastreams.utils import get_record_service_for_record -from .cascade_events import update_topic from .generic import AddTopicLinksOnPayloadMixin, OARepoAcceptAction if TYPE_CHECKING: @@ -50,7 +49,11 @@ def apply( and self.request["payload"]["keep_files"] == "true" ): topic_service.import_files(identity, new_version_topic.id) - update_topic(self.request, topic, new_version_topic._record, uow) + + if "payload" not in self.request: + self.request["payload"] = {} + self.request["payload"]["draft_record:id"] = new_version_topic["id"] + return super().apply( identity, request_type, new_version_topic, uow, *args, **kwargs ) diff --git a/oarepo_requests/resolvers/interface.py b/oarepo_requests/resolvers/interface.py new file mode 100644 index 0000000..980f302 --- /dev/null +++ b/oarepo_requests/resolvers/interface.py @@ -0,0 +1,47 @@ +from __future__ import annotations + +from typing import Any, TYPE_CHECKING + +from invenio_pidstore.errors import PersistentIdentifierError + +from oarepo_requests.resolvers.ui import resolve +import logging +if TYPE_CHECKING: + from invenio_requests.records import Request +log = logging.getLogger(__name__) + +# todo consider - we are not using this strictly in the ui context - so how should we separate these things in the future +def resolve_entity(entity: str, obj: Request, ctx: dict[str, Any]) -> dict: + """Resolve the entity and put it into the context cache. + + :param obj: Request object + :param ctx: Context cache + :return: The resolved entity + """ + entity_field_value = getattr(obj, entity) + if not entity_field_value: + return {} + + reference_dict: dict = entity_field_value.reference_dict + + key = entity_context_key(reference_dict) + if key in ctx: + return ctx[key] + try: + entity = resolve(ctx["identity"], reference_dict) + except Exception as e: # noqa + if not isinstance(e, PersistentIdentifierError): + log.exception( + "Error resolving %s for identity %s", + reference_dict, + ctx["identity"], + ) + entity = {"links": {}} + ctx[key] = entity + return entity + + +def entity_context_key(reference_dict: dict) -> str: + return "entity:" + ":".join( + f"{x[0]}:{x[1]}" for x in sorted(reference_dict.items()) + ) diff --git a/oarepo_requests/services/oarepo/config.py b/oarepo_requests/services/oarepo/config.py index 0345b97..d469223 100644 --- a/oarepo_requests/services/oarepo/config.py +++ b/oarepo_requests/services/oarepo/config.py @@ -10,14 +10,13 @@ from __future__ import annotations import logging -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING -from invenio_pidstore.errors import PersistentIdentifierError from invenio_records_resources.services.base.links import Link from invenio_requests.services import RequestsServiceConfig from invenio_requests.services.requests import RequestLink -from oarepo_requests.resolvers.ui import resolve +from oarepo_requests.resolvers.interface import resolve_entity if TYPE_CHECKING: from invenio_requests.records.api import Request @@ -27,36 +26,6 @@ class RequestEntityLinks(Link): """Utility class for keeping track of and resolve links.""" - def _resolve(self, obj: Request, ctx: dict[str, Any]) -> dict: - """Resolve the entity and put it into the context cache. - - :param obj: Request object - :param ctx: Context cache - :return: The resolved entity - """ - entity_field_value = getattr(obj, self._entity) - if not entity_field_value: - return {} - - reference_dict: dict = entity_field_value.reference_dict - key = "entity:" + ":".join( - f"{x[0]}:{x[1]}" for x in sorted(reference_dict.items()) - ) - if key in ctx: - return ctx[key] - try: - entity = resolve(ctx["identity"], reference_dict) - except Exception as e: # noqa - if not isinstance(e, PersistentIdentifierError): - log.exception( - "Error resolving %s for identity %s", - reference_dict, - ctx["identity"], - ) - entity = {"links": {}} - ctx[key] = entity - return entity - def __init__(self, entity: str, when: callable = None): """Constructor.""" self._entity = entity @@ -65,36 +34,24 @@ def __init__(self, entity: str, when: callable = None): def expand(self, obj: Request, context: dict) -> dict: """Create the request links.""" res = {} - resolved = self._resolve(obj, context) + resolved = resolve_entity(self._entity, obj, context) if "links" in resolved: res.update(resolved["links"]) - if hasattr(obj.type, "extra_entity_links"): - entity = self._resolve(obj, context) - res.update( - obj.type.extra_entity_links( - request=obj, entity=entity, entity_type=self._entity - ) - ) - return res -class RedirectLinks(Link): +class RedirectLink(Link): def __init__(self, when: callable = None): """Constructor.""" self._when_func = when def expand(self, obj: Request, context: dict) -> dict: """Create the request links.""" - links = {} - available_statuses = {"accept", "decline", "submit"} - for status in available_statuses: - if hasattr(obj.type, f"{status}_redirect_url"): - links[status] = getattr(obj.type, f"{status}_redirect_url")( - obj, context - ) - return links + link = None + if hasattr(obj.type, "get_ui_redirect_url"): + link = getattr(obj.type, "get_ui_redirect_url")(obj, context) + return link class OARepoRequestsServiceConfig(RequestsServiceConfig): @@ -110,5 +67,5 @@ class OARepoRequestsServiceConfig(RequestsServiceConfig): "topic": RequestEntityLinks(entity="topic"), "created_by": RequestEntityLinks(entity="created_by"), "receiver": RequestEntityLinks(entity="receiver"), - "redirect_urls": RedirectLinks(), + "ui_redirect_url": RedirectLink(), } diff --git a/oarepo_requests/types/delete_draft.py b/oarepo_requests/types/delete_draft.py index 5f033ba..a53be07 100644 --- a/oarepo_requests/types/delete_draft.py +++ b/oarepo_requests/types/delete_draft.py @@ -36,10 +36,11 @@ class DeleteDraftRequestType(NonDuplicableOARepoRequestType): dangerous = True - def accept_redirect_url(self, request: Request, context: dict, **kwargs): - topic_cls = request.topic.record_cls - service = get_record_service_for_record_class(topic_cls) - return service.config.links_search["self_html"].expand(None, context) + def get_ui_redirect_url(self, request: Request, context: dict) -> str: + if request.status == "accepted": + topic_cls = request.topic.record_cls + service = get_record_service_for_record_class(topic_cls) + return service.config.links_search["self_html"].expand(None, context) @classproperty def available_actions(cls) -> dict[str, type[RequestAction]]: diff --git a/oarepo_requests/types/delete_published_record.py b/oarepo_requests/types/delete_published_record.py index 8f8723e..59f904b 100644 --- a/oarepo_requests/types/delete_published_record.py +++ b/oarepo_requests/types/delete_published_record.py @@ -38,10 +38,11 @@ class DeletePublishedRecordRequestType(NonDuplicableOARepoRequestType): type_id = "delete_published_record" name = _("Delete record") - def accept_redirect_url(self, request: Request, context: dict, **kwargs): - topic_cls = request.topic.record_cls - service = get_record_service_for_record_class(topic_cls) - return service.config.links_search["self_html"].expand(None, context) + def get_ui_redirect_url(self, request: Request, context: dict) -> str: + if request.status == "accepted": + topic_cls = request.topic.record_cls + service = get_record_service_for_record_class(topic_cls) + return service.config.links_search["self_html"].expand(None, context) dangerous = True diff --git a/oarepo_requests/types/edit_record.py b/oarepo_requests/types/edit_record.py index 19adfde..56a81f9 100644 --- a/oarepo_requests/types/edit_record.py +++ b/oarepo_requests/types/edit_record.py @@ -12,9 +12,12 @@ from typing import TYPE_CHECKING, Any import marshmallow as ma +from invenio_drafts_resources.resources.records.errors import DraftNotCreatedError +from invenio_records_resources.services.errors import PermissionDeniedError from invenio_records_resources.services.uow import RecordCommitOp, UnitOfWork from invenio_requests.proxies import current_requests_service from invenio_requests.records.api import Request +from oarepo_runtime.datastreams.utils import get_record_service_for_record_class from oarepo_runtime.i18n import lazy_gettext as _ from typing_extensions import override @@ -54,11 +57,20 @@ class EditPublishedRecordRequestType(NonDuplicableOARepoRequestType): ), } - def extra_entity_links(self, request: Request, entity: dict, entity_type: str, **kwargs) -> dict: - if request.status == "accepted" and entity_type == "topic": - return {"topic_redirect_link": entity["links"]["edit_html"]} - else: - return {} + def get_ui_redirect_url(self, request: Request, ctx: dict) -> str: + if request.status == "accepted": + service = get_record_service_for_record_class(request.topic.record_cls) + try: + result_item = service.read_draft( + ctx["identity"], request.topic._parse_ref_dict_id() + ) + except (PermissionDeniedError, DraftNotCreatedError): + return None + + if "edit_html" in result_item["links"]: + return result_item["links"]["edit_html"] + elif "self_html" in result_item["links"]: + return result_item["links"]["self_html"] @classproperty def available_actions(cls) -> dict[str, type[RequestAction]]: diff --git a/oarepo_requests/types/new_version.py b/oarepo_requests/types/new_version.py index a5a8337..eacb47e 100644 --- a/oarepo_requests/types/new_version.py +++ b/oarepo_requests/types/new_version.py @@ -12,9 +12,12 @@ from typing import TYPE_CHECKING, Any import marshmallow as ma +from invenio_drafts_resources.resources.records.errors import DraftNotCreatedError +from invenio_records_resources.services.errors import PermissionDeniedError from invenio_records_resources.services.uow import RecordCommitOp, UnitOfWork from invenio_requests.proxies import current_requests_service from marshmallow.validate import OneOf +from oarepo_runtime.datastreams.utils import get_record_service_for_record_class from oarepo_runtime.i18n import lazy_gettext as _ from typing_extensions import override @@ -47,14 +50,27 @@ class NewVersionRequestType(NonDuplicableOARepoRequestType): attribute="draft_record:links:self_html", data_key="draft_record:links:self_html", ), + "draft_record.id": ma.fields.Str( + attribute="draft_record:id", + data_key="draft_record:id", + ), "keep_files": ma.fields.String(validate=OneOf(["true", "false"])), } - def extra_entity_links(self, request: Request, entity: dict, entity_type: str, **kwargs) -> dict: - if request.status == "accepted" and entity_type == "topic": - return {"topic_redirect_link": entity["links"]["edit_html"]} - else: - return {} + def get_ui_redirect_url(self, request: Request, ctx: dict) -> str: + if request.status == "accepted": + service = get_record_service_for_record_class(request.topic.record_cls) + try: + result_item = service.read_draft( + ctx["identity"], request["payload"]["draft_record:id"] + ) + except (PermissionDeniedError, DraftNotCreatedError): + return None + + if "edit_html" in result_item["links"]: + return result_item["links"]["edit_html"] + elif "self_html" in result_item["links"]: + return result_item["links"]["self_html"] @classproperty def available_actions(cls) -> dict[str, type[RequestAction]]: diff --git a/setup.cfg b/setup.cfg index 6a62c84..a98e710 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,6 +1,6 @@ [metadata] name = oarepo-requests -version = 2.3.10 +version = 2.3.11 description = authors = Ronald Krist readme = README.md diff --git a/tests/conftest.py b/tests/conftest.py index d43474a..87977a7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -141,6 +141,16 @@ class RequestsWithDifferentRecipients(DefaultRequests): requesters=[AnyUser()], recipients=[UserGenerator(1)], ) + edit_published_record = WorkflowRequest( + requesters=[IfNoEditDraft([IfInState("published", [RecordOwners()])])], + recipients=[UserGenerator(2)], + transitions=WorkflowTransitions(), + ) + new_version = WorkflowRequest( + requesters=[IfNoNewVersionDraft([IfInState("published", [RecordOwners()])])], + recipients=[UserGenerator(2)], + transitions=WorkflowTransitions(), + ) class RequestsWithApprove(WorkflowRequestPolicy): @@ -638,7 +648,7 @@ def record(identity, custom_workflow=None, additional_data=None): json = copy.deepcopy(default_workflow_json) if custom_workflow: # specifying this assumes use of workflows json["parent"]["workflow"] = custom_workflow - json = { + json_metadata = { "metadata": { "creators": [ "Creator 1", @@ -647,7 +657,7 @@ def record(identity, custom_workflow=None, additional_data=None): "contributors": ["Contributor 1"], } } - json = always_merger.merge(json, default_workflow_json) + json = always_merger.merge(json, json_metadata) if additional_data: always_merger.merge(json, additional_data) draft = record_service.create(identity, json) diff --git a/tests/test_requests/test_delete.py b/tests/test_requests/test_delete.py index 5dceff8..fab8e93 100644 --- a/tests/test_requests/test_delete.py +++ b/tests/test_requests/test_delete.py @@ -51,8 +51,7 @@ def test_delete( ), ) assert ( - link2testclient(delete.json["links"]["redirect_urls"]["accept"], ui=True) - == "/thesis/" + link2testclient(delete.json["links"]["ui_redirect_url"], ui=True) == "/thesis/" ) ThesisRecord.index.refresh() @@ -134,7 +133,7 @@ def test_delete_draft( assert request_after.json["status"] == "accepted" assert request_after.json["is_closed"] assert ( - link2testclient(request_after.json["links"]["redirect_urls"]["accept"], ui=True) - == "/thesis/" + link2testclient(request_after.json["links"]["ui_redirect_url"], ui=True) + == "/thesis/" ) assert read_deleted.status_code == 404 diff --git a/tests/test_requests/test_edit.py b/tests/test_requests/test_edit.py index ae5f1ed..c737281 100644 --- a/tests/test_requests/test_edit.py +++ b/tests/test_requests/test_edit.py @@ -7,7 +7,7 @@ # from thesis.records.api import ThesisDraft, ThesisRecord -from tests.test_requests.utils import link2testclient +from tests.test_requests.utils import _create_request, link2testclient def test_edit_autoaccept( @@ -61,3 +61,82 @@ def test_edit_autoaccept( assert len(search) == 1 assert search[0]["links"]["self"].endswith("/draft") assert search[0]["id"] == id_ + + +def test_redirect_url( + vocab_cf, + logged_client, + users, + urls, + edit_record_data_function, + record_factory, + search_clear, +): + creator = users[0] + receiver = users[1] + creator_client = logged_client(creator) + receiver_client = logged_client(receiver) + + record1 = record_factory(creator.identity, custom_workflow="different_recipients") + id_ = record1["id"] + + resp_request_create = creator_client.post( + urls["BASE_URL_REQUESTS"], + json=edit_record_data_function(record1["id"]), + ) + edit_request_id = resp_request_create.json["id"] + resp_request_submit = creator_client.post( + link2testclient(resp_request_create.json["links"]["actions"]["submit"]), + ) + receiver_get = receiver_client.get(f"{urls['BASE_URL_REQUESTS']}{edit_request_id}") + resp_request_accept = receiver_client.post( + link2testclient(receiver_get.json["links"]["actions"]["accept"]) + ) + # is request accepted and closed? + request = creator_client.get( + f'{urls["BASE_URL_REQUESTS"]}{edit_request_id}', + ).json + + creator_edit_accepted = creator_client.get( + f'{urls["BASE_URL_REQUESTS"]}{edit_request_id}', + ).json + receiver_edit_accepted = receiver_client.get( + f'{urls["BASE_URL_REQUESTS"]}{edit_request_id}', + ).json # receiver should be able to get the request but not to edit the draft - should not receive edit link + + assert ( + link2testclient(creator_edit_accepted["links"]["ui_redirect_url"], ui=True) + == f"/thesis/{record1['id']}/edit" + ) + assert receiver_edit_accepted["links"]["ui_redirect_url"] == None + + publish_request = _create_request(creator_client, id_, "publish_draft", urls) + creator_client.post( + link2testclient(publish_request.json["links"]["actions"]["submit"]) + ) + receiver_edit_request_after_publish_draft_submitted = receiver_client.get( + f"{urls['BASE_URL_REQUESTS']}{edit_request_id}" + ).json # now receiver should have a right to view but not edit the topic + assert ( + link2testclient( + receiver_edit_request_after_publish_draft_submitted["links"][ + "ui_redirect_url" + ], + ui=True, + ) + == f"/thesis/{record1['id']}/preview" + ) + + receiver_publish_request = receiver_client.get( + f"{urls['BASE_URL_REQUESTS']}{publish_request.json['id']}" + ).json + receiver_client.post( + link2testclient(receiver_publish_request["links"]["actions"]["accept"]) + ) + + creator_edit_request_after_merge = creator_client.get( + f'{urls["BASE_URL_REQUESTS"]}{edit_request_id}', + ).json + assert ( + creator_edit_request_after_merge["links"]["ui_redirect_url"] == None + ) # draft now doesn't exist so we can't redirect to it diff --git a/tests/test_requests/test_new_version.py b/tests/test_requests/test_new_version.py index dd16775..57d7d21 100644 --- a/tests/test_requests/test_new_version.py +++ b/tests/test_requests/test_new_version.py @@ -8,7 +8,7 @@ from thesis.records.api import ThesisDraft, ThesisRecord -from tests.test_requests.utils import link2testclient +from tests.test_requests.utils import _create_request, link2testclient def test_new_version_autoaccept( @@ -136,19 +136,22 @@ def test_redirect_url( ): creator = users[0] creator_client = logged_client(creator) + receiver_client = logged_client(users[1]) record1 = record_factory(creator.identity) + original_id = record1["id"] resp_request_create = creator_client.post( urls["BASE_URL_REQUESTS"], - json=new_version_data_function(record1["id"]), + json=new_version_data_function(original_id), ) + original_request_id = resp_request_create.json["id"] resp_request_submit = creator_client.post( link2testclient(resp_request_create.json["links"]["actions"]["submit"]), ) # is request accepted and closed? request = creator_client.get( - f'{urls["BASE_URL_REQUESTS"]}{resp_request_create.json["id"]}', + f'{urls["BASE_URL_REQUESTS"]}{original_request_id}', ).json ThesisDraft.index.refresh() @@ -161,6 +164,26 @@ def test_redirect_url( if x["parent"]["id"] == record1.parent["id"] and x["state"] == "draft" ][0] assert ( - request["links"]["topic"]["topic_redirect_link"] - == f"https://127.0.0.1:5000/thesis/{new_version['id']}/edit" + link2testclient(request["links"]["ui_redirect_url"], ui=True) + == f"/thesis/{new_version['id']}/edit" + ) + + publish_request = _create_request( + creator_client, new_version["id"], "publish_draft", urls + ) + submit = creator_client.post( + link2testclient(publish_request.json["links"]["actions"]["submit"]) ) + receiver_request = receiver_client.get( + f"{urls['BASE_URL_REQUESTS']}{submit.json['id']}" + ) + accept = receiver_client.post( + link2testclient(receiver_request.json["links"]["actions"]["accept"]) + ) + + original_request = creator_client.get( + f'{urls["BASE_URL_REQUESTS"]}{original_request_id}', + ).json + assert original_request["topic"] == { + "thesis": original_id + } # check no weird topic kerfluffle happened here diff --git a/tests/test_requests/test_topic_update.py b/tests/test_requests/test_topic_update.py index 8897270..f48e522 100644 --- a/tests/test_requests/test_topic_update.py +++ b/tests/test_requests/test_topic_update.py @@ -5,7 +5,6 @@ # modify it under the terms of the MIT License; see LICENSE file for more # details. # -from thesis.records.api import ThesisDraft from .utils import link2testclient @@ -43,67 +42,3 @@ def test_publish( ), ) check_publish_topic_update(creator_client, urls, record, resp_request_create) - - -def test_edit( - vocab_cf, - logged_client, - users, - urls, - edit_record_data_function, - record_factory, - search_clear, -): - creator = users[0] - creator_client = logged_client(creator) - - record1 = record_factory(creator.identity) - id_ = record1["id"] - - resp_request_create = creator_client.post( - urls["BASE_URL_REQUESTS"], - json=edit_record_data_function(record1["id"]), - ) - resp_request_submit = creator_client.post( - link2testclient(resp_request_create.json["links"]["actions"]["submit"]), - ) - # is request accepted and closed? - request = creator_client.get( - f'{urls["BASE_URL_REQUESTS"]}{resp_request_create.json["id"]}', - ).json - - assert request["topic"] == {"thesis_draft": id_} - - -def test_new_version( - vocab_cf, - logged_client, - users, - urls, - new_version_data_function, - record_factory, - search_clear, -): - creator = users[0] - creator_client = logged_client(creator) - - record1 = record_factory(creator.identity) - id_ = record1["id"] - - resp_request_create = creator_client.post( - urls["BASE_URL_REQUESTS"], - json=new_version_data_function(record1["id"]), - ) - resp_request_submit = creator_client.post( - link2testclient(resp_request_create.json["links"]["actions"]["submit"]), - ) - # is request accepted and closed? - request = creator_client.get( - f'{urls["BASE_URL_REQUESTS"]}{resp_request_create.json["id"]}', - ).json - ThesisDraft.index.refresh() - new_draft_id = creator_client.get(f"/user{urls['BASE_URL']}").json["hits"]["hits"][ - 0 - ]["id"] - assert new_draft_id != id_ - assert request["topic"] == {"thesis_draft": new_draft_id} diff --git a/tests/test_requests/utils.py b/tests/test_requests/utils.py index 500c191..15c344a 100644 --- a/tests/test_requests/utils.py +++ b/tests/test_requests/utils.py @@ -13,6 +13,19 @@ def link2testclient(link, ui=False): return link[len(base_string) - 1 :] +def _create_request(client, record_id, request_type, urls): + applicable = client.get( + f"{urls['BASE_URL']}{record_id}/draft/requests/applicable" + ).json["hits"]["hits"] + request_link = [ + type_["links"]["actions"]["create"] + for type_ in applicable + if type_["type_id"] == request_type + ][0] + ret = client.post(link2testclient(request_link)) + return ret + + # from chatgpt def dict_diff(dict1, dict2, path=""): ret = defaultdict(list)