feat(events): deep links #503

palkerecsenyi · 2025-10-21T07:30:01Z

Closes #494

Added a "Copy link" button to events/comments in the UI, which copies the current URL with a hash containing the event ID.
When TimelineFeed is mounted, it sees if there's a relevant hash in the URL, and requests the server to return the page that contains that event (instead of just requesting the first page).
Added an argument to the request event search route to return the page containing a given event instead of a specific numbered page. The actual page number chosen is returned by the server (see feat(records): return page number in RecordList invenio-records-resources#656). This is calculated by counting the number of records older than the specified one and dividing by the page size.

Screenshots

This copies the following link:

https://127.0.0.1:5000/communities/pal/requests/8642288d-5938-4eb7-a435-728d7266aa09#commentevent-03a3b14c-1f52-46fd-a60a-5b00ceefd7a6

And shows up like this when navigated to:

invenio_requests/assets/semantic-ui/js/invenio_requests/timelineEvents/utils.js

invenio_requests/assets/semantic-ui/js/invenio_requests/timelineEvents/TimelineCommentEvent.js

zzacharo · 2025-10-21T07:39:19Z

invenio_requests/services/events/service.py

        )
-        search_result = search.execute()

+        if include_event_id is not None:


Can we add the include_event_id as new Param to isolate the behaviour of the filtering?

invenio_requests/services/events/service.py

ntarocco · 2025-10-22T13:14:47Z

invenio_requests/assets/semantic-ui/js/invenio_requests/timeline/state/actions.js

  if (concurrentRequests) return;

-  dispatch(fetchTimeline(false));
+  dispatch(fetchTimeline(undefined, false));


Would it be possible to improve this?
fetchTimeline(undefined, false) looks... ugly?

Can we refactor that fetchTimeline and maybe remove the loadingState param (split in 2 different functions?) so that the new optional param is missing or defined?

I moved the IS_LOADING dispatch into the getTimelineWithRefresh function which is the only place where loadingState = true was used, so this way I got rid of loadingState completely. Hope this is okay, the calls look less ugly now.

invenio_requests/assets/semantic-ui/js/invenio_requests/timeline/state/actions.js

invenio_requests/assets/semantic-ui/js/invenio_requests/timelineEvents/utils.js

invenio_requests/services/events/service.py

zzacharo · 2025-10-22T14:03:57Z

@ntarocco regarding #503 (comment)

Just to be clear the new functionality returns the search results of the page that the given event_id falls into. So, in principle is a search but ensuring that the results you get back include the event you passed. I am happy to discuss for a new API, but how would it look like?

Currently, from REST API we have: /api/requests/<id>/timeline?include_event_id=<event_id>

ntarocco · 2025-10-22T14:23:55Z

@ntarocco regarding #503 (comment)

Just to be clear the new functionality returns the search results of the page that the given event_id falls into. So, in principle is a search but ensuring that the results you get back include the event you passed. I am happy to discuss for a new API, but how would it look like?

Currently, from REST API we have: /api/requests/<id>/timeline?include_event_id=<event_id>

Yes understood, the feeling is that we are specializing the search endpoint with a specific behaviour. I would rather implement this specific logic in a dedicated method, which then calls the "generic search" passing the page or from/to or whatever with the classic API.

palkerecsenyi · 2025-10-22T14:32:46Z

Ahh okay, I am happy to try this and we can see what it looks like! My main concern is making sure we keep to minimal code duplication, and this would probably do that.

invenio_requests/services/events/service.py

ntarocco

Looks better to me :)
Don't forget to add the REST API endpoint in the doc, and mention it in the vNext release notes, thank you!

invenio_requests/assets/semantic-ui/js/invenio_requests/api/InvenioRequestApi.js

invenio_requests/assets/semantic-ui/js/invenio_requests/api/InvenioRequestEventsApi.js

invenio_requests/assets/semantic-ui/js/invenio_requests/timeline/state/actions.js

invenio_requests/services/events/config.py

zzacharo · 2025-10-29T10:41:54Z

invenio_requests/assets/semantic-ui/js/invenio_requests/timelineEvents/TimelineCommentEvent.js

      <Overridable id={`TimelineEvent.layout.${this.eventToType(event)}`} event={event}>
-        <RequestsFeed.Item>
+        <RequestsFeed.Item
+          id={`commentevent-${event.id}`}


Can you take this from the self_html link of the comment? Just to make it in sync in case someone changes it in the backend?

I've resolved this as much as possible but I think we still have to hardcode the commentevent for the function that retrieves the event ID from the URL on page load.

zzacharo · 2025-10-30T13:13:07Z

invenio_requests/services/events/service.py

        return [EntityResolverExpandableField("created_by")]

-    def links_item_tpl_for_request(self, request):
+    def links_item_tpl_for_request_type(self, request_type: str):


hmmm that s breaking change for the API of the class....Can we avoid it?

Ah I think the name only changed between two of my commits within this PR. The method is actually new, neither links_item_tpl_for_request nor links_item_tpl_for_request_type exists within the class currently.

Can you squash the commits maybe?

@ntarocco @slint @kpsherva any objection to this?

zzacharo · 2025-10-30T13:14:10Z

invenio_requests/notifications/generators.py

        )
+        # Override the _links_item_tpl as done by all other RecordList-returning methods in the
+        # RequestEvents service.
+        request_events._links_item_tpl = (


Why did we need to override this?

In several methods of RequestEventsService, we are returning a self.result_list or self.result_item, which return RequestEventItem or RequestEventList respectively. By default (in some of the default method implementations of invenio_records_resources.services.records.RecordService), we return self.result_list passing self.links_item_tpl as the value for the links_item_tpl param, for example in the scan method.

However, in RequestEventsService, we cannot simply implement links_item_tpl as a property method, since we need to pass the request type into it so we can generate the correct variables for RequestEventLinks. So instead, everywhere we call result_list or result_item we pass self.links_item_tpl_for_request_type(str(request.type)) as the links_tpl or links_item_tpl value as appropriate.

However, we don't override the default scan method from RecordService in RequestEventsService. As a result, we are keeping its self.result_list call along with its default value of self.links_item_tpl. We could override the scan method entirely, but I went with the solution of just replacing the self._links_item_tpl of the returned RequestEventList class after it is returned.

@fenekku any idea to make this more elegant maybe?

Yeah, after spending a lot of time thinking about this actually, I would say to implement scan (taking request_id like search) and defining the links_item_pl there. We really don't want to reach to _links_item_tpl since it's private.

Thank you for taking a look at this! I am happy to go for that approach, I will implement it now :)

invenio_requests/assets/semantic-ui/js/invenio_requests/timelineEvents/utils.js

fenekku · 2025-11-06T15:26:23Z

