Rule based tags #1478

K-Phoen · 2015-10-11T16:47:08Z

As described in #1477, here is my PoC for a rule-based automatic tagging.

It rules engine is RulerZ and I created a new tab in the configuration panel to manage the tagging rules. It's a quick draft but at least it allows us to create rules and test them:

It's really a WIP so there are no tests, validation for the rules or help about the DSL used for the rules.

If you like the overall direction of this feature, I'll finish my PR :)

Note: there are a lot of additions but it's mainly the composer.lock file. Also, reading the PR commit-by-commit should be easier.

Updated screenshot:

tcitworld · 2015-10-11T20:11:05Z

Impressive ! Can you fix the Travis Error (it's just a use Doctrine\Common\Collections\ArrayCollection; missing in Entity/Config.php ).

K-Phoen · 2015-10-11T20:29:07Z

Should be better now :)

j0k3r · 2015-10-12T07:08:25Z

composer.json

@@ -54,7 +54,8 @@
        "lexik/form-filter-bundle": "~4.0",
        "j0k3r/graby": "~1.0",
        "friendsofsymfony/user-bundle": "dev-master",
-        "friendsofsymfony/oauth-server-bundle": "^1.4@dev"
+        "friendsofsymfony/oauth-server-bundle": "^1.4@dev",
+        "kphoen/rulerz-bundle": "dev-master"


Can't we use a tagged version instead?

j0k3r · 2015-10-12T07:25:41Z

This a good start! I wasn't aware that this bundle will include all that stuff from Hoa project 😳

Anyway, apart my first little comments:

a better kind of FAQ could be great for novice people, something like "Let assume you want to tag all BLABLA when BLABLA happen, you should put THAT in THIS field", etc ...
don't forget unit test on StringToListTransformer & RuleBasedTagger
did you validate the rule somewhere when adding it? I mean, what if a user set an invalid rule (let's assume it's possible) will it be notified if the rule is wrong?

K-Phoen · 2015-10-14T21:07:59Z

This a good start! I wasn't aware that this bundle will include all that stuff from Hoa project 😳

My bad, I didn't properly introduce RulerZ :)
This library primarily adds a layer on top of Hoa\Ruler allowing us to use rules on objects as well as any other data-source, that's why requiring RulerZ also requires a few libraries from the Hoa Project.
RulerZ also goes a step further than Hoa\Ruler: it really compiles rules instead of having to evaluate on-the-fly an abstract representation of the rule.

a better kind of FAQ could be great for novice people, something like "Let assume you want to tag all BLABLA when BLABLA happen, you should put THAT in THIS field", etc ...

Agreed. I just wrote a list of the usable variables so that you could see the possibilities of the feature. I'll try to write a proper FAQ once the code is done :)

don't forget unit test on StringToListTransformer & RuleBasedTagger

Done for StringToListTransformer, I will write tests for RuleBasedTagger later.

did you validate the rule somewhere when adding it? I mean, what if a user set an invalid rule (let's assume it's possible) will it be notified if the rule is wrong?

It was on my roadmap. I committed a first version of the validator today but it's really a naïve one: it just validates the syntax. For instance, if you use an undefined variable no validation error will be triggered. I'll improve the validator to detect these errors.

nicosomb · 2015-10-15T09:17:09Z

I just gave a try 👍

Do you think that it's possible to have a command to assign these tags to existing articles?

K-Phoen · 2015-10-15T09:20:16Z

I also had this idea in mind, that's why I used RulerZ instead of Hoa\Ruler.

While Hoa\Ruler only works on an object (in our case, an Entry), RulerZ can fetch all entries matching a given rule directly from the database.

So yeah, writing a command/controller to tag existing articles should be easy :)

nicosomb · 2015-10-15T09:22:06Z

Nice.

Don't forget to give availability to remove rule.
I created a new rule and all seems to be broken. My rule if « content like "%basket%" » then tag as « basket » (some doc will be necessary ;-) because I'm sure it's not the good syntax ), and I can't save a new entry.

K-Phoen · 2015-10-15T09:25:12Z

Yep, there is still some work to be done on the validation. Also, I'll make sure that even if the automatic tagging fails it does not prevent the user to add new entries.

K-Phoen · 2015-10-15T09:25:19Z

Thanks for your feedback btw 👍

K-Phoen · 2015-10-31T15:24:08Z

I implemented a better validator for the tagging rules and also provided a way to delete them.
There is also a wallabag:tag:all command to tag all entries for a given user according to its tagging rules.

There is still some documentation about the rules to be written. You wanted that on a separate page? Do you already have one?

j0k3r · 2015-10-31T15:28:40Z

src/Wallabag/CoreBundle/Entity/TaggingRule.php

