Articles admin feature

[mleanos]

This is a work in progress, but is intended to gather feedback. There has been talk of separating the Articles views between authenticated User's and non-authenticated Guests. This is an attempt to explore going down this road. However, it turned into more of a Public states exploration :)

I know there is competing changes to the Menu' service; #780 . I feel like the shouldRender method has been batted around lately. Once we determine how to proceed with implementing these desired features, I will adjust this PR to reflect the solution to the Menu's service refactoring. I think the difficulty in working with the shouldRender with these types of modifications, again shows the need for refactoring the Menu service.

@rhutchison @codydaig @trainerbill What do y'all think of this approach? Adding the Public route to the Core config felt a little funny. But it could be a decent solution. This would allow us to create Public routes for other modules.

[mleanos]

Okay, so now this PR is more of what it should be. I've scrapped the whole /public routes per the advice from @rhutchison and @codydaig

I've separated the Articles views into two distinct views; one for Admins & one for Users (including non-Authenticated users). With these changes, only User's with the admin role can create & edit Articles. This may be a big change, and need more input from the community. But it's something worth exploring.

I know the test's are failing. I will work on updating the tests to account for these changes, once the direction of this feature is defined more clearly & we're ready to merge.

[rhutchison]

this could just be roles: ['*']

[mleanos]

Added additional tests for Admin functionality when interacting with Articles. Also, removed unnecessary tests for non-Admin users.

Addressed @rhutchison line notes.

[mleanos]

Removed a few files, from this commit, that only had spacing changes.

[vaucouleur]

@mleanos In the frameworks that I worked with, people chose instead to have a single view for both the user and the admin, and instead played with showing/hiding buttons (delete/edit/..) depending on the access rights, and of course, duplicating the checks at the server side to avoid "by-pass". The main reason being: to avoid duplicated code. The default Article module is tiny, hence the code duplication issue might not be obvious using the default meanjs code (and by "code" here, I mean both the html code and the js code), but in a "real" SPA site, with complex screens, code duplication is a maintenance nightmare and should be avoided at all cost. Custom directives (or similar "concepts") can help mitigate the code duplication by factoring out a bit of the code, but typically the problem remain: you want your screens to be defined in a single location.

[trainerbill]

I have not had time to look much into this PR but I would have to agree with @vaucouleur. The only reason we had to completely write views for user admin is because there was nothing to start with. Why not just direct them to the articles views and do some small modifications to the views based on the role?

[mleanos]

@vaucouleur @trainerbill I think the main change introduced in this PR is what we should be focusing on. With these changes, ONLY an admin can create/edit Articles. This stems from a conversation @rhutchison and I had. We interpret the intention of the Articles module to be one that provides Article type functionality, rather than a user Post.

So naturally, if only a user with the admin role can create/edit Articles, there is no need for a User to ever have access to the same view/route that allows edits. Even with this reasoning, we could keep the current implementation by putting checks in the view to show/hide elements. However, once the editing aspects of the view gets more complex, it probably should be moved out on it's own anyways. IMO, we should hide as much implementation from the User as possible. Separating the views accomplishes this.

[rhutchison]

Since I gave direction to @mleanos, I'm a little bias, but I agree with @mleanos 👯

Maybe we just never understood what articles is trying to be, but by it's name, I see it being similar to how it would be implemented in any CMS. We have been open to discussion about what articles in this project really means, and I was hoping that we would all be able to come to a consensus and build it out further.

I don't think it's a great idea to build out a shared view and then have ng-show/hide or ng-if statements everywhere. Eventually it will become unmanageable and not make much sense to have in the same view.

Things we have discussed are active/expiration dates, rich text editing, additional roles like publisher/editor, etc.

If someone wants to create an issue for improvement, we can have a more directed discussion. Additionally, we can discuss in the gitter room

[rhutchison]

