My CRM is online now, hoping for some feedback

@cawex

Good job, I looked at the code and it looks good, I only have couple of observations..

First one is that I noticed you're flashing a message to the user on most actions, but some actions were missing this feedback. For example when deleting clients.

Second one is that your controllers seem to be housing a lot of the code.

@SaeedPrez Thanks you!

Yeah someone told me I should consider extracting out the database logic into repository/interface pattern from my controllers, so that is on the to-do.

I downloaded, will try to test it out over the next couple of days.

Thank you @jlrdw, I really appreciate it, look forward to hearing what you have to say :)

I've only looked through the code briefly but I'd have to agree with @SaeedPrez's comments about database code in your controllers.

It might also be worth while refactoring the return statements on your create methods to use a named route - I've made the mistake of using URL paths before and when we've changed routing later down the line it was a nightmare!

Current:

return redirect()->to("/tasks/{$task->id}");

Recommend:

return redirect()->route("tasks.show", $task->id);

I'll look some more over the next few days but congratulations. Getting to v1 is always hardest! :D

Congratulations on this new CRM! Installed it and have been playing with it for some time. Looks nice and the code is very readable although it lacks documentation.

Please have a look into this error

ErrorException in 861d59c23e700405511312aaeccabd58e940c57b.php line 60:
Illegal string offset 'owners' (View: D:\Laragon\flarepoint\resources\views\clients\create.blade.php)

that occurs when trying to "Get client info" in the "Create Client" form.

Hope this helps, Waltz

In your document model:

$this->belingsTo('clients', 'fk_client_id');

code can be improved

Code should have doc blocks....

Congratulations on your first project.

I saw in the Clients controller you are using cURL. I'd be tempted to use one of the cURL libraries that are available, or Guzzle.

I'd also move the code from Pages.index that gets the data for the Dashboard view into a view composer. As others have mentioned about accessing the database directly, I'd also change those to use query scopes so you could have something like; $tasks = Tasks::ThisMonth()->get();

Wow , first of all, thanks for all the response, I will definitely take everything into consideration.

@andrewclark : Thank you, and yes that makes sense, i will look into it. Look forward to hearing what else you have to say :)

@waltz I do believe that's because it's an invalid VAT, this currently only works with danish VAT numbers. My plan is to make it so you can specific what country you are in, and functions like this get VAT info will not be shown, as it's not currently available other than DK, if you do wanna test it you can try a VAT number like: "36361573". Also Thanks! :) I will try at do some better documentation.

@RonB1985 I don't follow sorry, care to elaborate?

@david001 A bit vague, any specific examples, or more constructive feedback ? :)

@zachleigh I will look into it.

@ZetecVan I Thank you, I tried to do it with Guzzle, but kept running into troubles, but I might give it another go. not sure what you mean exactly by moving it to a view composer, could you explain a bit? and that's probably a smart thing to do with the query scopes, I definitely put it on the to-do.

I thank you all for the feedback.

@cawex, sorry, just a typo:

belingsTo instead of belongsTo

@cawex A view composer is some code that runs every time a particular view or partial is rendered. They are generally used for views or partials that appear across multiple pages of an app. Eg, menus, the logged in user view (that appears in the top right) etc.

You can read more about them here: https://laravel.com/docs/5.2/views#view-composers

So for example, in my app/Http/ViewComposers folder (you may have to create this) I have a CategoriesComposer.php. This contains:

<?php
namespace App\Http\ViewComposers;
use App\Category;
use Illuminate\Contracts\View\View;
use Cache;
class CategoriesComposer
{
    /**
     * Bind data to the view.
     *
     * @param  View  $view
     * @return void
     */
    public function compose(View $view)
    {
        $categories = Cache::remember('categories', 22*60, function() {
            return Category::orderBy('name')->get();
        });
        $view->with('categories', $categories);
    }
}

This will return the results of the query in $categories to the view.

In app/Providers/ComposerServiceProvider.php I have

