[bug 1175123] Initial implementation of Trigger suggester #643
Conversation
|
This is described at: https://wiki.mozilla.org/Firefox/Input/Trigger_suggester There are a few things that aren't done yet that will be done under different bugs. I figured it's worth splitting this out into parts and this is the minimum viable initial part. Outstanding things that will be covered by other bugs:
To review:
I've gone through and done the hand testing of creating rules and then creating feedback that matches and doesn't match rules to make sure that works. If you want to do that, too, that's cool, but don't feel like you have to. We'll push this out and if there are issues we didn't catch in review, we can fix them as we find them. |
|
@mythmon Can you work through this? It's like 900 lines of code, but some of that is boilerplate and similar to the SUMO suggester. Sometime in the next week would be cool. |
| @@ -17,4 +17,4 @@ def get_redirectors(): | |||
| return list(_REDIRECTORS) | |||
|
|
|||
|
|
|||
| from .base import build_redirect_url, Redirector | |||
| from .base import build_redirect_url, Redirector, RedirectParseError | |||
There was a problem hiding this comment.
Is this a "import for export" type of thing? I don't see it in the __all__ definition above.
There was a problem hiding this comment.
Oops. It should be in the __all__.
|
That's all I've got for today. I'll do another quick check tomorrow, and pull it down and run it. This looks good. It seems like the suggestion/redirect provider stuff you did paid off pretty well. |
|
^^^ Fixes based on your comments. |
|
Er, that has fixes based on your comments except for writing up a comma-separated string field. I'll fiddle with that today to see if it's a good value. |
|
^^^ Rewrote the code using ListField (model field) and StringListField (form field). I like it. I think it's easier to work with than the previous iteration. @mythmon Can you look at that? |
| # This takes a unicode and reconsistutes it into Python base | ||
| # types. | ||
| return ast.literal_eval(value) | ||
| except Exception as exc: |
There was a problem hiding this comment.
Can you catch SyntaxError here, instead of all Exceptions?
There was a problem hiding this comment.
I don't know if SyntaxError is the only possible exception that can be thrown here. Regardless, I think I want to treat all exceptions as ValidationErrors, so seems better to keep it like this.
|
^^^ Addresses your last set of comments:
|
|
This looks good. I got the tests working before, and they all passed. The new model and form fields stuff looks well enough tested, and only a little goofy. I didn't test it myself, because I couldn't get Persona to work locally (I should fix that), but I'm sure we can test it in prod. r+ |
|
^^^ That squashed all the commits into one. Travis is green. I'll go through it once more, but I think we're good to go now. |
Implements the Trigger suggester which allows analyzers to set up trigger rules for adding suggestions to feedback that match those rules. Also implements ListField (model field) and StringListField (form field) for storing lists of things in the db.
|
^^^ Removes a function that was never used. I'll wait for Travis then land it. |
[bug 1175123] Initial implementation of Trigger suggester
|
LANDED! Thank you so much for reviewing this! |
Implements the Trigger suggester which allows analyzers to set up
trigger rules for adding suggestions to feedback that match those rules.
r?