Skip to content

[#33862] Improving the Joomla! extensions update for end-users#3775

Closed
nikosdion wants to merge 12 commits intojoomla:stagingfrom
nikosdion:feature/beating-some-sense-to-extension-updates
Closed

[#33862] Improving the Joomla! extensions update for end-users#3775
nikosdion wants to merge 12 commits intojoomla:stagingfrom
nikosdion:feature/beating-some-sense-to-extension-updates

Conversation

@nikosdion
Copy link
Contributor

See Joomla! Tracker 33862

Summary

This Pull Request makes minor modifications to com_installer for the benefit of end users who have problems fetching updates and extension developers who no longer have to explain to users that they have an impossible to fix Joomla! problem and not a bug with their extensions.

The first change is that the Purge button is reinstated in the toolbar. Reasoning: an extension provides two subsequent updates in a time period less than the "Updates caching (in hours)" (default: 6 hours) setting in the Options page. The update will be invisible until these many hours have elapsed. Even if the user knows the update is there they can't see it and report it as a bug to the developers.

The second change is the addition of an Update Sites management page. Reasoning: Joomla! will unpublish update sites without warning if it cannot contact the update site's server. The user is not notified and the update site cannot be re-enabled. This means that a temporary network issue at the wrong moment will disable updates without warning, putting end users' sites at risk. Even worse, it was IMPOSSIBLE to re-enable the disabled update sites without editing the database. This was also reported to extension developers as a bug with their software.

These are the most often reported issues with the Joomla! extension updater by end users. Narutally the project was completely unaware of the high impact as the receiving end of the complaints has always been the extensions developers, not the Joomla! PLT. As an extensions developer I felt the urge to provide a fix to them for the benefit of everyone using or developing for Joomla!.

Backwards compatibility

This PR is 100% backwards compatible.

Translation impact

This PR introduces 11 new translation strings in 3 INI files. The average time to translate them is 3 minutes.

Testing instructions

Feature 1: Purge button

Log in to your back-end. Go to the Extensions, Extension Manager menu item. Click on Update. Before the patch the toolbar buttons on the left are only Update and Find Updates. After the patch there is a new third button called Purge.

Make sure there are updates shown on the page (you may want to install an outdated extension for that matter). Click on Find Updates. The updates list is cleared. Click on Find Updates. The update list is populated again.

Feature 2: Update Sites

Log in to your back-end. Go to the Extensions, Extension Manager menu item. Look at the right hand link bar. Before the path the items are Install, Update, Manage, Discover, Database, Warnings, Install Languages. After the patch there is a new item called Update Sites. Click on it.

You see a list of update sites. Find an update site of an extension which currently has updates available and unpublish it. Go to Update, click on Purge and then click on Find Updates. You do not see the updates for the extension whose update site you just unpublished.

Go back to Update Sites. Publish the update site you had unpublished in the previous step. Go to Update, click on Purge and then click on Find Updates. You now do see the updates for the extension whose update site you just published

Nicholas K. Dionysopoulos added 2 commits June 13, 2014 20:29
@infograf768
Copy link
Member

+JLIB_DATABASE_ERROR_MUSTCONTAIN_A_TITLE_UPDATESITE="Update site must have a title"
has to be added also in admin en-GB

@infograf768
Copy link
Member

Feature 2:
Disabling language packs update site
Then going to Update
Purge
Result: Purged updates***, 1 disabled site was enabled*
And then Find Updates works without going back to Update Sites to re-enable the language pack update sites.

@nikosdion
Copy link
Contributor Author

The update site purge feature (InstallerControllerUpdate::purge) was calling InstallerModelUpdate::enableSites. That task was put in Joomla! long before this PR because there was no easy way to enable the already disabled update sites. However, now that we are giving our users the option of managing update sites it shouldn't be doing that.

You're right, it's a bug. And I just fixed it :) Can you please test this again?

@nikosdion
Copy link
Contributor Author

@infograf768 One more thing. Could you please provide the URLs of the out of date package you are using? It will help other people with testing this PR. Thank you very much!

@roland-d
Copy link
Contributor

@test:
There is an untranslated string in the browser bar called COM_INSTALLER_TITLE_updatesites when I click on the update sites link.


There are 2 entries in the update sites list that are the same:
updatesites

The 2 entries called Joomla CMS are actually the Joomla Core and Joomla Extension Directory, see here:
updatesites_db
I think the name should be taken from the #__update_sites table.


When I try to sort the update sites table by Type column I get this error:

Column 'type' in order clause is ambiguous SQL=SELECT s.*,e.extension_id,e.name as extension_name,e.type,e.element,e.folder,e.client_id,e.state,e.manifest_cache FROM jos_update_sites AS s INNER JOIN jos_update_sites_extensions AS se on(se.update_site_id = s.update_site_id) INNER JOIN jos_extensions AS e ON(e.extension_id = se.extension_id) WHERE state=0 ORDER BY `type` asc LIMIT 0, 20

When I try to sort by Update Site ID, Status or Name it is not sorting by these columns but it is sorting by the Update Site column.


Testing the check for updates works as described.

That is all for now :)

