Status Update
No update yet.
Comments
br...@google.com
You are correct that the setInProjectLookup() call is a bit late. This typically only happens after the routing is complete:
If a project is not (yet) known for a change, then the web app makes an index lookup to find out: repo is assumed to be undefined, which causes the actual 404s.
Can you look into why your index is incomplete?
If we have to deal with an incomplete index or want to be generally more robust, then we could make an additional setInProjectLookup() call here:
WDYT?
lu...@gmail.com
f a project is not (yet) known for a change, then the web app makes an index lookup to find out:
https://cs.opensource.google/gerrit/gerrit/gerrit/+/master:polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts;l=3151;drc=aff0e44198952d4f94b924608ff8016573632bbc . Unfortunately in your case the index seem to be incomplete. This request comes back with an empty response:https://eclipse.gerrithub.io/changes/?q=change:204910 . And thus the repo is assumed to be undefined, which causes the actual 404s.
The project has always been int he URL from start, that can be easily fetched with a JavaScript one-liner. Can this be a quick fix for this issue? What do you think?
Can you look into why your index is incomplete?
The index is complete: the change is fully discoverable but NOT with only the change number; imported changes need the project name otherwise won't be discoverable. See more details at
po...@gmail.com
If you recall, when we released the feature in v3.7 we had this problem and it was fixed. Possibly that got lost in the merge up?
I went through the change in 3.7 but I couldn't anything related to this issue. I could check in 3.8 maybe?
@ben: I checked how invasive would be to pass the project as well in the various APIs involved, but it would mean changing all the APIs all the way to _changeBaseURL. It seems to me the right approach, however quite invasive in particular on a stable branch. Not sure if moving to a mapRouteState for the comments could be an alternative less invasive way of fixing the issue. I am not familiar with the frontend code, so I might just talking heresies :P
I will keep looking at the code trying to get a better understanding.
po...@gmail.com
I see these are the available views at the moment:
export enum GerritView {
ADMIN = 'admin',
AGREEMENTS = 'agreements',
CHANGE = 'change',
DASHBOARD = 'dashboard',
DOCUMENTATION_SEARCH = 'documentation-search',
GROUP = 'group',
PLUGIN_SCREEN = 'plugin-screen',
REPO = 'repo',
SEARCH = 'search',
SETTINGS = 'settings',
}
Are we supposed to have a COMMENT view and persist the state the state when we land on a comment route? Similarly to what we do for Route<DashboardViewState>
br...@google.com
Here is a change with the proposal from #2. Please test.
COMMENT route for change numbers with unknown repo
br...@google.com
ap...@google.com
Branch: master
commit de94bbe0d3ba35ae4358b3b5a2786175c548c298
Author: Ben Rohlfs <brohlfs@google.com>
Date: Wed Nov 29 10:41:11 2023
Fix `COMMENT` route for change numbers with unknown repo
The `RestApiService` has a cached map for looking up the repo by
change number. This is usually populated in either the router's
`setState()` method or when change information was fetched from the
Rest API.
The `COMMENT` route is special, because it requires dedicated Rest
API calls for looking up where to redirect the user. This happens
before `setState()` is executed. Thus the repo lookup cache is not
yet populated. There is a fallback of looking up the repo by making
an index query, which was working ok until 3.7 (just making routing
a bit slower than desired), but this fallback won't work, if the
repo was imported and change numbers are not unique across the
entire Gerrit instance.
So the easy fix is to manually populate the repo cache in
`handleCommentRoute()` before making any Rest API calls.
Release-Notes: Fix comment directly link pointing to a change imported from other Gerrit server-ids
Bug:
Change-Id: I2fc29df3f29718b47a019d4a82b6b4abaf52e69f
M polygerrit-ui/app/elements/core/gr-router/gr-router.ts
ap...@google.com
Branch: stable-3.9
commit 156e97835212de4b0e63961e18fdb31ea61c0187
Author: Ben Rohlfs <brohlfs@google.com>
Date: Wed Nov 29 10:41:11 2023
Fix `COMMENT` route for change numbers with unknown repo
The `RestApiService` has a cached map for looking up the repo by
change number. This is usually populated in either the router's
`setState()` method or when change information was fetched from the
Rest API.
The `COMMENT` route is special, because it requires dedicated Rest
API calls for looking up where to redirect the user. This happens
before `setState()` is executed. Thus the repo lookup cache is not
yet populated. There is a fallback of looking up the repo by making
an index query, which was working ok until 3.7 (just making routing
a bit slower than desired), but this fallback won't work, if the
repo was imported and change numbers are not unique across the
entire Gerrit instance.
So the easy fix is to manually populate the repo cache in
`handleCommentRoute()` before making any Rest API calls.
Release-Notes: Fix comment directly link pointing to a change imported from other Gerrit server-ids
Bug:
Change-Id: I2fc29df3f29718b47a019d4a82b6b4abaf52e69f
(cherry picked from commit de94bbe0d3ba35ae4358b3b5a2786175c548c298)
M polygerrit-ui/app/elements/core/gr-router/gr-router.ts
Description
Step to reproduce
http://localhost:8080/c/P1/+/1/comment/f8b5cdb0_0527322b/)Expected result
The page is correctly loaded and it is pointing to the selected comment
Observed result
The PolyGerrit URI renders a pop-up that shows the following content:
This is happening from stable-3.9.
Background
The imported changes are discoverable using any property but not its change number alone, this is by design from v3.7 onwards. The change comment has the project in the URL, so the project is accessible and the change should be fully visible.
The error
undefined~204910on the GUI indicates that the front-end was not able to detect the project in the URL and instead triggered a backend lookup by change number, which isn't supported for imported changes. The front-end should have instead used the project name in the URL, because it was fully available from the beginning of the lifecycle of the page.The comment route in the front-end is not a
mapRouteState, so I wonder if the project is not stored in the state and it leads to the lookup to fail:Every other part of the change is rendered as expected, so it must be something more specific to this particular use-case.