+use KPhoen\RulerZBundle\Validator\Constraints as RulerZAssert;
+
+/**
+ * Config.


Bad copy/paste?

j0k3r · 2015-10-31T15:41:27Z

Looks good 😄

About the documentation your are talking about, is that the FAQ displayed below the tagging page or something better which need to be in the documentation of wallabag?
Maybe you can write a small FAQ in the tagging page (like I said in a previous comment: Let assume you want to tag all BLABLA when BLABLA happen, you should put THAT in THIS field)

And you can add a better documentation in our doc, which is now part of the repo since #1495. Maybe you'll have to rebase your PR to get that changes.

nicosomb · 2015-11-02T15:44:15Z

Conflict with v2 branch. Will try this PR maybe this evening.

nicosomb · 2015-11-13T10:12:21Z

Something like title matches "basket" would be enough? The matches operator would work like the stripos PHP function, it would return true only if there is an exact match.

Yes 👍

I also tried to reproduce your bug but I couldn't…

Will retry

wallabag:tag:all

Oh nice, I didn't see this feature! What do you think having a button: "tag all the previous entries"?

j0k3r · 2015-11-13T12:33:08Z

What do you think having a button: "tag all the previous entries"?

Depending on the number of entries for the current user, it could take long time to accomplish ..

K-Phoen · 2015-11-13T13:44:43Z

I added the matches operator :)

nicosomb · 2015-11-13T13:49:40Z

<3 (just need documentation about this new operator maybe)

j0k3r · 2015-11-13T13:51:41Z

src/Wallabag/CoreBundle/Operator/Doctrine/Matches.php

+            $pattern = sprintf("'%%%s%%'", substr($pattern, 1, -1));
+        }
+
+        return sprintf('UPPER(%s) LIKE UPPER(%s)', $subject, $pattern);


Is it working on MySQL, PostGres & SQLite ? Maybe a working test for the matches keyword could be a good idea.

K-Phoen · 2015-11-13T13:52:00Z

<3 (just need documentation about this new operator maybe)

Oops, my bad, I forgot :)
I'll add it tonight.

Is it working on MySQL, PostGres & SQLite ? Maybe a working test for the matches keyword could be a good idea.

It does. I'll add a test.

j0k3r · 2015-11-13T13:52:58Z

src/Wallabag/CoreBundle/Operator/PHP/Matches.php

+
+namespace Wallabag\CoreBundle\Operator\PHP;
+
+class Matches


Could you explain what these classes are used for (both Operator\Doctrine\Matches & Operator\Doctrine\PHP) ?
In the context of your PR, we can guess but when it will be inside the project, it won't be that easy to guess

K-Phoen · 2015-11-15T19:48:27Z

PHPdoc added, matches operator described in the FAQ and a little test was written :)

j0k3r · 2015-11-16T06:00:25Z

src/Wallabag/CoreBundle/Tests/Controller/EntryControllerTest.php

+        $entry = $em
+            ->getRepository('WallabagCoreBundle:Entry')
+            ->findOneByUrl($url);
+        $this->assertCount(1, $entry->getTags());


Could you just assert that the only tag for this entry is actually wallabag, as per the TaggingRule.

j0k3r · 2015-11-16T06:01:14Z

I think we are really close to get this merged 🚀
Thanks for the hard work and for taking care of all the feedback we provided!

j0k3r · 2015-11-16T06:10:22Z

src/Wallabag/CoreBundle/Resources/views/themes/material/Config/index.html.twig

+                            </div>
+                        </div>
+                    </div>
+
                    {% if is_granted('ROLE_SUPER_ADMIN') %}
                    <div id="set5" class="col s12">


It might be a side effect of your change (and some rebase) but could you set set6 here instead of set5? Because we have 2 set5 in the page and it breaks the layout because, if user is super admin, Add a user will always be displayed.

K-Phoen · 2015-11-29T15:26:07Z

I found some time to fix the tests, they're green now 🎉
Is there anything left to do before this PR is merged?

Anyway, thanks for all the valuable feedbacks you guys provided throughout this PR. Keep up the good work 👍

tcitworld · 2015-11-29T21:53:08Z

Note for later : Think on how to export rules from one installation to another.

K-Phoen · 2015-11-30T10:54:48Z

Both rules and tags can be represented as plain text so that should not be difficult :)

j0k3r · 2015-12-06T13:31:23Z

@K-Phoen Huge thank to you for that 👍

Rule based tags

Hywan · 2015-12-06T14:49:42Z

👍 \o/

K-Phoen changed the title ~~Rule based tags~~ [WIP] Rule based tags Oct 11, 2015

j0k3r added Improvement Work in progress labels Oct 12, 2015

j0k3r reviewed Oct 12, 2015
View reviewed changes

j0k3r reviewed Oct 31, 2015
View reviewed changes

j0k3r added this to the 2.0.0 milestone Oct 31, 2015

K-Phoen added 9 commits November 11, 2015 16:23

PoC of rule-based tagging

c351062

Add TaggingRule entity

ac9fec6

Add a form to create tagging rules

f19f9f6

Add missing tagging rule repository

9cbb404

Display the tagging rules in the config

981cacf

Add a table explaining the available variables

e9fbd2d

Add missing use statement

1d7b350

Fix ContentProxyTest

f530f7f

Add tests for the StringToListTransformer class

003fa77

Add matches operator

a6e27f7

j0k3r reviewed Nov 13, 2015
View reviewed changes

K-Phoen added 3 commits November 13, 2015 20:50

Add phpdoc for all Matches implementations

f27aca2

Document the matches operator in the FAQ

aeff8aa

Add a quick test

958671a

j0k3r reviewed Nov 16, 2015
View reviewed changes

K-Phoen added 3 commits November 16, 2015 13:34

Assert that the tag has is the good one

69edb77

Clean the tagging rule creation form

c13eda4

Fix tagging rules ordering

752b90d

j0k3r changed the title ~~[WIP] Rule based tags~~ Rule based tags Dec 6, 2015

j0k3r removed the Work in progress label Dec 6, 2015

j0k3r added a commit that referenced this pull request Dec 6, 2015

Merge pull request #1478 from K-Phoen/rule-based-tags

a7f1921

Rule based tags

j0k3r merged commit a7f1921 into wallabag:v2 Dec 6, 2015

K-Phoen mentioned this pull request Sep 13, 2016

Test rules to make sure they are valid K-Phoen/rulerz#50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rule based tags #1478

Rule based tags #1478

K-Phoen commented Oct 11, 2015

tcitworld commented Oct 11, 2015

K-Phoen commented Oct 11, 2015

j0k3r Oct 12, 2015

j0k3r commented Oct 12, 2015

K-Phoen commented Oct 14, 2015

nicosomb commented Oct 15, 2015

K-Phoen commented Oct 15, 2015

nicosomb commented Oct 15, 2015

K-Phoen commented Oct 15, 2015

K-Phoen commented Oct 15, 2015

K-Phoen commented Oct 31, 2015

j0k3r Oct 31, 2015

K-Phoen Oct 31, 2015

j0k3r commented Oct 31, 2015

nicosomb commented Nov 2, 2015

nicosomb commented Nov 13, 2015

j0k3r commented Nov 13, 2015

K-Phoen commented Nov 13, 2015

nicosomb commented Nov 13, 2015

j0k3r Nov 13, 2015

K-Phoen commented Nov 13, 2015

j0k3r Nov 13, 2015

K-Phoen commented Nov 15, 2015

j0k3r Nov 16, 2015

j0k3r commented Nov 16, 2015

j0k3r Nov 16, 2015

K-Phoen commented Nov 29, 2015

tcitworld commented Nov 29, 2015

K-Phoen commented Nov 30, 2015

j0k3r commented Dec 6, 2015

Hywan commented Dec 6, 2015


		namespace Wallabag\CoreBundle\Operator\PHP;

		class Matches

Rule based tags #1478

Rule based tags #1478

Conversation

K-Phoen commented Oct 11, 2015

tcitworld commented Oct 11, 2015

K-Phoen commented Oct 11, 2015

j0k3r Oct 12, 2015

Choose a reason for hiding this comment

j0k3r commented Oct 12, 2015

K-Phoen commented Oct 14, 2015

nicosomb commented Oct 15, 2015

K-Phoen commented Oct 15, 2015

nicosomb commented Oct 15, 2015

K-Phoen commented Oct 15, 2015

K-Phoen commented Oct 15, 2015

K-Phoen commented Oct 31, 2015

j0k3r Oct 31, 2015

Choose a reason for hiding this comment

K-Phoen Oct 31, 2015

Choose a reason for hiding this comment

j0k3r commented Oct 31, 2015

nicosomb commented Nov 2, 2015

nicosomb commented Nov 13, 2015

j0k3r commented Nov 13, 2015

K-Phoen commented Nov 13, 2015

nicosomb commented Nov 13, 2015

j0k3r Nov 13, 2015

Choose a reason for hiding this comment

K-Phoen commented Nov 13, 2015

j0k3r Nov 13, 2015

Choose a reason for hiding this comment

K-Phoen commented Nov 15, 2015

j0k3r Nov 16, 2015

Choose a reason for hiding this comment

j0k3r commented Nov 16, 2015

j0k3r Nov 16, 2015

Choose a reason for hiding this comment

K-Phoen commented Nov 29, 2015

tcitworld commented Nov 29, 2015

K-Phoen commented Nov 30, 2015

j0k3r commented Dec 6, 2015

Hywan commented Dec 6, 2015