Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Allow Guests to view Articles #781

Merged
merged 1 commit into from
Aug 12, 2015
Merged

Conversation

codydaig
Copy link
Member

@codydaig codydaig commented Aug 9, 2015

Currently, the API on the server allows anybody (regardless if signed in or not) to retrieve a list of articles. However, the client requires the user to be logged in to view the articles. This makes it so visitors that are not authenticated to view the articles.

This goes along with #780 that makes the TopBar visible by default.

@mleanos
Copy link
Member

mleanos commented Aug 10, 2015

LGTM. I've tested this and it works as expected.

@codydaig As a guest (not logged in)... I did notice that if there aren't any Articles in the list, then the No articles yet... is displayed with a link to articles.create state. I don't necessarily think this is an issue. It could be a way to encourage registration/login. Just wanted to point this out, in case others have a difference of opinion, or it was overlooked.

@lirantal
Copy link
Member

doesn't it also require update on the server side to allow articles to be retrieved?

@mleanos
Copy link
Member

mleanos commented Aug 10, 2015

@lirantal The ACL for the Articles api was already configured to allow Guests to retrieve Articles. This PR is merely matching the front-end behavior to reflect this.

@lirantal
Copy link
Member

Got it. That PR is already merged? :) I may have lost track

@mleanos
Copy link
Member

mleanos commented Aug 10, 2015

@lirantal Yes, I believe the back-end ACL's have been this way for some time. Either way, it's definitely in master now.

@lirantal
Copy link
Member

Ahh ok, thanks Michael.
@codydaig take a look at the comments from @mleanos and I'll merge this in

@lirantal lirantal added this to the 0.4.x milestone Aug 10, 2015
@lirantal lirantal self-assigned this Aug 10, 2015
@rhutchison
Copy link
Contributor

@codydaig @mleanos @lirantal does anybody else feel like the view that we have now is really meant for someone managing the articles, not intended for displaying to users?

The API is configured correctly, and the UI does prevent access to the list of articles, but I feel like this is intentional?

Are we "ok" with exposing the same "admin" type of interface (list) to users? Perhaps this is a good time to create the articles admin (anyone able to manage articles)?

@mleanos
Copy link
Member

mleanos commented Aug 11, 2015

@rhutchison +1 on implementing the Admin feature for Articles.

I can see the need for a separate view for Guests. Would definitely make it more secure and could provide more focus for both views; allowing for more robust feature sets in each.

@lirantal
Copy link
Member

Just to make it clear that the admin module is there as an example module for the framework, so whatever we choose to enhance there on the articles area (which IMO is blessed) should be contained to that area.

@codydaig
Copy link
Member Author

@rhutchison @mleanos Is there anything that I need to change in this PR?

@mleanos
Copy link
Member

mleanos commented Aug 11, 2015

@codydaig I think not. Since this is technically a bug in the front-end routing config.

After this is merged, we will need to discuss the Admin feature for Articles & the separation of the articles list views for Guests, and authenticated users. Each should be it's own PR.

@lirantal
Copy link
Member

Ok great, I'll merge.
Thanks guys.

lirantal added a commit that referenced this pull request Aug 12, 2015
@lirantal lirantal merged commit 0be1b11 into meanjs:master Aug 12, 2015
@codydaig codydaig deleted the bug/listArticlesAuth branch August 20, 2015 22:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants