? Success

User tests: Successful: Unsuccessful:

avatar rdeutz
rdeutz
6 Feb 2015

Executive summary

This patch removes some unused (javascript/PHP) code in the following views

  • Banners
  • Banners-Client
  • Categories
  • Articles
  • Users
  • Menu Items

Backwards compatibility

Full b/c, no problems are expected

Translation impact

none

Testing instructions

  • Install a fresh Joomla! with testing data.
  • Log into to backend and go to listed views above and check if ordering works when you click on the table headers and when using the select to chose ordering
  • Apply Patch
  • Go to listed views above and check if ordering works when you click on the table headers and when using the select to chose ordering
  • Nothing should have changed
avatar rdeutz rdeutz - open - 6 Feb 2015
avatar gunjanpatel
gunjanpatel - comment - 6 Feb 2015

Tested and works for me but don't know why I didn't find this issue on http://issues.joomla.org/tracker/joomla-cms/

avatar rdeutz
rdeutz - comment - 6 Feb 2015

@mbabker can you have a look why this isn't synced with issues.j.o, thanks

avatar mbabker
mbabker - comment - 6 Feb 2015

I'm resending failed payloads right now. See joomla/jissues#617 for more.

avatar brianteeman
brianteeman - comment - 6 Feb 2015

I am confused by this PR - if the code is not used then why was it
refactored and how was it actually tested with PR #5041, #5255 and quote a
few other PR

On 6 February 2015 at 13:56, Michael Babker notifications@github.com
wrote:

I'm resending failed payloads right now. See joomla/jissues#617
joomla/jissues#617 for more.


Reply to this email directly or view it on GitHub
#5994 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar dgt41
dgt41 - comment - 6 Feb 2015

@brianteeman The refactor of all these views was mainly focused on two parts:
introduce formvalidator
use the joomla API for the inline scripts.
The later (which is the case here) was a one-to-one transformation from inline scripts to addScriptDeclaration(), nothing more nothing less

avatar brianteeman
brianteeman - comment - 6 Feb 2015

My concern is that the code was "tested" - how could it be if the code isnt
used - Its a general comment about our testing procedures

On 6 February 2015 at 14:11, Dimitris Grammatiko notifications@github.com
wrote:

@brianteeman https://github.com/brianteeman The refactor of all these
views was mainly focused on two parts:
introduce formvalidator
use the joomla API for the inline scripts.
The later (which is the case here) was a one-to-one transformation from
inline scripts to addScriptDeclaration(), nothing more nothing less


Reply to this email directly or view it on GitHub
#5994 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar dgt41
dgt41 - comment - 6 Feb 2015

Tested => worked without error
Reviewed => Code was inspected
The problem wasn’t the testers in this particular event. Also no blame on the commiter or anyone that reviewed that code, because just by reading the changes made you couldn’t find any flaw because the initial code was useless.
My 2c

avatar brianteeman
brianteeman - comment - 6 Feb 2015

This is the problem "in general" that concerns me.

People are regularly applying patches and saying it works without
confirming beforehand that they have observed the issue and have observed
the changed behaviour. Joomla lives in a web browser - just reading the
code is not enough

On 6 February 2015 at 14:23, Dimitris Grammatiko notifications@github.com
wrote:

Tested => worked without error
Reviewed => Code was inspected
The problem wasn’t the testers in this particular event. Also no blame on
the commiter or anyone that reviewed that code, because just by reading the
changes made you couldn’t find any flaw because the initial code was
useless.
My 2c


Reply to this email directly or view it on GitHub
#5994 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar rdeutz
rdeutz - comment - 6 Feb 2015

The problem is that after introducing the search tools the old code wasn't removed. The fact that we don't use the search tools everywhere and in the other situations (not used search tools) these code is needed doesn't makes it easier. Without really looking deep into the code it is hard to spot that this is unused.

avatar zero-24 zero-24 - change - 6 Feb 2015
Category Administration Code style
avatar zero-24 zero-24 - change - 6 Feb 2015
Status New Pending
avatar rdeutz rdeutz - alter_testresult - 9 Feb 2015 - gunjanpatel : Tested successfully
avatar brianteeman brianteeman - test_item - 10 Feb 2015 - Tested successfully
avatar brianteeman
brianteeman - comment - 10 Feb 2015

@test I cant see any changes in ordering so I guess this is a successful test


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5994.
avatar brianteeman
brianteeman - comment - 11 Feb 2015

As we have two tests I am setting this RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5994.
avatar brianteeman brianteeman - change - 11 Feb 2015
Status Pending Ready to Commit
avatar brianteeman brianteeman - change - 11 Feb 2015
Labels Added: ? ?
avatar brianteeman brianteeman - change - 11 Feb 2015
Labels Added: ? ?
avatar infograf768 infograf768 - change - 12 Feb 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-02-12 08:20:13
avatar infograf768 infograf768 - close - 12 Feb 2015
avatar infograf768 infograf768 - reference | - 12 Feb 15
avatar infograf768 infograf768 - merge - 12 Feb 2015
avatar infograf768 infograf768 - close - 12 Feb 2015
avatar rdeutz rdeutz - head_ref_deleted - 12 Feb 2015

Add a Comment

Login with GitHub to post a comment