invenio_requests/services/events/config.py

    # ResultItem configurations
    links_item = {
        "self": RequestEventLink("{+api}/requests/{request_id}/comments/{id}"),
+        "self_html": RequestEventLink("{+ui}/requests/{request_id}#commentevent-{id}"),


Note to self: going to have to implement anchor to fully move away from Link

https://github.com/inveniosoftware/invenio-base/blob/master/invenio_base/urls/helpers.py#L22

Ahh I was also going to mention this but I forgot, I saw we are removing Link in v14. But at the moment I wasn't sure how to express this as an EndpointLink given the anchor. Thanks for catching it

fenekku

Thanks for that and kudos for going through that code. I was trying to work out "nicer" ways of dealing with all this all morning and nothing was particularly easier (more a commentary on the codebase we have 😄 ).

slint · 2025-11-07T14:04:38Z

invenio_requests/services/events/config.py

-        vars.update({"id": record.id, "request_id": record.request_id})
+        request_type = current_request_type_registry.lookup(vars["request_type"])
+        vars.update({"id": obj.id, "request_id": obj.request_id})
+        vars.update(request_type._update_link_config(**vars))


shelve: given that this pattern now has spread also to request events, it's time to actually revisit how we define request types in a way that keeps all the logic for the different layers (data, service, presentation, etc.) contained under a single interface.

We already have #332 which specifically describes the issue for links, but other bad examples are:

UI display type labels for all types of requests are defined in this module (which shouldn't know about implementations of other requests): https://github.com/inveniosoftware/invenio-requests/blob/master/invenio_requests/assets/semantic-ui/js/invenio_requests/contrib/labels/TypeLabel.js

Jinja view rendering for the /me/requests/... and /communities/{comm_id}/requests/... pages is all defined in https://github.com/inveniosoftware/invenio-app-rdm/blob/master/invenio_app_rdm/requests_ui/views/requests.py

We should discuss in what way we can bring all of these implementation aspects under a single file/definition/interface.

invenio_requests/services/events/service.py

slint · 2025-11-07T14:16:36Z

invenio_requests/notifications/generators.py


        # fetching all request events to get involved users
        request_events = current_events_service.scan(
+            request_id=request["id"],


shelve: looking at this now, it's not great that we go through all of the comments to find the participants (imagine 10k comments between just 2 participants)... we should instead:

Keep this, but only fetch the created_by.user field via the source search parameter

Do an aggregation query (terms bucket) on created_by.user to fetch the unique user IDs

Tracking this in #508

invenio_requests/assets/semantic-ui/js/invenio_requests/api/InvenioRequestApi.js

invenio_requests/assets/semantic-ui/js/invenio_requests/timeline/state/actions.js

* Added a "Copy link" button to events/comments in the UI, which copies the current URL with a hash containing the event ID. * When TimelineFeed is mounted, it sees if there's a relevant hash in the URL, and requests the server to return the page that contains that event (instead of just requesting the first page). * Added an argument to the request event `search` route to return the page containing a given event instead of a specific numbered page. The actual page number chosen is returned by the server (see inveniosoftware/invenio-records-resources#656). This is calculated by counting the number of records older than the specified one and dividing by the page size.

palkerecsenyi force-pushed the deep-links branch 2 times, most recently from 139ce3c to c1ee20b Compare October 21, 2025 07:39

zzacharo reviewed Oct 21, 2025

View reviewed changes

palkerecsenyi force-pushed the deep-links branch from 2684c2a to 78cead2 Compare October 21, 2025 11:47

ntarocco reviewed Oct 22, 2025

View reviewed changes

palkerecsenyi force-pushed the deep-links branch from aa49bb8 to 1e822c3 Compare October 24, 2025 14:00

zzacharo reviewed Oct 24, 2025

View reviewed changes

invenio_requests/services/events/service.py Outdated Show resolved Hide resolved

palkerecsenyi force-pushed the deep-links branch from 7365fc5 to 788ae96 Compare October 24, 2025 14:36

zzacharo reviewed Oct 27, 2025

View reviewed changes

invenio_requests/services/events/service.py Show resolved Hide resolved

ntarocco reviewed Oct 27, 2025

View reviewed changes

invenio_requests/assets/semantic-ui/js/invenio_requests/api/InvenioRequestApi.js Outdated Show resolved Hide resolved

invenio_requests/assets/semantic-ui/js/invenio_requests/api/InvenioRequestApi.js Outdated Show resolved Hide resolved

palkerecsenyi force-pushed the deep-links branch from a410047 to 9cba9ff Compare October 27, 2025 15:55

zzacharo reviewed Oct 29, 2025

View reviewed changes

zzacharo reviewed Oct 30, 2025

View reviewed changes

invenio_requests/assets/semantic-ui/js/invenio_requests/timelineEvents/utils.js Outdated Show resolved Hide resolved

palkerecsenyi force-pushed the deep-links branch from 019f6ec to e6a49db Compare October 30, 2025 13:33

fenekku self-requested a review November 6, 2025 14:04

fenekku reviewed Nov 6, 2025

View reviewed changes

palkerecsenyi force-pushed the deep-links branch from e6a49db to 9e59bbc Compare November 6, 2025 15:48

fenekku approved these changes Nov 6, 2025

View reviewed changes

slint reviewed Nov 7, 2025

View reviewed changes

palkerecsenyi force-pushed the deep-links branch from 9e59bbc to 3af33f8 Compare November 10, 2025 07:45

feat(events): deep links #503

Are you sure you want to change the base?

feat(events): deep links #503

Uh oh!

Conversation

palkerecsenyi commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zzacharo commented Oct 22, 2025

Uh oh!

ntarocco commented Oct 22, 2025

Uh oh!

palkerecsenyi commented Oct 22, 2025

Uh oh!

Uh oh!

Uh oh!

ntarocco left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fenekku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palkerecsenyi commented Oct 21, 2025 •

edited

Loading

ntarocco left a comment •

edited

Loading