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

[Ember.js] Bug -- Editor/:postId 404 on draft #2857

Closed
novaugust opened this issue Jun 1, 2014 · 7 comments · Fixed by #2892
Closed

[Ember.js] Bug -- Editor/:postId 404 on draft #2857

novaugust opened this issue Jun 1, 2014 · 7 comments · Fixed by #2892
Labels
affects:admin Anything relating to Ghost Admin bug [triage] something behaving unexpectedly
Milestone

Comments

@novaugust
Copy link
Contributor

Issue Summary

Entering the url /ghost/ember/editor/*pid* will 404 when that post is a draft.

Steps to Reproduce

  1. Turn your first post to draft status.
  2. Click on the edit button (so that you're at /ghost/ember/editor/x/
  3. Refresh the page.
    Baby, you just 404'd.

At a glance, this is because ember's using this route: http://localhost:2368/ghost/api/v0.1/posts/ to find posts, without the ?status=all query param.

Technical details:

Ghost Version: master (latest commit: 61dbed0)
OS: Windows 8
OS: Windows 8
Node Version: 0.10.25
Browser: Chrome 35
Database: MySQL

@ErisDS ErisDS added bug labels Jun 1, 2014
@ErisDS ErisDS added this to the 0.4 Ember.js milestone Jun 1, 2014
@ErisDS
Copy link
Member

ErisDS commented Jun 2, 2014

@rjackson - do you have any advice here?

We have the following:

var PostsRoute = AuthenticatedRoute.extend(styleBody, {
 ...
    model: function () {
        return this.store.filter('post', paginationSettings, function () {
            return true;
        });
    },
...

Where paginationSettings is all the additional query params that have to be passed to the API to be sure the request gets all the right posts.

Then we have:

var PostsPostRoute =  Ember.Route.extend({
    model: function (params) {
        var post = this.modelFor('posts').findBy('id', params.post_id);
...

Which says to use the model for the parent route, but it loses all the query params. I am not sure what the proper way to do this would be?

@jgable
Copy link
Contributor

jgable commented Jun 2, 2014

I noticed the new version of Ember out from last week had some meta data related changes, maybe @igorT has some input on this?

For reference: the pagination stuff in routes/posts.js

@ErisDS
Copy link
Member

ErisDS commented Jun 2, 2014

Ah ... I think it's the EditorRoute that's wrong, not the PostsPostRoute we need to find a post by both ID and with a query param - I think we have to create our own findQuery: emberjs/data#1326 @wycats?

I'm finding the existing setup where they both use the same controller kinda hard to follow. Suggestions for improving this please?

@rwjblue
Copy link
Contributor

rwjblue commented Jun 3, 2014

Here is some pseudo code (written on my mobile) that should be fairly close to working for this:

diff --git a/core/client/routes/editor.js b/core/client/routes/editor.js
index fa33e62..8335328 100644
--- a/core/client/routes/editor.js
+++ b/core/client/routes/editor.js
@@ -5,7 +5,24 @@ var EditorRoute = AuthenticatedRoute.extend(styleBody, {
     classNames: ['editor'],
     controllerName: 'posts.post',
     model: function (params) {
-        return this.store.find('post', params.post_id);
+        var self = this;
+
+        function checkPost(post) {
+          if (post.get('id') === params.post_id) {
+            return true;
+          }
+        }
+
+        return this.store.filter('post', checkPost)
+            .then(function(records) {
+              var post = records.get('firstObject');
+              if (post) { return post; }
+
+              return self.store.filter('post', {status: all}, checkPost)
+                  .then(function(records) {
+                      return records.get('firstObject');
+                  });
+            });
     }
 });

The idea is basically, that we check to see if the post exists in the current store (via store.filter without params), return it if found, but if not found do another filter passing the proper query params.

I don't have time at the moment to test and PR, so if someone wants to take this gist and run with it, that would absolutely fine with me.

@darvelo
Copy link
Contributor

darvelo commented Jun 4, 2014

Edit: Actually I don't think any of our code works if you refresh when the post doesn't belong to the first page. Seems we need a way to hit posts/:id with { status: 'all' } .. looking for an Embery way to do that, unless the API can be changed? Perhaps put an else here like:

if (!(options.context && options.context.user)) {
    data.status = 'published';
} else {
    data.status = data.status || 'all';
}

You might be able to do a synchronous check on the store with getById before fetching from the server.

var EditorRoute = AuthenticatedRoute.extend(styleBody, {
     classNames: ['editor'],
     controllerName: 'posts.post',
     model: function (params) {
        var post = this.store.getById('post', params.post_id);
        if (post) { return post; }

        return this.store.filter('post', { status: 'all' }, function (post) {
            return post.get('id') === params.post_id;
        }).then(function (records) {
            return records.get('firstObject');
        });
     }
 });

I've got a PR for this waiting on Travis.

@rwjblue
Copy link
Contributor

rwjblue commented Jun 4, 2014

@appleYaks - 👍 That looks good (the last snippet).

@darvelo
Copy link
Contributor

darvelo commented Jun 4, 2014

Okay, I got a little carried away with this... please see #2883.

darvelo added a commit to darvelo/Ghost that referenced this issue Jun 5, 2014
closes TryGhost#2857
- in EditorRoute, get model from the datastore if it's there
- if page is refreshed, get model from `/posts` API with `{status: 'all'}`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:admin Anything relating to Ghost Admin bug [triage] something behaving unexpectedly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants