Make default rules editable (#26)

[Canx]

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!

[Canx]

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]

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]

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]

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]

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]

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!

[Canx]

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 ;)

[FMCorz]

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]

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]

Hi,

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

[FMCorz]

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]

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!

[FMCorz]

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]

Hi Ruben,

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

Thanks! Fred

[Canx]

current version (v3.0) added this functionality.