[#26445] - [4.0] Remove duplicate items from list and filtering by language in mail templates list display
- Closed
- 3 Oct 2019
- Medium
- Build: 4.0-dev
- # 26445
- Diff
- richard67:4.0-dev-mail-templates-sql-1
- Pending Hound Hound is busy sniffing around... Details
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Summary of Changes
This Pull Request (PR) changes the list query for the mail templates list so that no duplicates are shown anymore in the list. This is done by removing the language column from both the result set and the group by. The languages is later be glued to the result with another query in PHP and so not needed for this query.
Somehow this changes fixes also the editing of mail subject and content, which did not work for me without this PR but works now.
Furthermore, this PR removes filtering by language from the list because it does not make sense.
The reason for this is that while in database there is 1 record for each language + 1 with empty language, which is used for diplay of the common properties of all mail templates of this kind (template_id, which is not unique in the table but unique per language) and uses flags to link to the edit form for the reord for that particular language.
This is not the way how other things work regarding languages, but it makes sense here and is explained in a comment of the PR which added this new feature.
Testing Instructions
Please report back if tested with MySQL (or MariaDB) or PostgreSQL. I've tested both with success of course.
- Install clean 4.0-dev.
- Install some languages and publish their content languages. (optional)
- Go to the mail template list as shown by the red marks in following screenshot.
- Edit the mail template and toggle the body switcher element and enter any random text.
- Do the same with the title.
- Save and close.
- Edit again the same mail template for the same language to see if the changes have been applied.
Result: See section "Actual result" below.
- Apply the patch of this PR and refresh the page.
- Edit the mail template and toggle the body switcher element and enter any random text.
- Do the same with the title.
- Save and close.
- Edit again the same mail template for the same language to see if the changes have been applied.
Result: See section "Expected result" below.
Expected result
There is one record shown with ID "com_config.test_mail", which has for each language (content or site, not sure) 1 flag. By clicking on the flag you can edit the mail template for that particular language.
The filter options don't contain an option for filtering by language, same as the sort options, because that would not make sense in this kind of view for reasons stated in description above.
Changing mail subject and content for a particular language works, i.e. you can see the real text and not only the language string when the edit toggle has been changed, and changes are saved and still there when editing again.
Actual result
There are two (in case if no language installed) or maybe more (in case if some languages installed) records shown with ID "com_config.test_mail", which has for each language (content or site, not sure) 1 flag. By clicking on the flag you can edit the mail template for that particular language.
The filter options contain an option for filtering by language, which does not make sense in this kind of view for reasons stated in description above. In opposite to that, the sort options don't contain an option to sort by language. Both together is inconsistent.
Changing mail subject and content for a particular language doesn't work, i.e. changes are not there when editing again.
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration |
| Title |
|
||||||
maybe i'm reading this quite fast & on phone,
but,
this smells to me like a very bad schema design
nothing personal @richard67 .... but a query on only 1 table that needs a DISTICINT.... talk for themself
i don't remeber if is 2nd or 3rd or even 1st normal form ....
my mystake i've just trying to fix a bug instead to go much more deeper as it was needed
@alikon This list display is simply wrong designed, language filtering doesn't work and doesn't make sense there. That's why I leave this PR like it is. Simplified solution would only make sens if no language filtering. @wilsonge Pleace decide: Remove the language filtering, or reqwite this list display so it makes sense.
AFAIK joomla is multilanguage... or at it least it claim to be ...
that's why i've expressed concern when the original PR has been "merged" without tests...
@alikon Listen: I have not made this wrong design for how this mail template function uses languages. I have made a PR to fix an issue with it so it works like it should at the moment, and I've tested that it works on MySQL and PG. Shall George decide on what to do, I wrote about the alternatives above, but there is nothing wrong on this PR here. And don't request from me to change this PR here to repair all the maldesign in that function: I know how yolub would react when it was your PR and someone would request that. Basta.
don't over react,
i'm just pointing something that it's plain wrong, not about your PR where you are trying to fix something...and i 've fallen on the same issues myself fixing only the GROUP BY syntax , instead of trying to discover the root issue...
but ok calm down
... simply ignore my feedback....
@alikon I agree with you that there is something wrong. What is wrong is that this email template component works differently regarding languages and multilanguage than all other stuff does. So this PR makes a fix so that it works like that, differently, and wrong in my opinion, too, but maybe we are both wrong and all is right for it. But if you and me are right and it is wrong, then it has to be rewritten with another PR.
or simply revert the original pr which has been demonstrated to be simply wrong
sorry to be honest/blunt/toxic
@alikon Regarding DISTINCT ... : https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_content/Model/ArticlesModel.php#L207
Language filtering is definitely not intended for this view. I will remove it and simplify the database query.
| Labels |
Added:
?
|
||
PR is ready for test now. SQL simplified and filtering by language removed.
| Title |
|
||||||
| Title |
|
||||||
The more I think about this component and it’s data structures, the less I think it is bad design. If you customize all mail subjects of all languages, you need to have the original setting somewhere if you want to be able to undo your changes for some language. And the defaults are same for all languages because they are language strings, so it would be silly to save them in extra columns in each language-specific records. And the default language strings can’t be hard coded in PHP because different for each extension. So I think meanwhile it is even clever, it just is different how we do it elsewhere for the reason explained here.
I’ll fix the review results tonight after work (German time zone).
Fairly certain that this language stuff was discussed in the original PR and explained there
@brianteeman Thanks for the hint, i've found it: #22126 (comment).
@Hackwar Since you know the history of the mail templates: Could you check and report back if this PR here is correct? I think filtering by language doesn't make any sense in the list display, but let me know if I'm wrong and how it was intended to work, e.g. show less flags for that 1 item in the list (which is the master item with empty language), or whatever else was intended.
I have tested this item
let me ping @infograf768 i would like to hear his opinion
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26445.
This PR is not correct. You are supposed to filter by language, because not all mail templates are always customised in all languages. By default, a mail template has no language specific implementation.
Thought so ;)
Ok, it has to be fixed in a different way then. Will make new PR in a while when I have time. Thanks for reviewing and clarifications.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-10-03 08:12:19 |
| Closed_By | ⇒ | richard67 |
I have it, new PR is in preparation.
@alikon @wilsonge Please test.