Move out Filters from Controllers to a helper / library

[edgarf]

Currently, filter bars are generated in each of the controllers. We want to create a library (or a helper) and put all related code inside.
The controller should have something similar to the following:
$generated_filter_data = $this->filter_helper->generate('students',$fields,$data(optional)
students = Controller
$fields = An array with field labels and titles ($fields = array ('School Year'=>"school_year", "Grade Level =>"grade_level)
$data = In some situations, we would like to have custom value lists for the fields, in that case we will define this information in this array.

The helper would need to have field value generation inside. For example, to get all of the Grade Levels? For these purposes ORM Entities should be used. This would allow us to remove all of the custom SQL code currently written in controllers.

The above is just a suggestion, all other proposals are welcome to be discussed.

Tasks

• Create a library / helper
• Use ORM to generate filter values inside of the library / helper
• Clean-up code which is no longer needed in Controllers (raw sql queries)
• Test and validate that all of the filters are functioning correctly.

[regiscamimura]

I'm reviewing your changes, we still need some improvements:

1. Sometimes, the filters are generated NOT through database queries, but based on the result set. In those cases, you must pass the result set as an argument to the helper function that will generate the filters, first to make sure the results are the same, and if the query changes, we won't need to replicate the change to the query inside the helper, second to avoid running queries twice, for performance reasons.

For example, in "Sections" area, we have a dropdown for School Year. I see that you created a function for "filter_year_listing", and you have a query inside it. But in this case, that query should NOT be executed there, you should pass the result set as a parameter and generate the filter based on that parameter.

So I would like the filter_year_listing to be slightly different, something like this:

function filter_year_listing($result=null, $params=[]) {
     if (is_array($result)) {
           // if $result is provided, check if it is an array. If so, iterate over the array and generate the filter array.
         // $params can be an array with values to determine what entry from the array to be used as a key, and what entry to be used as value/label. For example, $params = ['key'=>'SchoolYear', 'label'=>'SchoolYear']. 
           $listing = [];
           foreach ($result as $row) {
                $listing[$row[$params['key']]] = $row[$params['label']];
           }
     }
}

In this case, it does NOT makes much sense to use the $params, cause we know what entry to use, but lets say that we want to add a filter for year in the future, in another area, with a different result set, and the query that generated the result set uses a different column name, for example, instead of SchoolYear, we have a column named just Year -- because that's the column name in the database, or because we used in alias for that column for some reason. Having the $params parameter would allow us to use that very same function, passing as argument the result set (i.e., the array/object with the rows from the database query), and giving an extra argument where we say which columns to be used as key and value for the array.

We should actually generalize this function name to something like "filter_from_result", and whenever we want a filter that is generated from a result query, we can use that function and pass the proper $params. It's usage would be something like that:

filter_from_result($result, ['key'=>'SchoolYear', 'label'=>'SchoolYear']);
If it's convenient, we can still have a filter_year_listing function, which would in the background just call the filter_from_result passing the proper parameters, so we don't need to bother with passing parameters from the view.

The other thing I want you to give a try is to get the most simply query we have in the filter helper and convert it to Gas ORM. Please investigate it and figure how hard it would be to use it. You might need to create some models too, if they are used in Joins and we don't have the respective model in ORM.

Be aware that Gas can generate models automatically. Look at the docs http://gasorm-doc.taufanaditya.com/, and try to generate a model, IF NEEDED only. I did it before, it speed up things a bit, but it's not as smooth as it should be cause the database is MSSQL and the structure is too complex, so it's likely that if you generate the model automatically, you can copy and paste a lot of code, but still need to create the model manually.

[skoshkarev]

@regiscamimura

I'm seeing that in many cases we use objects as a query result:

$query->result()

I used to deal with arrays:

$query->result_array()

Does it matter for you? What is your recommendation? Should we use a common coding style like "always use arrays" because such details may take effect on other thing (like the function in filter_helper that generates listings from the given result).

[skoshkarev]

@regiscamimura

Another interesting question for you.

Should I pass result object (array) to a view, and generate listing there or do not pass result set to a view and generate listings in controllers (and pass arrays for form_dropdown() function to a view)?

Can we choose a common approach to always follow it?

[regiscamimura]

@skoshkarev While I also prefer to use arrays, Gas ORM uses objects, and many other ORMs are all using objects. We do have a function to convert an object to an array on Gas ORM, but still there seems to be a tendency and preference over the community on using objects. I guess we should follow the flow.

Well, for now, lets use objects by default, as the code is mixed with arrays and objects, but the majority of the code uses objects. But lets not get too strict on that. Whenever we need to use arrays, we can. On the filter helper, when using a result set, just verify if the argument is an array or an object and convert it to an array in the scope of the function, so whatever the argument is, it will be handled fine.

For the view/controller thing, I guess it's best to pass the result object in the view. The view call the filter helper to generate the listing and it can be used inside the form_dropdown().

[skoshkarev]

@regiscamimura

There is another issue related to filter_dynamic_listing() function that we will be using in many cases.

Some of listings require extra manipulation like:

foreach ($data['result'] as $row) 
{ 
    $year = date('Y', strtotime($row->AdministrationDate)); 
    $filter['year'][easol_year($year)] = easol_year($year);  // not possible

   $filter['subject'][$row->Subject] = $row->Subject; // can be used 
}

So that approach is not applied for all situations.

[skoshkarev]

@regiscamimura

The only place where I used new function filter_dynamic_listing was assessments view.
I'm sure it will be useful somewhere else.

[regiscamimura]

@skoshkarev For that situation where we need to use easol_year($year), there are two approaches we can use:

1. In the arguments, we add a parameter for a callback function, like
   filter_dynamic_listing($result, ['key'=>'AdministrationDate', 'label'=>'AdministrationDate', 'format_key'=>'easol_year', 'format_label'=>'easol_year']);
   So we could use the format_key and format_label parameters to know that the values should go through the easol_yearfunction. In this case, we also would need to update the easol_year function to accept a full date (instead of a single int as a parameter), and when it's a full date and not a simple int, convert it to a year before doing its thing.
   We can use other approaches to pass this parameter, but in the end, it would do the same thing. But we could use this instead:
   filter_dynamic_listing($result, ['year'=>'easol_year(AdministrationDate)', 'key'=>'year', 'label'=>'year']);
   So this would cause the creation (or the override) of a $year variable, based on the AdministrationDate passing through the easol_year function, and we would use this variable as key and label.
   Using that approach, note the argument can be anything, so how would the filter_dynamic_listing to know when it's to run the process to create/override variables? We can use regex or use some keyword for that argument, like ['set:year'=>"easol_year(AdministrationDate)"], so when the argument has the keyword for 'set:', we know what to do.
2. Second option is just to create a specific function for that case, like
   filter_year($result, ['key'=>'AdministrationDate', 'label'=>'AdministrationDate']);
   So inside this function, we would first iterate over the $result, and make the 'AdministrationDate' to be replaced with the returning value of easol_year($row['AdministrationDate']), and after doing that on top of the $result array, filter_yearcalls filter_dynamic_listing, with $result having the 'AdministrationDate' override for each row.

What I want you to avoid is to create a specific function that does exactly the same thing that filter_dynamic_listing do, just with some extra manipulation. DRY.

Well, maybe its too late for the feedback, but here it goes. Please look into my comments and make sure you use an approach like 1 or 2, or similar, and did not make a copy of the filter_dynamic_listing with some changes for data manipulation. If your code is more or less what I described above, just put this issue into acceptance testing again.