Added links API #15446
Added links API #15446
Conversation
|
|
||
| return { | ||
| data: links, | ||
| meta: { |
There was a problem hiding this comment.
Not sure about hardcoding this here, should maybe come from the getLinks method?
There was a problem hiding this comment.
Lets keep it in the endpoint implementation for now. The getLinks method shouldn't really know the endpoint structure I think
Codecov ReportBase: 53.33% // Head: 53.28% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #15446 +/- ##
==========================================
- Coverage 53.33% 53.28% -0.05%
==========================================
Files 1418 1419 +1
Lines 89465 89662 +197
Branches 9650 9656 +6
==========================================
+ Hits 47713 47774 +61
- Misses 40797 40937 +140
+ Partials 955 951 -4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
allouis
force-pushed
the
link-redirects-and-tracking
branch
from
1826ec3
to
8019cf2
Compare
| for (const model of collection.models) { | ||
| result.push(new LinkRedirect({ | ||
| id: model.id, | ||
| from: new URL(model.get('from'), this.#urlUtils.getSiteUrl()), |
There was a problem hiding this comment.
This needs to correctly work with subdirectories, we need to trim the leading / from the from property
| result.push(new LinkRedirect({ | ||
| id: model.id, | ||
| from: new URL(model.get('from'), this.#urlUtils.getSiteUrl()), | ||
| to: new URL(this.#urlUtils.transformReadyToAbsolute(model.get('to'))) |
There was a problem hiding this comment.
transformReadyToAbsolute has moved to the model layer for now because the activity feed also needs to return these. Ideally the activity feed would also use the repository pattern, but with all the relations that would become difficult (because we need click event models, member models, link models and post models) unless we just ignore the whole bookshelf relations and start wiring up the relations manually using repositories. Then do a call to the LinkClickRepository to get the 'events' (and where you need to pass in a repository for members, links and posts), which returns a different kind of object that includes the member, link and posts. Curious to know your opinion on this.
There was a problem hiding this comment.
Ah cool, good to know, thanks!
The plan is to still keep all of the DB code in the models for now, so that everything is in one place for pulling out/improving/replacing in future. I think the repository pattern serves us best in new code so we can easily stub and mock the data layer in development, as well as typing all of our code, we don't need to update the activity feed stuff just yet I don't think :)
SimonBackx
force-pushed
the
link-redirects-and-tracking
branch
from
8019cf2
to
8f70487
Compare
| const postLinks = await this.#postLinkRepository.getAll({ | ||
| filter: options.filter | ||
| }); | ||
|
|
||
| if (postLinks.length === 0) { | ||
| return data; | ||
| } | ||
|
|
||
| const links = await this.#linkRedirectService.getAll({ | ||
| filter: `id:[${postLinks.map(x => x.link_id.toHexString())}]` | ||
| }); | ||
|
|
||
| for (const link of links) { | ||
| const events = await this.#linkClickRepository.getAll({ | ||
| filter: `link_id:[${link.link_id.toHexString()}]` | ||
| }); | ||
|
|
||
| const result = { | ||
| id: link.link_id.toHexString(), | ||
| url: link.to.toString(), | ||
| post_id: postLinks.find((postLink) => { | ||
| return postLink.link_id.equals(link.link_id); | ||
| }).post_id.toHexString(), | ||
| click_events: events | ||
| }; | ||
|
|
||
| data.push(result); | ||
| } |
There was a problem hiding this comment.
IMO this is where the repository pattern is lacking. We have to do this.#linkRedirectService.getAll, but this is returning the same database models in the implementation as this.#postLinkRepository.getAll. And we are fetching all the events of all the links in a loop. I'm not sure why we need to return all the event when we are only going to display the total count.
SimonBackx
force-pushed
the
link-redirects-and-tracking
branch
from
4a0cd9c
to
ad650dd
Compare
|
I moved the in memory repositories to a different PR for now: #15455 |
This expose the /links endpoint on the Admin API, which is filterable by Post ID.
We also add in memory repository implementations, just because they exist locally. I don't think we should necessarily merge them with this PR, but it is what I was building this API against initially.
Missing tests & some small bug fixes