Pending

User tests: Successful: 0 Unsuccessful: 0

avatar brianteeman
brianteeman
19 Apr 2013

Only active clients should be visible here and not the one which are in the trash See http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=29562

avatar brianteeman brianteeman - open - 19 Apr 2013
avatar AmyStephen
AmyStephen - comment - 20 Apr 2013

The syntax isn't the best, but it should work and I don't believe the CMS is (yet) enforcing coding standards (which this should fail.) BUT, the bigger question is -- is this really what you want to do?

The client is a very loosely implemented data entity in the CMS. There are many other implications to be considered if the entity is treated as a 1:M relationship. For example, should banners associated with a client that has been unpublished or archived also be unpublished or archived? Should edits be added to prevent a client from being unpublished if there are banners associated? And, so on.

This one needs some thought. Personally, I don't see adding the rigor for this component but if this fine tuning are added, all of them need to be added, or, you'll wind up with banners associated with clients - where the client is unpublished - and the banner can no longer be saved since the client is not listed in the listbox.

You can get yourself into trouble quickly with these types of edits absent some thought about how all of the actions and data interact.

avatar brianteeman
brianteeman - comment - 20 Apr 2013

Thanks so much for your really helpful comments explaining what was wrong with the Pull Request. With help like yours is it any wonder that people don't enjoy contributing to joomla. I know I will think twice before contributing a fix to a problem again if this is the way that real users of joomla and not stalkers comment.

avatar AmyStephen
AmyStephen - comment - 21 Apr 2013

Please don't take technical feedback as personal. It really is not. Databases are my specialty. I've paid a lot of attention to Joomla's data model over the years.

Let me see if this explanation helps:

This PR removes client values from a listbox for banners on the basis of a status code.

That would be just fine, except there is no logic that prevents changing the status code when banners belong to the client.

So, this change could create "orphans" where clients that have a non-published status still have banners associated with them. But, there is no logic in place to deal with that problem when editing the banner or on the frontend interface, either, with displaying or not displaying the banners.

These are just relational database facts.

This is not the first time this issue has been raised for Banners. It's poorly designed in this regard but the implications require more thought. This change could actually create unintended problems.

Thanks, Brian.

avatar phproberto phproberto - reference | - 9 Jun 14
avatar phproberto
phproberto - comment - 9 Jun 2014

This was fixed in 73af2c4

avatar phproberto phproberto - close - 9 Jun 2014

Add a Comment

Login with GitHub to post a comment