@b2z
Copy link
Member

b2z commented Jun 14, 2014

I am confirming @roland-d test issues.

@speedy111
Copy link

Thanks for your contribution, Nicholas!

Typo in the test instructions:

Feature 1: Purge button
... Click on Find Updates Purge. The updates list is cleared. Click on Find Updates. The update list is populated again.

Purging and finding updates works for me.

Feature 2: Update Sites

Enabling/disabling update sites works.

  1. How about adding the Update Sites menu-item after the Updates menu-item in stead of the last in the list?
  2. UX improvement: can you move the status column to the second column, right after the checkboxes?
  3. Filter by status doesn't work. Ordering works fine.

Besides that I don't see anything wrong.

I've used Joomla 3.3.1 for testing.

@brianteeman
Copy link
Contributor

Why reinstate the purge button. Why not just make Find Updates do an automatic purge

@cyrez
Copy link
Contributor

cyrez commented Jun 14, 2014

@test success for latest fixes. (great PR Nic!)
@test success for feature 1 & feature 2

One suggestion :
in Updates Sites, we have duplicated id (one for each language) for Accredited Joomla! Translations, but it's only one sever. Maybe having only one line, and replace name by "Translation Packs" ?

I agree too with Brian about an automatic purge (More convenient for the average user)

Tested on 3.3.1

@brianteeman
Copy link
Contributor

When you Purge the cache you get two messages.
Purged updates and There are no updates available at the moment. Please check again later.
screenshot 2014-06-14 11 17 27

That is confusing and bad UX. Surely the correct procedure would be to immediately (and automatically) run the Find Updates. This way you will get two messages
Purged updates AND either There are no updates available at the moment. Please check again later. OR a list of actual updates.

Why make me press two buttons. If I am purging the cache then obviously I want to see if there are any new updates available as well

@brianteeman
Copy link
Contributor

Not created by this PR but would be good to fix at the same time.

The Notice displayed in the Extension Updater is not great
Before updating ensure that the update is compatible with your Joomla! installation. If updating Joomla!, ensure installed extensions are available for the new Joomla! version.

The second sentence is irrelevant as you cant update Joomla here.

screenshot 2014-06-14 11 21 52

However the second sentence belongs in the Com_joomlaupdate screen where there is no notice

screenshot 2014-06-14 11 22 04

@nikosdion
Copy link
Contributor Author

@jurianeven I moved the Status column. The Update Sites was made the last item on the list as it's supposed to be the least frequently used.

@brianteeman The Find Updates button must not purge the updates. Each update site has its own last time checked column. This means that Find Updates will only fetch the update information for the extensions for which updates were not retrieved within X hours, as configured in the Options page. If you purge before finding updates you have to retrieve all updates. This is problematic since when you have too many extensions on a bandwidth limited shared host you may hit the PHP timeout limit. I'm trying to prevent that – unless you really want Joomla! to do that and you click on Purge.

