? ? Failure

User tests: Successful: Unsuccessful:

avatar nikosdion
nikosdion
25 Aug 2014

Please review the discussion in issue #4141 and reply here.

This PR implements an extension selection drop-down field in the post-installation messages view.

Moreover, this PR fixes the issues with the wrong extension ID being used when you are resetting the post-installation messages.

Finally, it adds a model method to add post-installation messages in the database. The upside of using it instead of directly inserting entries in the database is that it gives you intelligible error messages if you screw up :)

Close gh-4141 (hopefully this will close the other issue once we merge this PR)

@Bakual @n3t Please review this PR since you were involved in the original issue. Thanks!

avatar nikosdion nikosdion - open - 25 Aug 2014
avatar jissues-bot jissues-bot - change - 25 Aug 2014
Status Pending New
Labels Added: ?
avatar n3t
n3t - comment - 26 Aug 2014

I tested the irst three patches, and for 3rd party components this seems to work as expected. I found only one small issue, if com_postinstall is opened for 3rd party component, clicking on Components menu link will still show messages for this component, however the title of the page changes to "Post-installation Messages for Joomla!".

Anyway, from my view, expected behavior would be, if clicking tho components menu link, reset eid to Joomla core.

avatar nikosdion
nikosdion - comment - 27 Aug 2014

@n3t This is the expected behaviour, that's why we added the drop-down. @Bakual specifically said that if the drop-down is implemented we don't need to change that menu item to open eid 700 (Joomla!) messages.

avatar Bakual
Bakual - comment - 27 Aug 2014

I think what he meant is that the title changes fine to show "Post-installation Messages for com_foo" when changing using the select. But when you come back using the menu item, the title says "Post-installation Messages for Joomla!", but it still has the old filter applied.

Imho it's fine that the filter is still applied. That's what I personally would expect. But the title should of course match the selection.

The PR works fine from what I tested. I didn't test the new model method yet, however from glancing over the code it looks fine.

If it's possible, it would be great to have the extension name translated in the title and dropdown. Seeing "com_foo" as the title and selection isn't that nice.

Also I noted that apparently Seblod does enter a postinstall message upon installation ("welcome") and uninstallation ("has been uninstalled by admin xy"). It means that the dropdown now shows only a number (because the actual extension isn't there anymore) and we have no way of deleting those no longer needed messages. We can still hide them, but the number in the dropdown will always be there. There may of course be other extensions as well which do not remove their messages upon uninstallation.
I think it may be nice to have a way to actually delete messages instead of just hiding them. Or some sort of "cleanup" button to get rid of messages for etensions no longer installed. Not sure what would be best.
It's not really in the topic of this PR. Just a side effect I noticed which can be improved in another PR :smile:

avatar Bakual
Bakual - comment - 27 Aug 2014

Also, you have a few codestyle issues. See the Travis results for the details.

avatar nikosdion
nikosdion - comment - 27 Aug 2014

@Bakual Fixed the title and code style.

Regarding the translation of the component name, of course it is translated. The prerequisite is that Joomla! has already loaded the extension's language file. This is done automatically for all components with a Components menu entry. I don't see how you could possibly get an untranslated com_foo? Steps to replicate, please?

Regarding the messages of uninstalled extensions, the Seblod guys are doing something they shouldn't be doing. It should be self-understood that when you uninstall an extension its eid is no longer valid and its language files (pointed to by the post-installation messages) are no longer present. Do you want me to filter out uninstalled extensions?

avatar Bakual
Bakual - comment - 27 Aug 2014

Regarding the translation of the component name, of course it is translated. The prerequisite is that Joomla! has already loaded the extension's language file. This is done automatically for all components with a Components menu entry. I don't see how you could possibly get an untranslated com_foo?

Ah I see. It's probably my fault then because I was lazy and just changed an existing message to a different eid.

Regarding the messages of uninstalled extensions, the Seblod guys are doing something they shouldn't be doing. It should be self-understood that when you uninstall an extension its eid is no longer valid and its language files (pointed to by the post-installation messages) are no longer present. Do you want me to filter out uninstalled extensions?

I agree that extensions should remove their messages on uninstall.
Filtering them out may be a good solution indeed.

avatar nikosdion
nikosdion - comment - 29 Aug 2014

I tested the new model method by changing Akeeba Backup to use post-installation messages and found two small bugs which I fixed.

I think that this PR has to be included in Joomla! 3.4, otherwise post-installation messages for third party components will not be very usable.

avatar Bakual Bakual - change - 29 Aug 2014
Labels Added: ?
avatar Bakual
Bakual - comment - 29 Aug 2014

Still one codestyle issue.
However the failed unit tests are not related to this PR but to the newly deprecated assertTag method in PHPUnit itself.

avatar roland-d
roland-d - comment - 29 Aug 2014

Finally had some time to test this and everything works. A few things I wanted to point out:

  • The dropdown should be bootstrap style as is everywhere else in Joomla
  • Why is the message reset button only shown when all messages are hidden and not when 1 of n has been hidden? Could it just be added to the toolbar?
avatar Bakual Bakual - change - 29 Aug 2014
Labels Added: ?
avatar Bakual
Bakual - comment - 29 Aug 2014

The dropdown should be bootstrap style as is everywhere else in Joomla

That's actually not Bootstrap style. It's the chosen script which does that.

Why is the message reset button only shown when all messages are hidden and not when 1 of n has been hidden? Could it just be added to the toolbar?

I think that would be a good improvement for another PR.

I'm setting this to RTC since the new functionality is fine. Everything else can be improved later as well.

avatar roland-d
roland-d - comment - 29 Aug 2014

That's actually not Bootstrap style. It's the chosen script which does that.

Of course, I stand corrected.

avatar Bakual Bakual - reference | - 31 Aug 14
avatar Bakual Bakual - close - 31 Aug 2014
avatar Bakual Bakual - change - 31 Aug 2014
Status New Closed
Closed_Date 0000-00-00 00:00:00 2014-08-31 18:12:38
avatar Bakual Bakual - close - 31 Aug 2014
avatar Bakual
Bakual - comment - 31 Aug 2014

Merged into 3.4-dev branch. Thanks!

Add a Comment

Login with GitHub to post a comment