Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Check for empty client com_banners #31411

Closed
wants to merge 2 commits into from

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Nov 15, 2020

Pull Request for Issue #31345 and #31355

Summary of Changes

Adds a check to ensure that the client name can not be saved with just blank characters

Adds a check to ensure that the contact name can not be saved with just blank characters

Testing Instructions

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

Repeat above for Client Name

Actual result BEFORE applying this Pull Request

Article is saved

Expected result AFTER applying this Pull Request

Article is not saved with error message saying why

Documentation Changes Required

none

@richard67
Copy link
Member

@brianteeman Do I understand right that this PR here makes PR #31354 obsolete?

@ceford
Copy link
Contributor

ceford commented Nov 16, 2020

With spaces in Name or in Contact Name the error message says in both cases:

Save failed with the following error: Please provide a valid, non-blank title.

Can it be adjusted to say Name or Contact Name as appropriate?


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

@brianteeman
Copy link
Contributor Author

it could be - means more strings for translators - is it really needed?

@infograf768
Copy link
Member

Why this PR?
Is'nt #31354 fine?
If something has to be modified there, why not propose it there?
(I'm thinking of the maybe useless part concerning try/catch)

Also: JLIB_CMS_WARNING_PROVIDE_VALID_NAME uses the wording title. New strings are correctly used in #31354

Note: instructions are wrong as this does not concern article.

@brianteeman
Copy link
Contributor Author

Sorry I didnt see the other PR - I just saw two open issues and wrote a PR to fix them - there was no reference on those issues

@infograf768
Copy link
Member

infograf768 commented Nov 16, 2020

If the try/catch is useless in #31354, please ask there to modify.
And, to encourage new testers/patchers, please close here as duplicate. ;)

@brianteeman
Copy link
Contributor Author

As I said already I did not see the other PR as no one had referenced it on the issues and the issues were left open

@richard67
Copy link
Member

The other PR is from a new contributor, so maybe that's why it didn't reference the issues, or only one of them. Sure we should try not to discourage him, but if it is better to use this PR here then I can try to find some nice words there and ask him to close his PR.

@ceford
Copy link
Contributor

ceford commented Nov 16, 2020

I looked into how to solve this problem client side. It would be nice if we could get a Javascript guru to take this on:

Add class="validate-notwhite" to any form field that is required but must not be white space.

As an experiment, put this in the code that displays the form:

$doc->addScriptDeclaration('
jQuery(document).ready(function(){
	document.formvalidator.setHandler(\'notwhite\',
		function (value) {
			return value.trim().length > 0;
		});
	});
');

It would need something else for a field that is not required but if used must not be white space.

@toivo
Copy link
Contributor

toivo commented Nov 16, 2020

I have tested this item ✅ successfully on b2e0bf8

Tested successfully in Beta6-dev of 16 November


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

@ghost
Copy link

ghost commented Nov 16, 2020

I have tested this item ✅ successfully on b2e0bf8


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 16, 2020
@infograf768
Copy link
Member

Imho I don’t think this pr is better than the other one.
A simple reason: the other pr has clear new strings for the error.
The other possible reason is the try/catch code which may be missing here.

@brianteeman brianteeman deleted the sat2 branch November 16, 2020 23:57
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 16, 2020
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.

None yet

6 participants