Add early exit for "fatal" errors like wrong usergroup, not valid yet, not valid anymore, etc. #23
Conversation
| if (!$message) { | ||
| return ''; | ||
| } | ||
| $message = '<li class="promo_error_item">' . $this->_helper->__($message, $params) . '</li>'; |
There was a problem hiding this comment.
Shouldn't this be the default behaviour? Doesn't _formatMessage always generates invalid HTML by putting text directly inside the ul without having a li?
There was a problem hiding this comment.
No, because the nested messages come with container already back. But we have the problem, that we try to translate the already nested stuff. So in theory we should refactor all that, so that we have arrays with the messages, then we translate everything and then we format it with html. But I don't have the right mind currently to do that properly and test it.
There was a problem hiding this comment.
Since we're adding the new <li> elements, can we apply them to the additional messages? I updated the tests that do not have the additional messages to reflect the new elements, but the additional messages are still contained directly within <ul> elements.
Also, would be great to have some tests for the new show-stopper behavior.
|
I have not forgotten, and read the comment - will take care of that! Soon(ish) :-/ |
|
Sorry @lfolco that is now a lot, but I gave my best, fixed a bug, cleaned up a lot, and added at least one test for the show stopper |
|
@sprankhub you can review either here or on the company repo 😅 |
|
Sorry for the delay, it's been a crazy holiday season and then some. Will review this weekend! |
|
These changes don't work for conditions. For example, on the improvements branch:
On the master branch: This is for a rule setup like this:
There are a ton of changes in this branch, too many to figure out what exactly is going on, but over half of the of the tests fail:
Any chance you can break this PR up into smaller PRs that don't change so many things? |
Add early exit for "fatal" errors like wrong usergroup, not valid yet, not valid anymore, etc.
fix broken HTML
fix date format for magemail (hopefully)
Cleanup
Closes #22