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

PartialViews or API #11

Closed
murst opened this issue Jul 13, 2017 · 20 comments
Closed

PartialViews or API #11

murst opened this issue Jul 13, 2017 · 20 comments

Comments

@murst
Copy link

murst commented Jul 13, 2017

I've been really impressed with what you guys have done w/ Blogifier. I spent some time creating my own theme using angular material, and so far so good.

The biggest limitation I'm running into is that angular really shines in single-page apps. However, there's really not a good way to asynchronously request posts for the home page, single page, categories, authors, and search. This could be easily solved by adding PartialView results to controllers in Blogifier.Core. Even if your "standard" and "custom" themes don't support these views, it would allow others to create their own themes to take advantage of them. Another option would be to add api access. There is an API right now, but only for authenticated users - adding an anonymous api (something that preferably returns json or xml) would be an alternative solution.

If you guys are busy, I'd be happy to add either partial views or an API and create a pull request. I'd just like to know if that's something that you'd be interested in adding, and if so, which method you would prefer.

BTW, I only spent a few hours on this, but here's a version of your blog that runs on "stock" angular with angular material. I'd be much better with transitions, but I need a way of loading things async for that. I didn't need to modify anything in Blogifier.Core to make it work.

http://bloodforge.azurewebsites.net/blog

@rxtur
Copy link
Collaborator

rxtur commented Jul 13, 2017

That seems reasonable, if you just do pull request implementing single partial view with minimum functionality I can review it and we discuss if this fits overall design. I think it can be done without adding too much complexity, but if this is too messy I'd rather keep it as a fork with specific Angular feature.

Just keep in mind we update code sometimes almost hourly, so please do pull latest before submitting request.

@murst
Copy link
Author

murst commented Jul 13, 2017

Ok, it took me 2 min to make the change, but 2 hours to figure out how to commit it. I think I figured it out.

Also, let me know if you want me to change the approach. I simply added a new parameter which would result in a PartialView instead of a View to the method in the Controller.

Oh, and it seems Visual Studio changed some spacing in my file. Sorry about that!

@rxtur
Copy link
Collaborator

rxtur commented Jul 14, 2017

Although it looks like a small change, down the road people will have no idea why this code in the controller and will break it without even knowing it, making it a maintenance issue. So I'd rather avoid partial views.

What you can do is add API controllers in the "parent" application. For example, home controller in the sample application (WebApp) can use code like this:

`using Blogifier.Core.Data.Interfaces;

IUnitOfWork _db;

public HomeController(IUnitOfWork db)
{
_db = db;
}`
So you can in your code isolate public API and, if this works fine without changing anything else, we can pull it into Core under /Controllers/Api/Public.

Let me know if you see issues here or have any suggestions.

@murst
Copy link
Author

murst commented Jul 14, 2017

I saw this right after I got my theme using PartialViews.... :)

I'll switch it over to an API sometime this weekend.

@murst
Copy link
Author

murst commented Jul 14, 2017

I changed everything over to an API. I'll do a pull request on the files so you can take a look at them. You can see the end result here: http://bloodforge.azurewebsites.net/blog.

I have a few concerns:

  1. The API returns basically the same data as the standard controller, which means that nearly identical code is in two files now. If you guys decide on including an API, it might make sense to refactor the files so that the logic to get a post or posts by author, etc, all happens in a single place.

  2. The BlogPostDetailModel appears to have looping references. It failed serialization until I altered the following:
    services.AddMvc();
    to:
    services.AddMvc().AddJsonOptions(options => options.SerializerSettings.ReferenceLoopHandling = Newtonsoft.Json.ReferenceLoopHandling.Ignore);
    This code is in the Startup.cs file of my web app.

  3. I'm not too crazy about having this API split amongst several controllers. The purpose is to return posts, and ideally it would all happen from a single "Posts" controller. I tried to follow what was happening for the View controllers, so I ended up with multiple files, but I'm not sure if that's the correct approach. I'm sure that if you guys decide to include an API, you'll have your own opinions on how that should be done, so I'll just leave that decision for you.

@murst
Copy link
Author

murst commented Jul 14, 2017

Actually, instead of polluting your repository w/ my crappy commits, you can see them in my branch:

murst@358c04a

@rxtur
Copy link
Collaborator

rxtur commented Jul 15, 2017

I'll be looking into it when we closer to release, so I don't have to maintain 2 sets of APIs while we keep making changes to controllers.

@murst
Copy link
Author

murst commented Jul 16, 2017

Makes sense. I'll adapt to whatever you guys make when its ready.

In the meantime, I'll maintain my own version in a way that makes sense to me, instead of trying to match the shifting state of the blog controllers. I moved all APIs that get posts (or a single post) into a single controller and trimmed them down a little, so that the most complex model that is returned is the BlogPostModel (stuff like categories, disqus, etc, don't need to be there IMO if we're just requesting the posts - however, I didn't do an IEnumerable because I wanted the Pager).

I now retrieve posts like so:

  • All posts (for home page):
    blogifier/api/public/posts
    blogifier/api/public/posts?page={page}

  • Posts by author:
    blogifier/api/public/posts/author/{author}
    blogifier/api/public/posts/author/{author}?page={page}

  • Posts by category:
    blogifier/api/public/posts/category/{cat}
    blogifier/api/public/posts/category/{cat}?page={page}

  • Search:
    blogifier/api/public/posts/search/{term}
    blogifier/api/public/posts/search/{term}?page={page}

  • Single Post:
    blogifier/api/public/posts/post/{slug}

