? ? ? Pending

User tests: Successful: Unsuccessful:

avatar MeghaBiranje
MeghaBiranje
7 Nov 2020

…splaying blank value

Pull Request for Issue #31355 // added by JM

Summary of Changes

Testing Instructions

  1. log in to the backend
  2. Goto Components-> Banners -> Clients
  3. Click the New button and fill the form
  4. While filling the form add tabs/spaces in "Contact Name" which is the required field.
  5. Click on Save and close button
  6. You can see the contact column for our added client is blank.

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

avatar MeghaBiranje MeghaBiranje - open - 7 Nov 2020
avatar MeghaBiranje MeghaBiranje - change - 7 Nov 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 7 Nov 2020
Category Administration com_banners
avatar MeghaBiranje MeghaBiranje - change - 7 Nov 2020
The description was changed
avatar MeghaBiranje MeghaBiranje - edited - 7 Nov 2020
avatar adj9 adj9 - test_item - 7 Nov 2020 - Tested successfully
avatar adj9
adj9 - comment - 7 Nov 2020

I have tested this item successfully on f94cf99
Schermata 2020-11-07 alle 15 28 54


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31354.
avatar SharkyKZ
SharkyKZ - comment - 7 Nov 2020

This removes HTML filtering.

avatar ceford
ceford - comment - 8 Nov 2020

I do not see the problem described. And the test by @adj9 does not show the Clients screen. This is my screen grab without the patch applied:

screen shot 2020-11-08 at 07 15 49

I have not looked at the trim filter but I would expect that to remove leading and trailing white space only.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31354.

avatar brianteeman
brianteeman - comment - 8 Nov 2020

@ceford to replicate the issue you need to only put spaces in the field

avatar ceford
ceford - comment - 8 Nov 2020

@ceford to replicate the issue you need to only put spaces in the field

I started with just a space in "John Doe" and that name appears in the Contact column without the patch. The screen grab by @adj9 looks like the Banners list but even there I see the client name. It is not empty. So for me the issue described does not exist.

avatar brianteeman
brianteeman - comment - 8 Nov 2020

ONLY put spaces and see what happens
Then try to create an article with a title containing ONLY spaces

avatar richard67
richard67 - comment - 8 Nov 2020

@MeghaBiranje Is this pull request made for solving your issue #31355 ? If yes: Why haven't you referenced that issue by using the "Pull Request for Issue # ." at the top of the description? It just need to change it to "Pull Request for Issue #31355 ." Or at least leave in the issue a comment referring to this PR, or whatever else way to show the PR belongs to that issue. You can't expect maintainers to do a forensic research to find out which of YOUR pull requests solves which of YOUR issues.

avatar ceford ceford - test_item - 8 Nov 2020 - Tested successfully
avatar richard67
richard67 - comment - 8 Nov 2020

What about @SharkyKZ 's comment above that this PR removed HTML filtering because using the TRIM filter now?

avatar ceford
ceford - comment - 8 Nov 2020

I have deleted my Pass test. With the patch applied I can include html tags, such as <h1>, that are evident in the list view. That is an undesirable side effect.

avatar richard67 richard67 - alter_testresult - 8 Nov 2020 - ceford: Not tested
avatar richard67
richard67 - comment - 8 Nov 2020

@ceford It was not enough to delete the test comment. You should have used again the "Test this" button and then select the "I have not texted" (or so) test result. I've done that for you now, I've reset your test result so it isn't counted by the tracker and in the test on GitHub.

avatar richard67
richard67 - comment - 8 Nov 2020

@ceford It's not only an undesirable side effect, it justifies a negative test result (failed test).

avatar ceford
ceford - comment - 8 Nov 2020

@richard67 thank you for the tips - it seems I am learning the hard way.

Related: is there a case for trimming string input by default?

I was looking for documentation on TRIM and concluded that all of the tutorials I could find are a bit dated. The code in InputFilter.php seemed the best place to look.

avatar infograf768 infograf768 - change - 9 Nov 2020
The description was changed
avatar infograf768 infograf768 - edited - 9 Nov 2020
avatar infograf768
infograf768 - comment - 9 Nov 2020

Looks like the problem here is that the ClientTable does not have its own check() method where we could check that the text entered is empty. Therefore something like this:

	/**
	 * Overloaded check function
	 *
	 * @return  boolean  True if the object is ok
	 *
	 * @see     Table::check()
	 * @since   {
	 */
	public function check()
	{
		try
		{
			parent::check();
		}
		catch (\Exception $e)
		{
			$this->setError($e->getMessage());

			return false;
		}

		// Check for valid contact
		if (trim($this->contact) === '')
		{
			$this->setError(Text::_('COM_BANNERS_PROVIDE_VALID_CONTACT'));

			return false;
		}

		return true;
	}
avatar MeghaBiranje MeghaBiranje - change - 9 Nov 2020
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 9 Nov 2020
Category Administration com_banners Administration com_banners Language & Strings
avatar infograf768
infograf768 - comment - 9 Nov 2020

Needs
use Joomla\CMS\Language\Text;

on top of file

avatar MeghaBiranje MeghaBiranje - change - 9 Nov 2020
Labels Added: ?
avatar MeghaBiranje MeghaBiranje - change - 9 Nov 2020
The description was changed
avatar MeghaBiranje MeghaBiranje - edited - 9 Nov 2020
avatar MeghaBiranje MeghaBiranje - change - 9 Nov 2020
The description was changed
avatar MeghaBiranje MeghaBiranje - edited - 9 Nov 2020
avatar infograf768 infograf768 - test_item - 16 Nov 2020 - Tested successfully
avatar infograf768
infograf768 - comment - 16 Nov 2020

I have tested this item successfully on 7435739


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31354.

avatar gostn gostn - test_item - 16 Nov 2020 - Tested successfully
avatar gostn
gostn - comment - 16 Nov 2020

I have tested this item successfully on 7435739


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31354.

avatar richard67 richard67 - change - 16 Nov 2020
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 16 Nov 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31354.

avatar richard67
richard67 - comment - 16 Nov 2020

@infograf768 Could you approve the code on GitHub so the "Changes requested" disappears at the bottom of the PR which currently still is shown due to your previews review with change requests?

avatar infograf768 infograf768 - change - 19 Nov 2020
Labels Added: ?
avatar infograf768
infograf768 - comment - 19 Nov 2020

Applied @Quy suggestion in #31354 (comment) .
No change for tests. Keeping RTC.

avatar infograf768 infograf768 - change - 22 Nov 2020
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-11-22 08:52:15
Closed_By infograf768
avatar infograf768 infograf768 - close - 22 Nov 2020
avatar infograf768 infograf768 - merge - 22 Nov 2020
avatar infograf768
infograf768 - comment - 22 Nov 2020

tks

Add a Comment

Login with GitHub to post a comment