@mleanos create/edit can be 1 page - article-form.html (or some naming convention that won't get my hand slapped)

@vaucouleur also, there is some refactoring in #800 which will simplify the article service calls.

[vaucouleur]

It's a very interesting and noble discussion. To me, this is one of those key questions at the heart of software design. Let's consider two extreme cases:

• Case 1: What we have is the classical, MVC example. You have some data that you want to display using different views. One is a pie chart, the other is a table display.
• Case 2: What we have is a complex SPA screen based on a "business object" (article, user, bank account, etc.). We have lots of things going-on in the screen, and what we want is to disable or enable some features -- typically, show/hide a delete button.

Naturally, cases are not always so extreme, we have a continuum. Now, if you consider that you are closer to Case 1, it makes sense to have different views. If you feel that you are closer to Case 2, it makes sense to avoid code duplication and have a single view.

Whatever your choice, what would be nice (and I am not sure whether you had this in mind for the long-run), would be to avoid hard-coding as much as possible the roles+permissions on the client-side. Ideally, to me at least, the permission logic should be centralized in the policy file on the server side, and come back-up to the client-side, and be checked (again), when the calls comes back to the server side (on save/update/etc.), using the same centralized policy file.

A concrete example would the following:

• Say, our articles have a theme: sport, politics, science etc.
• These give rise to "editors" who are admins just for the articles that are in their domain of expertise.
• Hence, we have a role "admin_science", a role "admin_politics", etc.
• When a user GET an article, a function in the policy file for article get invoked that returns the permissions for this user.
• Since our user has an "admin_science" role and the article is about science, he gets the permissions ['delete', 'edit', ...], and we are either routed toward the right screen (Case 1 above), or the features are enabled/disabled in the screen (Case 2 above).
• Conclusion: There is little duplication of the policy logic, little hard-coded roles on the client-side, and routing, and/or ng-show/hide, is based on the permissions for that user and that article.

[ilanbiala]

Maybe it was fine actually, I think I just mixed up a few and so I thought they weren't separated.

[mleanos]

@ilanbiala (or anyone else) Did you have a chance to take it for a test drive?

[trendzetter]

Seems to work. Is this a proposal to remove the user submitted article in favor of admins generating content and users consuming? You could demonstrate both and the use of roles by creating 2 types of cruds (articles & tags?). Of course this would be a more dramatic change.

[mleanos]

@trendzetter Yes. I think it makes sense to restrict the creation/editing/deleting of Articles to Admin User's.

This is a pretty big change, and I don't think introducing "Tags" would make sense at this point.

I'm still not sure how the community feels about this change. I wonder what the typical use case for the Articles module really is. Allowing User's to create/manage Articles seems like a very narrow use case for most applications.

My hunch is that more often than not, our users scrap the Article's feature or drastically change it's behavior. Given this assumption, I think the changes introduced here would be more in line with a standard use case for most applications.

@codydaig @ilanbiala @lirantal Perhaps we should keep this as a lower priority until we're closer to releasing 0.5.0, and then re-evaluate it's relevance at that time. WDYT?

[ilanbiala]

SGTM, but maybe just rebase to avoid further conflicts and possibly redoing the PR from scratch if it gets too far from master?

[coveralls]

Coverage remained the same at 70.615% when pulling c1b7d67b1d171f8cd63c44005de38d5dd873495c on mleanos:feature/articles-admin into 59e6daa on meanjs:master.

[coveralls]

Coverage remained the same at 70.615% when pulling 432e49ace07f3eddf3953ce314dbd78f789a29cf on mleanos:feature/articles-admin into e3cd65f on meanjs:master.

[trendzetter]

@mleanos I think it looks good. The practical functionality can be either, it doesn't really matter that much since none of the functionality should be used as is but rather as a starting point and those examples can be used for either user generated content as well as an application with administrators/editors managing the content.

[lirantal]

@mleanos do you want to kick this in please?

[mleanos]

@lirantal I would like to get this merged. However, I was hoping for more feedback & a few LGTM's since this is a pretty big change on how the Articles module works.

If you're fine with this merging, I'll test once again & review for any changes that might conflict.

[codydaig]

I would like to see this get added!

[lirantal]

@mleanos sure you're welcome to fix, test, and merge it.

[ilanbiala]

@mleanos reviewed before and LGTM, just rebase and it should be fine, but I'll check again after the rebase to make sure.

[mleanos]

@meanjs/contributors Thanks! I'll rebase & resolve the conflicts. These changes I made to the Articles client controller & service are most likely the culprit.

[mleanos]

@meanjs/contributors Rebased & resolved merge conflicts.

@ilanbiala Can you review once more before I merge?

[ilanbiala]

LGTM.

[mleanos]

Big change here. We probably should update our documentation to clearly explain these changes. I'll look at that, but may need someone else to tackle the documentation PR.