Regarding the messages, they are not both messages. The green one is a message (only displayed after the action). The blue one is status information. I disagree with the "There are no updates" being displayed in a blue message-like alert box, but this is how Joomla! has been doing it since 3.0 and I am not going to change this in this PR. Simply put, I DESPERATELY NEED the Purge button and Update Sites management features for the benefit of my paying clients and I'd be damned if I risked my improvements being rejected or talked to death because I also happened to change the layout of a message.

Same goes for anything else regarding the UX of com_installer. It's deliberately out of scope of this PR. This PR has the sole purpose of saving me 20 hours of senseless support and bad reputation after each and every single release of my software, all caused by Joomla! not allowing my paying clients to easily reset the update information without going into the database. Pretty selfish? Yes, indeed. Also works for everyone else publishing extensions for Joomla!? Darn right. Everybody wins.

@brianteeman
Copy link
Contributor

If we are going to make changes (and we should) then lets get them right this time and not have to make further changes in the future

@speedy111
Copy link

Thanks Nicholas. Can you also fix this please?

Filter by status doesn't work.

@speedy111
Copy link

Why make me press two buttons. If I am purging the cache then obviously I want to see if there are any new updates available as well

You could rename the Purge button to Purge and find updates and then of course also find updates when that button has been clicked.

@roland-d
Copy link
Contributor

@nikosdion All the issues I reported have been fixed. Thanks 👍
As for @jurianeven suggestion to rename the button, looks good to me :)

@nikosdion
Copy link
Contributor Author

@brianteeman I vehemently disagree. My PR intends to fix a very specific thing: Joomla! updates get stuck and Joomla! silently disables the update site. Anything else is out of scope of this PR.

I agree that there are UX issues, that's why I proposed a rewrite of com_installer and drafter a whitepaper on how to do that. There's a WG on that and I'm a technical advisor and sort-of-coordinator. If you want to be the UX advisor, please drop me a line. But let's keep it outside this completely unrelated PR.

@Bakual
Copy link
Contributor

Bakual commented Jun 14, 2014

I agree with the PR in general here. It's something which was on my mind for quite some time as well. So huge +:100: from me for that contribution.

From code review it looks mostly good. I wonder if it could be done without using the deprecated JError. I'd say either use exceptions or enqueue a message there. Or are they really needed? Sounds a bit wrong to use depecated classes and methods in new code 😄

@ghost
Copy link

ghost commented Jun 14, 2014

+1 for not adding back the purge button.
Purge should be done on 'Find updates' and should be done for ALL update sites (also the ones checked 2 seconds ago).

@speedy111
Copy link

Yep leave it like that for KISS reasons (I think the UX idea of Purge and find updates is not bad, but then you have to split the requests with AJAX and so on to make it work, and even then it has to be fine tuned. Way too much hassle for something which isn't used often anyway.). Btw I've read your whole description, @nikosdion. Hopefully it'll ease the pain of explaining.

@roland-d
Copy link
Contributor

Now we can move the JC tracker to Ready for review?

@Bakual
Copy link
Contributor

Bakual commented Jun 14, 2014

I deliberately followed the design of other com_installer models / controllers even though I don't particularly enjoy using JError. IMHO everything should be moved to exceptions. However, for consistency's sake (and saving the sanity of whoever will be maintaining the code in 1-2 years), it should be an all or nothing approach.

I know we have that JError stuff still in many places. The goal is to remove them and the class is already deprecated in favor of native exceptions. So I fully agree with you that it should be exceptions where possible. If you can do it in that PR, it would sure guide others how to properly remove JError in other places. I would very much appreciate if you could do it 👍

@Kubik-Rubik
Copy link
Member

Very good PR, @nikosdion! 👍

@nikosdion
Copy link
Contributor Author

All right, there you go @Bakual Bonus code clarity improvement: eliminating redundant else-blocks, see Rafael Dhoms' keynote in JaB14/

@kennst
Copy link

kennst commented Jun 14, 2014

@test OK

What should the user do when a component have several update sites? My testside includes now 3 update sites for JCE, with three different update sites ID. Is there an easy way to clean up the update sites?

I hoped this would fix issue 33482 (JoomlaCode). But it didn't. @nikosdion do you have a solution for this too? The trouble is with updating Akeeba backup and admin tools
Error: Package download failed: https://www.akeebabackup.com/component/ars/?view=download&id=2690&dlid=&dummy=my.zip

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33482&start=0

@nikosdion
Copy link
Contributor Author

@kennst A single extension can define multiple update sites. Likewise, many extensions can all be updated from the same update site. This is not a bug or a problem, it's how Joomla! works. There's nothing to "do".

Regarding JC33842 you need to upgrade to Joomla! 3.3 and enter your Download ID in the component's Options page. However, this is out of topic. If you need support with our software please file a support ticket on our site, Monday to Friday and me or a co-worker will reply as soon as possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do without an else here, making R.Dohm even more happy 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL @Bakual , was about to write the same 😆 (but for sure, Not a show-stopper)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could swear I had already done that change. Huh! No worries, onto it.

@Bakual
Copy link
Contributor

Bakual commented Jun 15, 2014

Thanks for getting rid of JError, much appreciated 👍

@ghost
Copy link

ghost commented Jun 15, 2014

@test
Works as expected, both feature 1 & 2.

@beat
Copy link
Contributor

beat commented Jun 15, 2014

@nikosdion Great work and good improvement !

Wondering if "Purge" button could have a less scary and clearer name ? like e.g. "Refresh entries" or "Reactivate updates" ?

@infograf768
Copy link
Member

@nikosdion

Still need to sync both lib_joomla.ini as stated in my first post.

Also, we have 2 "Purge updates" string values in core:
one in the installer, one in the library
en-GB.com_installer.ini:150: COM_INSTALLER_PURGED_UPDATES="Purged updates"
en-GB.lib_joomla.ini:491: JLIB_INSTALLER_PURGED_UPDATES="Purged updates"
en-GB.lib_joomla.ini:492: JLIB_INSTALLER_PURGED_UPDATES="Purged updates"

Looks like it is the library one used here; I suggest to change the value to something like:
"Updates successfully purged. Click on 'Find Updates'"

@nikosdion
Copy link
Contributor Author

@infograf768 We are using both, in different contexts. The JLIB string is used in com_joomlaupdate, the COM_INSTALLER string is used in com_installer. I will remove the COM_INSTALLER strings to keep everything consistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this method is not needed and same result could be achieved using the load() method?
https://github.com/phproberto/joomla-cms/blob/staging/libraries/joomla/table/table.php#L692-L701

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you are right, this is is not necessary as far as I can tell. I just followed the example of the other table classes (which I firmly believe are in desperate need of a rewrite, like the rest of the extensions installation/uninstallation code). Do you want me to remove it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please. Less code to maintain 💃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I made the change.

I am hoping we can include this in Joomla! soon. Today I had another three clients with update issues, two caused by unpublished update sites and one caused by a stuck update cache.

@phproberto
Copy link
Contributor

Added a comment about removing an (in my opinion) unneeded function but 👍 here. I was very happy when the purge button was removed but I can understand why it's needed. Thanks for the explanation.

@peterlose
Copy link
Contributor

Just tested the functionality of the PR, seem to work as expected. Great job @nikosdion!

@Bakual
Copy link
Contributor

Bakual commented Jul 9, 2014

Merged into 3.4. Thanks all for testing and especially @nikosdion for coding! 👍

@Bakual Bakual closed this Jul 9, 2014
@betweenbrain
Copy link
Contributor

w00t!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.