<?php
namespace App\Providers;
use Illuminate\Support\ServiceProvider;
use View;
class ComposerServiceProvider extends ServiceProvider
{
    /**
     * Register bindings in the container.
     *
     * @return void
     */
    public function boot()
    {
        View::composer('widgets.categories', 'App\Http\ViewComposers\CategoriesComposer');
        View::composer('widgets.categories_vue', 'App\Http\ViewComposers\CategoriesComposer');
        View::composer('widgets.recent_posts', 'App\Http\ViewComposers\RecentPostComposer');
        View::composer('widgets.disqus', 'App\Http\ViewComposers\DisqusComposer');
        View::composer('widgets.google_analytics', 'App\Http\ViewComposers\GoogleAnalyticsComposer');
        View::composer('widgets.post_bottom_scripts', 'App\Http\ViewComposers\PostBottomScriptsComposer');
    }
    /**
     * Register.
     *
     * @return void
     */
    public function register()
    {
        //
    }
}

This is says 'when you render this view, call this view composer'. So I have a widget that shows the categories. When that widgets.categories view is rendered it calls the method listed.

The method on the controller doesn't have to worry about $categories as the view composer is getting that.

Finally, in order for the composers to be recognised by your app, you need this in the providers array in app.php

'providers' => [
     
       'App\Providers\ComposerServiceProvider',
        
],

So you could move this out:

  $alltasks = Tasks::all()->count();
        $allCompletedTasks = Tasks::where('status', 2)->count();
        if (!$alltasks || !$allCompletedTasks) {
            $totalPercentageTasks = 0;
        } else {
            $totalPercentageTasks =  $allCompletedTasks / $alltasks * 100;
        }

to say a tasks service provider that populates 'alltasks', 'allCompletedTasks' and 'totalPercentageTasks', when a 'tasks' partial is rendered, and do the same for other similar things. That means you can then use the tasks partial on many pages if you wish, without having to re-do the SQLs.

Hope that makes sense.

@RonB1985 Ah I see, Thanks :)

@ZetecVan Ah that sounds really smart! it makes sort of sense, but i think I will understand it better when I try to do it myself. I will give it a go, and see if I can get it to work! Thanks for the advice.

Off topic:

@andrewclark Why should we use named routes? Just wondering :).

Checked the Danish VAT number - 36361573. This seems to work. Again, great job to build version 1.0.

Also have a look at this one: https://codecanyon.net/item/lcrm-next-generation-crm-web-application/14506615

Perhaps it will give you some additional ideas...

Waltz

@dees040

I do believe it's because, in case you change URL, it will still link to or in case you change the controller method names, you won't have to edit anything besides, the route as long as you keep the routes name as well ofc. (I do believe that is that main reason, but maybe someone else has a better explanation.)

@waltz

Thanks again :) , i will give it a look, and see if I can find some new features to implement.

@ZetecVan

Quick question, would I use code like the one you mentioned in a composer view when it's only used once(in the dashboard, and nowhere else?)

  $alltasks = Tasks::all()->count();
        $allCompletedTasks = Tasks::where('status', 2)->count();
        if (!$alltasks || !$allCompletedTasks) {
            $totalPercentageTasks = 0;
        } else {
            $totalPercentageTasks =  $allCompletedTasks / $alltasks * 100;
        }

The code here is only used once throughout the entire page, and that is for the dashboard. Is the view composer not more for the code that is repeated multiple times?

@dees040 - Just from personal experience I'd recommend it.

I worked on a rather large application once and then once it was "finished" we sat down with the product we decided to change some URL segments for various areas. It was a nightmare finding all of the URLs in the code base and changing them, so as I went I refactored them all to named routes.

It's just a case of being prepared for the unexpected more than a performance or cleaner code issue.

@cawex I guess ultimately it would be personal preference, but if it were me, I would have a dashboard that is built of various partials, and each partial gets its data from a view composer. That way, your dashboard method would go from:

public function dashboard()
    {
        $settings = Settings::findOrFail(1);
        $companyname = $settings->company;
        $users = User::all();
        
        $totalClients = Client::all()->count();
        $completedTasksToday = Tasks::whereRaw(
            'date(updated_at) = ?',
            [Carbon::now()->format('Y-m-d')]
        )->where('status', 2)->count();
        $createdTasksToday = Tasks::whereRaw(
            'date(created_at) = ?',
            [Carbon::now()->format('Y-m-d')]
        )->count();
        $completedLeadsToday = Leads::whereRaw(
            'date(updated_at) = ?',
            [Carbon::now()->format('Y-m-d')]
        )->where('status', 2)->count();
        $createdLeadsToday = Leads::whereRaw(
            'date(created_at) = ?',
            [Carbon::now()->format('Y-m-d')]
        )->count();
        $createdTasksMonthly = DB::table('tasks')
                     ->select(DB::raw('count(*) as month, created_at'))
                     ->groupBy(DB::raw('YEAR(created_at), MONTH(created_at)'))
                     ->get();
        $completedTasksMonthly = DB::table('tasks')
                     ->select(DB::raw('count(*) as month, updated_at'))
                     ->where('status', 2)
                     ->groupBy(DB::raw('YEAR(updated_at), MONTH(updated_at)'))
                     ->get();
        $completedLeadsMonthly = DB::table('leads')
             ->select(DB::raw('count(*) as month, updated_at'))
             ->where('status', 2)
             ->groupBy(DB::raw('YEAR(updated_at), MONTH(updated_at)'))
             ->get();
        $createdLeadsMonthly = DB::table('leads')
         ->select(DB::raw('count(*) as month, created_at'))
         ->groupBy(DB::raw('YEAR(created_at), MONTH(created_at)'))
         ->get();
        $taskCompletedThisMonth = DB::table('tasks')
                     ->select(DB::raw('count(*) as total, updated_at'))
                     ->where('status', 2)
                     ->whereBetween('updated_at', array(Carbon::now()->startOfMonth(), Carbon::now()))->get();
        $leadCompletedThisMonth = DB::table('leads')
                     ->select(DB::raw('count(*) as total, updated_at'))
                     ->where('status', 2)
                     ->whereBetween('updated_at', array(Carbon::now()->startOfMonth(), Carbon::now()))->get();
      
        $totalTimeSpent = DB::table('tasks_time')
         ->select(DB::raw('SUM(time)'))
         ->get();
        $alltasks = Tasks::all()->count();
        $allCompletedTasks = Tasks::where('status', 2)->count();
        if (!$alltasks || !$allCompletedTasks) {
            $totalPercentageTasks = 0;
        } else {
            $totalPercentageTasks =  $allCompletedTasks / $alltasks * 100;
        }
        $allleads = Leads::all()->count();
        $allCompletedLeads = Leads::where('status', 2)->count();
        if (!$allleads || !$allCompletedLeads) {
            $totalPercentageLeads = 0;
        } else {
            $totalPercentageLeads =   $allCompletedLeads / $allleads * 100;
        }
       
        return view('pages.dashboard', compact(
            'completedTasksToday',
            'completedLeadsToday',
            'createdTasksToday',
            'createdLeadsToday',
            'createdTasksMonthly',
            'completedTasksMonthly',
            'completedLeadsMonthly',
            'createdLeadsMonthly',
            'taskCompletedThisMonth',
            'leadCompletedThisMonth',
            'totalTimeSpent',
            'totalClients',
            'users',
            'companyname',
            'alltasks',
            'allCompletedTasks',
            'totalPercentageTasks',
            'allleads',
            'allCompletedLeads',
            'totalPercentageLeads'
        ));
    }

to

public function dashboard()
 {      
        return view('pages.dashboard');
 }

Because you could also extract out

      $settings = Settings::findOrFail(1);
      $companyname = $settings->company;

to a view composer. This would probably be used in more than one place.

Congratulations on Flarepoint! I'm working on a CRM as well and I know this is a HUGE task. I downloaded the project and took a look and I have few comments from my experience so far:

1 You will end up with lots of duplicate data in the database that could be better fixed with separate tables. What if you have multiple clients at the same company? With the same address? It would be a good plan to separate out the companies and maybe even addresses and associate them with a relationship table. Things that are the same such as states, countries, cities even, could be made into their own table to better organize the database. 2 I would suggest adding a 'notes' field to everything in the system. We started having to use our system while it is in development (DO NOT RECOMMEND THAT BY THE WAY! :) And, there will be times when you will want to add a note to a user, a ticket, client, etc. 3 It seems weird to me that errors appear at the bottom of the page and not at the top or in a flash message. Again, this might be temporary but otherwise I'd suggest move it to the top 4 Another might be temporary thing, for a CRM where you will be in and out of sections alot, annoying that the sidebar accordion thing closes after every click. There is no hover state and it doesn't stay opened which means lots of duplicate clicks 5 The first time I made a new client, I received a javascript popup error and got logged out of the system. When I logged back in the client was there and adding new clients had no issue. Not sure if that is an issue on the first client and don't want to reinstall just to check cause I'm too lazy 6 I love how easy it is to add time, but didn't see a way (yet) to see all the time entries

It's interesting to see how someone else is doing things. One of the problems I'm having is I'm trying to do a lot of database commands via eloquent/laravel methods while you are putting the raw commands into the controllers. Nothing wrong with that at all, perhaps if I'd done it that way things would have progressed faster, haha! Anyway, I think it looks good and will be interesting to see it completed.

7 And as an aside... You gotta own your system! The title still says "Laravel CRM" :) That's like the first thing you gotta change!

@laradonk Oh wow thank you, that's some feedback :) I really appreciate it.

1. I might do this later, that's gonna require a lot of work and a lot of "remodeling" on the entire CRM, but thanks for the suggestion, and it does indeed make sense.

2. I am gonna insert a notes field on the client's just have to figure out where to place it :), the tasks and leads have the comment's though, so I don't think it's gonna be a problem there.

3. This is true, I wanted to make it so the error appears under each input, it is on the to-do just have to figure out how to do it easiest.

4. THIS right here, have bothered me so much, I've spent hours trying to figure out how to keep it "folded-out" on the current tab you are in, but was without luck, I spent so long trying to do this :P. Might give it another shot at some point, if you have any guidance to it do tell, please.

5. Hmm sounds, weird not something I have experienced myself, do you remember what the JS error said? 3

6. there is CURRENTLY no way to see all time entries atm, only the total amount of hours inserted into the different tasks/leads, but is is definitely something I will see implemented soon.

7. Haha good point, don't know how I missed that one.

I am currently in work of inserting everything into repositories(That's a lot of work) but that is what I am doing atm, as it's been recommended multiple times now, so at the next push, the controllers shouldn't be so "messy".

Thank you for the feedback I do really appreciate all of it!

@ZetecVan I see that makes sense, I will definitely do this after the repositories "clean-up", it makes a lot of sense, and makes a lot of sense, I will probably have a few questions on the way :)

@cawex No problem. Just @ me any questions when you do the refactoring.

@SaeedPrez and @andrewclark, so I've just made some changes and put everything into repositories/interfaces, does it look correct for you guys now? Would love some feedback on what I've just pushed to Github.

Why repositories? What value did you and your users gain from that?

@zachleigh, well I have, to be honest, probably nothing as of right now, but I do believe it's gonna be easier to work with later on(or if done from the start) when implementing new features etc., It's been recommended to me, that I work with repositories. What would you have recommend?

Having repositories is fine I guess, but all the interfaces are a waste of time in my opinion.

@zachleigh a bit vague I have to say :) do you care to elaborate, as I'm just trying to learn "the best practices".

Well, having repositories can be a good thing because it keeps all your queries in one place. This can be especially important if you have massive, complex queries. I had a brief look at your app and your queries are all pretty standard (delete, create etc.) so I dont know how much youre gaining from having these one line, simple queries be in a more difficult place to find.

The reason behind all the interfaces is that if, in some distant future, your app outgrows Eloquent, you can easily swap it out for something else by simply making a new implementation of the interface for the new database. However, the odds that your app outgrows Eloquent are immensely small and even if you did change out Eloquent for something else, moving your queries over to a new database interface is going to be the least of your concerns.

So, I follow these very general rules about repositories:

• If I am making a crud app which has simple queries, I do not use repositories. I might extract some of the queries out to a trait or something, but a repository is overkill for this situation, in my opinion.
• If I am making an app which is going to have large, complex queries, I make repositories, but without the interfaces.
• If I am bored and want to waste a full day creating a whole crap load of interfaces for no reason, I take a bike ride.