Article route / controller refactor #800
Conversation
|
Anyone have any comments? |
| function ($scope, $state, $stateParams, Articles) { | ||
| //Check if article is loaded if not load it | ||
| if ($scope.article === undefined || typeof $scope.article !== 'object' || $scope.article._id === undefined || $scope.article._id !== $stateParams.articleId) { | ||
| $scope.loadArticleById($stateParams.articleId); |
There was a problem hiding this comment.
Article should come from route resolve, not from controller.
|
I think it can be simplified with 2 controllers - ArticleListController, ArticleController. Controllers should be separate files. |
|
+1 on using just two controllers, and as separate files. @trainerbill There are a lot of changes revolving around the Articles module, right now. I have an open PR #807 that contradicts this one in a sense. I'm not even sure if my proposed changes are what we want to go with. But I wanted to make sure you were aware of it. |
|
Thanks @mleanos. I think that the 2 PR's can coexist. Just keep yours to only admin functionality. I really think the article module needs to be refactored separately. |
|
Alright gents. I moved the load and authcheck to the $state resolve. Split out into 2 controllers. The only thing I don't like is having a mock article resolve for the create route since you have to resolve article to use the ArticlesController. |
| controller: 'ArticlesController', | ||
| resolve: { | ||
| article: function (Articles, $stateParams, $state) { | ||
| return Articles.get({ |
There was a problem hiding this comment.
just return Articles.get(..).$promise;
the 404 should be handled by the http interceptor (https://github.com/meanjs/mean/pull/796/files#diff-90593c1684669b4a0cc185ec424f0a24R26)
There was a problem hiding this comment.
I agree... but I doubt they will merge this in without the interceptor and I don't know if they like the interceptors.... So we would need both PR's to merge together. Which I am alright with.
|
Can I get a couple LGTM? I want to fix the tests but need to settle on the code first. |
|
LGTM. Haven't tested, but like and agree with the concept. |
| return article; | ||
| }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Wait, how can the backend return something you're not authorised to see? You should get 403 in that case. Or am I missing something?
In any case, this is a lot of logic for route config. Thoughts?
There was a problem hiding this comment.
Because we are not hitting the backend until the actual update of the article. So the article is loaded in scope when it is viewed. Now when you put /edit in the URL you will get to the edit route because we are not hitting the backend. If you tried to update you would get authorization error but that does not prevent you from getting to the route. This will.
| templateUrl: 'modules/articles/client/views/list-articles.client.view.html', | ||
| controller: 'ArticlesListController', | ||
| resolve:{ | ||
| articles: function (Articles, $stateParams, $state) { |
There was a problem hiding this comment.
don't like having the articles list in the resolve, the service should still be a part of the controller.
There was a problem hiding this comment.
Why not? Do we not want the most recent data on the route resolve? Not sure why we would want to call it in the controller after moving everything else to resolve.
There was a problem hiding this comment.
In my opinion, resolve's on a route are better suited for when you're interacting with an instance of a resource/entity. When a view is dependent on specific data actually existing at the time of loading. In the case of lists, it's not critical that there's data when the view loads or even if such data exists.
The concern of the route config (resolve in this case) should be to make sure the view has everything it needs in order to function properly, and for authorization purposes.
| }, | ||
| resolve: { | ||
| article: function () { | ||
| return { }; |
There was a problem hiding this comment.
no space, but on another note, why do we need this?
There was a problem hiding this comment.
Because article controller is now expecting to resolve an article. Its either send a blank object or split it to another controller
There was a problem hiding this comment.
This seems too hacky, I'll try to think of a better way.
|
@ilanbiala This PR is really messed up. Evidently I merged master in a couple times. You care if I close and reopen with just the changes? This commit is killing me on rebase: Looks like I did something pretty dumb on it. |
|
Please do whatever you need to keep it tidy. 👍 |
|
Closing in favor of #874 |
This has been bothering me for awhile. I refactored the Article controller and routes to be more in line with ui-router.
Benefits
Obviously the tests are going to fail. I will fix them once I get buy in and the code is set.
@lirantal @ilanbiala @rhutchison @mleanos @codydaig