murst@18b1875

@murst
Copy link
Author

murst commented Jul 17, 2017

I did run into an issue. After creating a few different web services (I ended up with controllers for Posts, Authors, and Categories), I started getting the following error:

a second operation started on this context before a previous operation completed

My services were being accessed pretty much at the same time. It probably also doesn't help that my database is not local.

I'm not very familiar w/ the IUnitOfWork pattern and how its being used here using dependency injection. I'm probably not using it right (although my code seems pretty much identical to the other controller constructors). I was able to fix my problem by changing the IUnitOfWork injection from a singleton to scoped: services.AddScoped<IUnitOfWork, UnitOfWork>(); , although I don't really understand enough about this to know if that is the appropriate solution to the problem, or if I should have accessed the data in a different way.

@rxtur
Copy link
Collaborator

rxtur commented Jul 17, 2017

Usually to get around this issue I would do collection.ToList() which frees DbContext right away , but scoped seems to make sense here:

Scoped lifetime services are created once per request within the scope. It is equivalent to Singleton in the current scope. eg. in MVC it creates 1 instance per each http request but uses the same instance in the other calls within the same web request. (from here).

@rxtur
Copy link
Collaborator

rxtur commented Jul 19, 2017

Actually, about public APIs - I think there should be no duplicates at all. Every time Blogifier needs to use any of "get" in your list internally, it can use public. No need to have separate near identical call in both places. I'll try to pull your public folder and see if it can be used internally.

@murst
Copy link
Author

murst commented Aug 2, 2017

Any update on the public APIs? My theme won't work without a way to get the data asynchronously.

:(

@rxtur
Copy link
Collaborator

rxtur commented Aug 2, 2017

With 1.0 out of the gate, I can focus on this now, no worries. Just didn't want it to block release.

@rxtur
Copy link
Collaborator

rxtur commented Aug 3, 2017

I pushed update with public post APIs included to dev branch. Check this out, you'll need to make some adjustments because some things changed, for example posts by category require author slug etc. In general, I added data service that sits in between repository and controllers and can be reused by private and public APIs, you just need to set "pub" indicator when calling this service to true to have fields sanitized for public view.

@murst
Copy link
Author

murst commented Aug 3, 2017

The only thing I ran into so far was that I got a looping reference error when serializing a BlogPostDetailModel. I'll look into this some more tomorrow and try to figure out what's causing it.

I noticed is that in GetPostBySlug, the BlogPostDetailModel populates a profile twice:

BlogPostDetailModel.Profile
BlogPostDetailModel.BlogPost.Profile

Each of these profiles is populated in a different call to the database. Without changing too much of the code, you could probably change
vm.Profile = _db.Profiles.Single(b => b.Id == vm.BlogPost.ProfileId);
to
vm.Profile = vm.BlogPost.Profile;

Thanks for working on the API! :)

@rxtur
Copy link
Collaborator

rxtur commented Aug 3, 2017

I've added that "ReferenceLoopHandling.Ignore" to fix circular reference between post and profile.

@murst
Copy link
Author

murst commented Aug 3, 2017

I put in a few changes for you to review:

  1. I added support for category listing when in single-blog mode. Passing an empty string for "auth" into DataService.GetPostsByCategory will return all posts in the category, regardless of who the author is. Some changes were required in BlogCategoryModel since certain properties assumed a Profile, and in this case the Profile would be null.

  2. I added ordering to PostRepository.ByCategory. I think this was unintentionally omitted.

  3. I updated a few comments in the public API.

  4. Finally, I renamed the Category method in the public API to "AuthorCategory", and added a "Category" method. The AuthorCategory method is intended for multi-blog scenarios and expects an author slug, while the Category method is intended for single-blog scenarios.

You can preview the changes here:
murst@a1a814c

@rxtur
Copy link
Collaborator

rxtur commented Aug 3, 2017

One minor thing - we not generating documentation from XML comments, so I'm not using them anywhere in the code, at least not intentionally. Just use regular "//" instead.

The nulls in profile can be major, it needs to be tested for different scenarios, not just single blog.

@murst
Copy link
Author

murst commented Aug 3, 2017

Sorry, the comment thing is almost a reflex for me. I'll try using double slashes instead.

The effect of having a null profile will certainly need to be addressed. I looked at the latest 'Custom" theme from github, and while there is a Category page, I can't find a way to get to that page from either the blog homepage or from a post. The existing Category page does require an author slug in the code.

The theme probably needs to be updated to support two Category options. One for single-blog where no profile is expected, and one for Author+Category. The BlogController should also have a method for Category where no author slug is expected.

I recall something like this being in there in the past, but it was removed. There used to be two different methods: Category and AuthorCategory, I think.

EDIT: Anyways, I tried to provide support for both the single and multi-blog options in the Public API... that's why there are two different Category methods in there.

@rxtur
Copy link
Collaborator

rxtur commented Aug 29, 2017

I think it safe to close, if I"m still missing something we can open new issue.

@rxtur rxtur closed this as completed Aug 29, 2017
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

No branches or pull requests

2 participants