Make default rules editable (#26) #59

Canx · 2017-02-19T21:33:08Z

Fixes #26.

I dare to do this pull-request and close the circle...

Resuming, I created "block_xp_default_filters" where I populate static filters. Then, each time you put the block in a course it copies filters from this table. Now the rules editor doesn't differenciate between user filters and default filters.

The next step I had in mind was adding the rule editor in the admin plugin page to edit default rules.

Hope you tell me what you like and what not and what I have to modify to be accepted! I'll apreciate your feedback!

FMCorz

Thanks a lot for your work on this, it's great! You will have seen some of my comments through the code, but I think we need to discuss a bit more before you do more work on this.

Here are a few things I'd like to allow some time to think about:

It may not be clear from the user interface why those default rules are there and what they are for. Especially the odd ones like "assessable_uploaded". Previously the UI showed a brief explanation of the default rules. We could keep the separation by flagging each rule as coming from the 'default'. We could also only copy the rules when the user clicks 'change default rules' or something like that.
At some point it'd be nice to have those default rules editable from the admin, not that it's required now but it'd be nice.
Do we want an admin to be able to lock rules so that they are never editable?
Rather than adding logic here and there in the manager and filter class about default rules, perhaps we can abuse the courseid 0, create other classes, or subclass existing ones. The goal is to keep the code as clear as possible, with less duplication and less mixed purpose.

What do you think?

FMCorz · 2017-02-21T03:56:22Z

block_xp.php

+
+ // Copy default filters to course
+ $filtermanager = new block_xp_filter_manager($manager);
+ $filtermanager->copy_default_filters_to_course();
 return true;


This would add the default rules again, and again, if the block was remove, then re-added. Notice in instance_delete I do not remove anything form the database, I simple update the block to be disabled. That is another improvement we could make at a later stage.

FMCorz · 2017-02-21T04:00:02Z

classes/filter.php

@@ -232,6 +232,23 @@ public function save() {
 }
 }

+ public function save_default() {


Could you add the PHP Documentation

Do you think we need to duplicate so much of the code from save? All of it is identical, except for the rules.

How about abusing the courseid field, where 0 would mean default? So we do not need a new table.

Alternatively, we could keep the separate table if we think more options could be added to the default rules at a later stage, the first thing that comes to mind is the editable flag, which I'd like to keep (see more below).

Yet, another option is to extend the filter class for a filter_default class.

FMCorz · 2017-02-21T04:01:41Z

classes/filter_manager.php

@@ -57,19 +57,6 @@ public function __construct(block_xp_manager $manager) {
 *
 * @return array of fitlers.
 */
- public function get_all_filters() {


Because this method is public, I would prefer you do not remove it (also the PHP doc should have been removed). We could keep maintain the behaviour of this method as it was: "Returning all the filters to apply to the current course", and also keep get_static_filters even though the later wouldn't contain anything by default any more.

FMCorz · 2017-02-21T04:02:06Z

classes/filter_manager.php

@@ -78,7 +65,7 @@ public function get_all_filters() {
 * @return int points.
 */
 public function get_points_for_event(\core\event\base $event) {
- foreach ($this->get_all_filters() as $filter) {


Related to above, I think we can keep this previous method and inject the new default admin filters from there.

FMCorz · 2017-02-21T04:05:47Z

classes/filter_manager.php

@@ -129,7 +116,7 @@ public static function get_static_filters() {
 *
 * @return array Of filter data from the DB, though properties is already json_decoded.
 */
- public function get_user_filters() {
+ public function get_course_filters() {


Again, please do not change the public methods. Some developers may have built things on top of the plugin and if they relied on those their code would break without notice. Unless there is a very good reason, I'd suggest to keep the methods. And if their names need to change, the older method needs to remain and be an alias to the new one, or its behaviour needs to be maintained.

FMCorz · 2017-02-21T04:08:01Z

classes/filter_manager.php

@@ -141,6 +128,28 @@ public function get_user_filters() {
 return $filters;
 }

+ // Used when adding block to course
+ public function copy_default_filters_to_course() {


PHP documentation is missing.

FMCorz · 2017-02-21T04:15:34Z

classes/filter_manager.php

@@ -150,4 +159,12 @@ public function invalidate_filters_cache() {
 $cache = cache::make('block_xp', 'filters');
 $cache->delete('filters_' . $this->manager->get_courseid());
 }
+
+ public static function save_default_filters() {


Missing PHP Doc. Again, perhaps we better have a transparent handling of the filters using the courseid 0. I'm not sure, let's discuss it.

FMCorz · 2017-02-21T04:16:03Z

db/install.php

@@ -0,0 +1,12 @@
+<?php
+
+function xmldb_block_xp_install() {


Could you add the missing PHP Doc for the, and the function?

FMCorz · 2017-02-21T04:16:22Z

db/install.php

+function xmldb_block_xp_install() {
+
+ // Populate block_xp_default_filters table
+ block_xp_filter_manager::save_default_filters();


We surely need to do this on upgrade as well, don't we?

FMCorz · 2017-02-21T04:17:25Z

db/install.xml

@@ -79,5 +79,16 @@
 <INDEX NAME="courseid" UNIQUE="false" FIELDS="courseid"/>
 </INDEXES>
 </TABLE>
+ <TABLE NAME="block_xp_default_filters" COMMENT="Default Filters">


As stated above, maybe we do not want another table. I'm still uncertain at this stage though.

Canx · 2017-02-21T10:09:29Z

Hi Fred, superthanks for your feedback! I feel a little like a bull in a china shop but I hope to learn from you and be more careful next time.

Now my "not too much strong" opinion about your topics you asked:

It may not be clear from the user interface why those default rules are there and what they are for. Especially the odd ones like "assessable_uploaded". Previously the UI showed a brief explanation of the default rules. We could keep the separation by flagging each rule as coming from the 'default'. We could also only copy the rules when the user clicks 'change default rules' or something like that.

I agree that a description should be associated with a group of default rules. Common teachers don't understand the meaning of these rules.

Another problem is that they don't know which rules are applied by default when they install the block. They have to discover the rules page (two clicks distance is a lot for some teachers ;).

I was thinking on a pop-up when you add the block in which the teacher could choose to use or not default rules. There you could add a rule description of how it will behave or start empty and point them to the rule editor page if they want to add or modify the rules.
That could be extended later to select from different groups of default rules (with a description for each group).

About default/user rule separation I prefer your first option: having a mark to know which are default rules and which are user rules. In the second option it's not clear it user rules could be intermixed by priority with default rules.

I wouldn't remove completely though the default rules interface. Maybe could be reused to copy default rules to the course easily if you didn't choose first in the pop-up.

So maybe I agree with the 2 ideas. Preserving non-editable rules in UI but they would only be applied if they are copied to the course rules (with a copy button) or from the start pop-up. Later could be extended to select different group sets of default rules.

I need to draw this:
https://awwapp.com/b/ughvdzwlg/

What do you think?

At some point it'd be nice to have those default rules editable from the admin, not that it's required now but it'd be nice.

Yes, this was also in my mind.

Do we want an admin to be able to lock rules so that they are never editable?

I don't think so, teachers like to play with the rules ;)

Rather than adding logic here and there in the manager and filter class about default rules, perhaps we can abuse the courseid 0, create other classes, or subclass existing ones. The goal is to keep the code as clear as possible, with less duplication and less mixed purpose.

Here you're the expert. I'm starting to read "Learning php design patterns" to catch up! I guess we now have two types of filters (default and course). Default rules haven't context so they can't contain rules that are restrained to an activity, for example. So maybe they should have different validation rules.
Also it's clear that they must be saved differently (with a different table or some column in the table).
I think courseid 0 would be usefull only if there are only one group set of default rules, if in the future are more then it wouldn't be enough.
With my little knowledge subclassing and make abstract filter and two concrete filters (user_filter and default_filter) is the first it comes to my mind but I don't if is the best option.

I'm prone to errors, I'll try to think more on this and have a better opinion in the weekend, now I've got work to do! And still have in the queue reading your code comments... :)

Thanks again!

PD: I think I'm confused with filters/rules terminology in code and I mix them...

Canx · 2017-02-26T09:55:47Z

Ok Fred, I did some of the cleanup you suggested:

refactor save and save_default methods.
leave untouched current public API
add comments and clean them.
add creation and population of table in upgrade.php
but couldn't remove install.php, or new installs wouldn't have the new table.

Also did minor cleanup in the way, like:

extract cache function from get_all_filters().

But didn't have time for more... Some things still pending to discuss/implement:

remove editable flag, or transform it (in a enable/disable flag?)
more filter class refactoring needed now and what kind.
separate database vs add columns vs abuse courseid 0.
UI proposed (similar to the rubric template).

I have no problem in changing things if you think is better, I take the process as a learning experience. Although I can't deny theses features would be great for the next scholar year!

Also I'm slow, low skilled and have little time but I'll try to "commit" to a monthly commit at least.

Canx · 2017-03-04T10:08:39Z

This week could draw this class diagram using umlet to clarify myself:

Still figuring out how to refactor this... I need a little more time!

Canx · 2017-03-07T15:55:06Z

Hi Fred,

I'm in the middle of a refactoring and I wanted to share where I'm trying to get and see if I'm doing it wrong:

In short, I created an abstract class for filter collections (block_xp_filters). Subclasses can load and save filters differently (default filter, static filter, course filter). For saving filters I delegate to block_xp_filter subclasses using the method save() and create_filter() in block_xp_filters (abstract factory?).

I'm in the process of moving more logic from block_xp_filter_manager to block_xp_filters and subclasses, and also moving logic from block_xp_filter to block_xp_course_filter subclass. Tell me if I'm doing this terribly wrong please ;)

PD: After drawing I also noticed that "block_xp_ruleset" follows the composite pattern, nice! It's great starting to understand OOP ;)

Canx · 2017-03-09T13:12:45Z

Rather than adding logic here and there in the manager and filter class about default rules, perhaps we can abuse the courseid 0, create other classes, or subclass existing ones. The goal is to keep the code as clear as possible, with less duplication and less mixed purpose.

Ok, I hope some of this is on the way now. With these new classes it will be easy to remove the "block_xp_default_filters" table to the "zero courseid hack" and come back easily in the future if needed. I understand that a new table is redundant so lets see how much weigh can support block_xp_filters...

Also comes to my mind new oportunities like copying rules from one course to another (between one teacher courses):

$course1_filterset = new block_xp_default_filterset(courseid1);
$course2_filterset = new block_xp_course_filterset(courseid2);

$course2_filterset->import($course1_filterset);

Still pending to clean the code (comments, coding conventions,...), think about editable, more refactoring, modify the UI to distinguish default rules (another color?), and add a button to add default rules. That maybe could mean having a new column to know the filter origin... you don't mind, don't you? ;)

Cheers!

Canx · 2017-03-10T12:26:11Z

After discovering the "/tests" directory (WTF moment) I realized that I should check tests and create new ones before commiting. Learn about testing also would be interesting...

I've read PHPUnit Moodle page and installed it. I tried to pass Moodle tests in my computer and had to cancel after 1 hour, can't believe there are so many tests... Luckily I could pass only levelup tests and see some errors I made, so I'll try to fix them and create some tests for the new classes.

Improving step by step!

… informative

Canx · 2017-03-12T09:31:55Z

Ok Fred,

now current tests pass, I'll think about which tests should be useful next week...

It may not be clear from the user interface why those default rules are there and what they are for. Especially the odd ones like "assessable_uploaded". Previously the UI showed a brief explanation of the default rules. We could keep the separation by flagging each rule as coming from the 'default'. We could also only copy the rules when the user clicks 'change default rules' or something like that.

As a cheap step, I undeleted default rules part in rules.php to keep the explanation and rule structure but now this part is only informative. So you can see static rules as non-editable and the same rules (copied default rules) are showed as editable on the course rules.

Upgrading the module now would mean that current courses would lose default rules so it will be needed to add them to each course on upgrade. That could mean duplicating them, so maybe I can add a "exists" method in filterset, compare ruledata and add the static rule if it's not there...

Ok, when I thought things were going better I discovered the codechecker plugin and a lot of warnings in my code...

And to add more I discovered what cache::make was meant for (at first I thought it was related to cookies ;). So at a later stage I'll think how to add "cache" to the new classes, and then learn about profiling to check improvements. This Moodle learning "experience" seems to never end!

Will keep informing of my progress. I undertand you are busy, so I'll let you know when I'm ready to face another proper code review ;)

to fix...

FMCorz · 2017-03-16T06:32:02Z

Hey, thanks a lot for your work on this. I had a talk with someone else recently who is interested in the same feature. I'll review your code when you deem it ready, else I'm sure we could collaborate to get that done.

Canx · 2017-03-21T11:22:05Z

Hi Fred,

I'm starting to see light at the end of the tunnel, hope to have some decent basic code by the end of the week... I warn you that maybe I changed too much things!

FMCorz · 2017-03-21T12:59:24Z

Hi,

I warn you, in turn, that too much changes won't make it easier to integrate ;).

…ed unnecessary code

FMCorz · 2017-03-23T05:03:30Z

I'm getting worried that you're changing so much that it won't be easy to drag that in. I understand the thrill of rewriting everything, it's a constant struggle I am facing too. But in order to integrate patches the smaller they are, the better.

I have no looked at your patch yet, but if there are many changes with different objectives I might ask you to split those in different issues so that each change can be analyse on its own. The best way to make mistakes is to do too many changes at once.

I'm grateful for the work you're doing here. I just want to make sure that we're on the same page and that we do not end-up in a counter productive situation.

Canx · 2017-03-23T08:59:27Z

Thanks for your advice! After your comment I found an article that supports your point and explains how to do better pull requests, and now I can see that mine is pretty bad. I'll figure out how to make smaller pull requests from this one, even if it takes more time.

I appreciate your feedback, I'm learning a lot through this!

…ilters_cache method to class.

…atabase primitives

FMCorz · 2017-06-28T03:54:05Z

Hi Ruben,

I've started looking at your patch. It definitely is huge. There are many things which change in the way the filters work, maybe for the better as I'm not sure I nailed it the first time, but it'll require some time for me to review this. There are also some PHP7-only features which I need to remove, the plugin supports PHP 5.4, as per Moodle 2.7 requirements. At the same time, I'm working a major refactor of the plugin's internals, so I'll see how (and if) your changes can fit in there.

Nonetheless, I'll be starting working on this feature soon. I still haven't made my mind towards the way to approach this problem. Copying the filters in each new course has its pros, but also cons. I'll have to review the history of our discussion to see I can make up my mind.

Huge thanks for your work on this! It's always easier to code something after seeing how it looks when done a different way. I'm grateful for your time and effort and will do my best to put your work to use.

Thanks!
Fred

FMCorz · 2017-08-08T04:08:17Z

Hi Ruben,

Could you check the latest version and see if it does what you expected?

Thanks!
Fred

Canx · 2017-08-18T16:07:24Z

current version (v3.0) added this functionality.

Canx added 2 commits February 19, 2017 19:21

Now static filters are editable.

2f56bf4

Populate block_xp_default_filters correctly.

20d0e52

FMCorz changed the title ~~Issue 26~~ Make default rules editable (#26) Feb 20, 2017

FMCorz requested changes Feb 21, 2017

View reviewed changes

improving based on review

a33bbcc

Refactoring filters, not finished

f3b01e5

Canx added 2 commits March 8, 2017 16:18

Refactored aiming at filter_manager::course_default_filters_to_course

b174f18

Renamed classes and files to follow ruleset model

32dc4f0

Canx added 3 commits March 9, 2017 16:53

Default filters now are stored using zero courseid in block_xp_filters

c4cb5eb

Merge branch 'issue-27-zerohack' into issue-26

76c8ba4

Renamed files and classes to follow ruleset pattern

f5f0aa5

Canx added 7 commits March 10, 2017 14:16

Fixing first failing test

590a5a3

Undoing bug, don't know why tests are failing...

8edd6cf

Now tests pass

e6559cb

Readded non-editable static rules in rules.php, but now they are only…

1592295

… informative

Added header license in new files

70b547c

Wrong description

374a614

abstract functions should be protected

a22c0ea

Canx added 3 commits March 13, 2017 12:21

Cleaned code style

9600918

Added comment

ae568c8

Improving filter classes and adding tests. Still some failing old tests

cee1259

to fix...

FMCorz added the enhancement label Mar 16, 2017

Canx added 4 commits March 20, 2017 10:50

Refactor load filterset method

7ae003a

use factory from filter class

ae4c262

rules.php refactor and solved errors.

fe57237

Readding types in params (PHP5) and refactoring filter match.

e11d003

Added iterator pattern to filterset

1c1e15c

Canx added 3 commits March 21, 2017 14:31

Iterator->SeekableIterator interface for filterset. Removed and clean…

23555f6

…ed unnecessary code

Simple factories to get more readable code.

547ba24

Added test to check addition of default filters to current courses.

9e3342e

Canx added 14 commits March 23, 2017 11:21

Renamed methods.

b584083

properties should be protected.

149f652

Filtersets now have renderer.

55127a9

forgot to rename function.

716ba0e

Added test to check filter_manager::save_default_filters().

468f6a8

Added admin page to modify default rules.

1176abd

Merged from upstream with latest version.

a7859f5

Added tests to check adding filters to courses and moved invalidate_f…

f2c18bd

…ilters_cache method to class.

thinking of using decorator pattern to cache

2e55685

Only add default filters if they are not still in the course.

9e12b04

Replaced save_default_filters by a raw DB function.

08aad6d

Added generator tool to help write raw DB commands.

97118ff

append_default_rules_to_courses function implemented with low level d…

ac6f77f

…atabase primitives

Cleaned upgradelib.

d6d199e

Canx closed this Aug 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make default rules editable (#26) #59

Make default rules editable (#26) #59

Canx commented Feb 19, 2017

FMCorz left a comment

FMCorz Feb 21, 2017

FMCorz Feb 21, 2017

FMCorz Feb 21, 2017

FMCorz Feb 21, 2017

FMCorz Feb 21, 2017

FMCorz Feb 21, 2017

FMCorz Feb 21, 2017

FMCorz Feb 21, 2017

FMCorz Feb 21, 2017

FMCorz Feb 21, 2017

Canx commented Feb 21, 2017 •

edited

Loading

Canx commented Feb 26, 2017

Canx commented Mar 4, 2017 •

edited

Loading

Canx commented Mar 7, 2017 •

edited

Loading

Canx commented Mar 9, 2017

Canx commented Mar 10, 2017

Canx commented Mar 12, 2017

FMCorz commented Mar 16, 2017

Canx commented Mar 21, 2017

FMCorz commented Mar 21, 2017

FMCorz commented Mar 23, 2017

Canx commented Mar 23, 2017 •

edited

Loading

FMCorz commented Jun 28, 2017 •

edited

Loading

FMCorz commented Aug 8, 2017

Canx commented Aug 18, 2017

Make default rules editable (#26) #59

Make default rules editable (#26) #59

Conversation

Canx commented Feb 19, 2017

FMCorz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Canx commented Feb 21, 2017 • edited Loading

Canx commented Feb 26, 2017

Canx commented Mar 4, 2017 • edited Loading

Canx commented Mar 7, 2017 • edited Loading

Canx commented Mar 9, 2017

Canx commented Mar 10, 2017

Canx commented Mar 12, 2017

FMCorz commented Mar 16, 2017

Canx commented Mar 21, 2017

FMCorz commented Mar 21, 2017

FMCorz commented Mar 23, 2017

Canx commented Mar 23, 2017 • edited Loading

FMCorz commented Jun 28, 2017 • edited Loading

FMCorz commented Aug 8, 2017

Canx commented Aug 18, 2017

Canx commented Feb 21, 2017 •

edited

Loading

Canx commented Mar 4, 2017 •

edited

Loading

Canx commented Mar 7, 2017 •

edited

Loading

Canx commented Mar 23, 2017 •

edited

Loading

FMCorz commented Jun 28, 2017 •

edited

Loading