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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰Fixed auto redirect for extra data queries on post and page resources #9828

Merged
merged 4 commits into from Dec 3, 2018

Conversation

kirrg001
Copy link
Contributor

closes #9791

  • we only made use of the redirect middleware, who detects if a redirect should happen, for taxonomies (tags, authors)
  • data: page.team will now redirect too
  • data: post.team will now redirect too
  • you can disable the redirect using the longform

@kirrg001 kirrg001 changed the title [WIP] 馃悰Fixed auto redirect for extra data queries on post and page resources 馃悰Fixed auto redirect for extra data queries on post and page resources Aug 30, 2018
@kirrg001
Copy link
Contributor Author

We'll merge this next week. Want to test again.

@gargol want's to look at it and learn it 馃憤

@kirrg001 kirrg001 changed the title 馃悰Fixed auto redirect for extra data queries on post and page resources [WIP] 馃悰Fixed auto redirect for extra data queries on post and page resources Aug 31, 2018
@kirrg001 kirrg001 changed the title [WIP] 馃悰Fixed auto redirect for extra data queries on post and page resources 馃悰Fixed auto redirect for extra data queries on post and page resources Sep 17, 2018
@kirrg001
Copy link
Contributor Author

@gargol Ready to review

@naz
Copy link
Member

naz commented Sep 25, 2018

Added reminder to review this first thing on Monday (October 1st), sorry for a drag 馃

@kirrg001
Copy link
Contributor Author

@gargol 馃槑

@naz
Copy link
Member

naz commented Oct 29, 2018

The fix for redirect works in short-form scenarios e.g: data: page.team . Just have a question about something that I found confusing: specified data: page.ipsum and data: post.ipsum and in both cases, it properly rendered the page with according {{page}} and {{post}} objects but the post with ipsum slug was never marked as a page.

Have noticed another inconsistency when trying to define a custom route through longform like so:

  /about/team/:
    data: 
      name:
        type: read
        resource: pages
        filter: slug:team
    template: 
      - team

gave me:

The following definition "{"type":"read","resource":"page","filter":"slug:team"}" is invalid: page not supported. Please use tags,users,posts.

It makes sense, but maybe we should allow pages as another resource especially in the context of changes that were done in API v2 where the page is treated more like a separate resource with its own controller. Also with corrected the definition above (with posts resource), the redirect stopped working from http://ghost.local/team/ -> http://ghost.local/about/team/.

@kirrg001
Copy link
Contributor Author

specified data: page.ipsum and data: post.ipsum and in both cases, it properly rendered the page with according {{page}} and {{post}} objects but the post with ipsum slug was never marked as a page.

routes:
  /a/:
    data: page.ipsum
  /b/:
    data: post.ipsum

Do you mean this?
A slug is unique, you can't create a post and a page with the same slug.

@kirrg001
Copy link
Contributor Author

It makes sense, but maybe we should allow pages as another resource especially in the context of changes that were done in API v2 where the page is treated more like a separate resource with its own controller

Hmm it is inconsistent yes, but i think it's logical?
Because the long form is the internal format. And a page is under the hood a post.

We could maybe allow this for "routes" directive, BUT for collections you can't define pages. You can't have a collection of pages.

@kirrg001
Copy link
Contributor Author

Also with corrected the definition above (with posts resource), the redirect stopped working from http://ghost.local/team/ -> http://ghost.local/about/team/.

Will look into this 馃憤

closes TryGhost#9791

- we only made use of the redirect middleware, who detects if a redirect should happen, for taxonomies (tags, authors)
- `data: page.team` will now redirect too
- `data: post.team` will now redirect too
- you can disable the redirect using the longform
- /{primary_tag}/{slug}/ as permalink
- /route/: data: post.welcome

- does not work
@kirrg001
Copy link
Contributor Author

Now that v2 exists and we have added /pages, it probably makes sense to make "pages" available as resource.

This will work for v0.1:

/about/team/:
    data: 
      name:
        type: read
        resource: posts
        page: 1
        slug: 'team'
    template: 
      - team

This will work for v2:

routes:
  /about/team/:
    data:
      name:
        type: read
        resource: pages
        slug: 'team'
    template:
      - team

I've committed some smaller changes to make both versions working 馃憤

@kirrg001
Copy link
Contributor Author

@gargol This has priority for Monday.

@naz
Copy link
Member

naz commented Nov 20, 2018

Just some observations for the usages form comment above.

In v1, this route:

/about/team/:
    data: 
      name:
        type: read
        resource: posts
        page: 1
        slug: 'team'
    template: 
      - team

does not respect page:1 filter because it's only allowed to provide id, slug, status, uuid as filter options (v2 works correctly, returnes 404 if resource is pecified as post and renders correctly when resource is page).

The redirect: http://ghost.local/team/ -> http://ghost.local/about/team/ - works 馃憤

@naz
Copy link
Member

naz commented Nov 20, 2018

When creating a long-form route (for v2) like:

    data: 
      customPostHere:
        type: read
        resource: pages
        slug: 'team'
	redirect: true
    template:
      - team

The variable name customPostHere is not respected and page property is being sent as a variable instead.

Also when trying to turn off the redirect in long-form:

  /about/team/:
    data: 
      customPostHere:
        type: read
        resource: pages
        slug: 'team'
	redirect: false
    template:
      - team

it still keeps redirecting http://ghost.local/team/ -> http://ghost.local/about/team/

Copy link
Member

@naz naz left a comment

Choose a reason for hiding this comment

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

Found couple more bugs explained in general PR comments.

@kirrg001
Copy link
Contributor Author

kirrg001 commented Dec 3, 2018

does not respect page:1 filter because it's only allowed to provide id, slug, status, uuid as filter options (v2 works correctly, returnes 404 if resource is pecified as post and renders correctly when resource is page).

This page property is currently not documented. IMO you have to use the filter property?

@kirrg001
Copy link
Contributor Author

kirrg001 commented Dec 3, 2018

it still keeps redirecting http://ghost.local/team/ -> http://ghost.local/about/team/

Can't reproduce. Works for me.

@kirrg001
Copy link
Contributor Author

kirrg001 commented Dec 3, 2018

The variable name customPostHere is not respected and page property is being sent as a variable instead.

Can't reproduce neither.
/about/team/ has access to {{customPostHere}}.

@kirrg001
Copy link
Contributor Author

kirrg001 commented Dec 3, 2018

@gargol Could you pls double check? I want to merge this PR asap 馃憤

@naz
Copy link
Member

naz commented Dec 3, 2018

Confirming I was wrong about both bugs. Don't know how this was happening:

/about/team/ has access to {{customPostHere}}

But the redirect didn't work in Chrome due to caching. When trying it out in incognito everything just worked.

@kirrg001 I think I've exausted the amount of testing and reviews that can be done here 馃槅 please 馃殺 this when ready

@kirrg001 kirrg001 merged commit fc21b25 into TryGhost:master Dec 3, 2018
allouis added a commit that referenced this pull request Dec 4, 2018
* master:
  Version bump to 2.7.0
  Updated Ghost-Admin to 2.7.0
  馃悰 Fixed contributors being able to delete draft posts as co-author (#10239)
  Added configuration controller to v2 API (#10056)
  馃悰 Fixed site changed webhook not triggered for scheduled posts
  馃悰 Fixed invalid imported subscribers count (#10229)
  馃悰Fixed auto redirect for extra data queries on post and page resources (#9828)
  Included relations if static resource is post | page (#10148)
  Renamed API -> Api for v2 auth logic (#10142)
  馃悰Removed user reference warning from importer report if post is a draft (#10169)
  馃悰 Fixed edit permission of the common article by multiple authors (#10214)
  Updated npm keywords (#10217)
allouis added a commit that referenced this pull request Dec 7, 2018
* master:
  Version bump to 2.7.1
  Updated Ghost-Admin to 2.7.1
  Version bump to 2.7.0
  Updated Ghost-Admin to 2.7.0
  馃悰 Fixed contributors being able to delete draft posts as co-author (#10239)
  Added configuration controller to v2 API (#10056)
  馃悰 Fixed site changed webhook not triggered for scheduled posts
  馃悰 Fixed invalid imported subscribers count (#10229)
  馃悰Fixed auto redirect for extra data queries on post and page resources (#9828)
  Included relations if static resource is post | page (#10148)
  Renamed API -> Api for v2 auth logic (#10142)
  馃悰Removed user reference warning from importer report if post is a draft (#10169)
  馃悰 Fixed edit permission of the common article by multiple authors (#10214)
  Updated npm keywords (#10217)
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.

Dynamic Routing: automatically redirect post and page resource
4 participants