Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚧 Implemented plugin based author model in API v2 #10284

Merged
merged 17 commits into from
Jan 3, 2019

Conversation

naz
Copy link
Contributor

@naz naz commented Dec 13, 2018

refs #10124

  • Author model returns only users that have published posts

This is just a first go with Authors API v2 endpoint only. More to come tomorrow once this approach is discussed :)

@naz naz requested a review from kirrg001 December 13, 2018 20:05
@kirrg001 kirrg001 changed the title 🚧 Implemented plugin based author model in API v2 [WIP] 🚧 Implemented plugin based author model in API v2 Dec 14, 2018
@kirrg001
Copy link
Contributor

Code looks good in general 👍

Just leaving some thoughts:

  • i think we have to clarify the sitemap service before shipping any of this change? if we don't return authors without posts, then these authors will 404 for v2, but sitemap service will still show the urls
  • group model representations of existing models in a sub folder?

@naz
Copy link
Contributor Author

naz commented Dec 14, 2018

group model representations of existing models in a subfolder?

This is a good question. For now, there is just one/two models, so maybe too early of an optimization, but definitely would go with something to separate models that just extend existing ones.

@naz naz force-pushed the author-model-separation branch 2 times, most recently from 404a95d to a6740b2 Compare December 14, 2018 15:39
@naz
Copy link
Contributor Author

naz commented Dec 18, 2018

Notes from a discussion we had with @kirrg001 about further approach:

  1. Introduce 2nd config for resources - we will maintain 2 configs apiV01 and apiV2 as there's no better approach to separate logic at the moment
  2. Call 'hasPosts' plugin straight from knex during UrlService initialization
  3. Double check if cold boot happens when theme provided ghost-api value changes (logic similar to releaseResourcesOnly)
  4. Do a UrlService data reset only when the version is changed
  5. Add a comment in the codebase about long-term plan behind resource

@naz naz force-pushed the author-model-separation branch 2 times, most recently from e6026fc to 265910e Compare December 18, 2018 14:14
Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach does not work with the get helper.

e.g. {{#get "tags"}}

It will query the tags controller and not the tags-public controller.

@kirrg001
Copy link
Contributor

kirrg001 commented Jan 3, 2019

It will query the tags controller and not the tags-public controller.

You probably just need to tweak the get helper.
If we solve #10106, it will be easier to handle this.

@kirrg001
Copy link
Contributor

kirrg001 commented Jan 3, 2019

@gargol Could you please rebase against master? Thanks 😃

@naz naz force-pushed the author-model-separation branch from 137c172 to 7c77624 Compare January 3, 2019 12:21
@naz
Copy link
Contributor Author

naz commented Jan 3, 2019

Corrected the {{get "tags"}} helper but still running into a situation when there's no data populated in UrlService after the theme is switched, will need to investigate this problem a bit deeper.

@kirrg001
Copy link
Contributor

kirrg001 commented Jan 3, 2019

Found two more bugs:

1

  • v2 is enabled
  • i have 0 tags listed on the sitemap, because none of my posts have tags
  • now i add a tag to a post
  • sitemaps are not being refreshed
  • same appears for when i remove a tag from a post and the tag is not assigned to any post

I assume this not trivial to solve, because we will receive the url event for the post, but we don't have a trigger that the tag url is now available.

2

If i have a tag which is not assigned to any post, i won't receive the tag via the content API, but i can serve the tag on the site.

To reproduce:

If you do the same with an author. It works for both cases.

@kirrg001
Copy link
Contributor

kirrg001 commented Jan 3, 2019

I will look into (2) now. I will also rebase the branch against master.

@kirrg001 kirrg001 force-pushed the author-model-separation branch from 227b97b to 222b00c Compare January 3, 2019 16:33
@naz naz changed the title [WIP] 🚧 Implemented plugin based author model in API v2 🚧 Implemented plugin based author model in API v2 Jan 3, 2019
@naz
Copy link
Contributor Author

naz commented Jan 3, 2019

Merging it as is for now 🚀 Will follow up with an issue that should address 2 problems discovered by QA

@naz naz merged commit d3f3b3d into TryGhost:master Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants