search

salesagility / SuiteCRM

SuiteCRM - Open source CRM for the world
https://www.suitecrm.com
GNU Affero General Public License v3.0
4.5k stars 2.09k forks source link

Export fails when trying to export all records (DB error) #3470

Closed gunnicom closed 7 years ago

gunnicom commented 7 years ago

Issue

If you go to a module and select entire list without a filter in the search, the export fails with a database error. This occurs only if you have multiple pages in your list.

The error seems to be in the SELECT at " WHERE () AND leads.deleted=0" As workaround you can filter by names and simply use "%" as filter, then it works.

Tested in Demo on suitecrm site. Error is reproducible there.

Expected Behavior

Export should work if entire list is selected.

Actual Behavior

Export does not work because of database error.

Steps to Reproduce

  1. Go to leads
  2. delete any filter
  3. select complete list
  4. export. If you have more than one page it will fail. With only one page it will work.

Your Environment

ptwentypt commented 7 years ago

Hi,

I can confirm this is also an issue on the 7.8.3. release, MYSQL 5.5 and PHP 5.6.

Thank you

SohanTirpude commented 7 years ago

I have checked on SuiteCRM demo version. But couldn't reproduce the same error. I am able to download complete list of records which were almost 50 in account module without any hassle. Also checked on SuiteCRM 7.8.2. But export functionality is working smooth.

chris001 commented 7 years ago

Possibly the difference is, the online demo is using mysql 5.6, while @gunnicom is using the version of mysql which comes with ubuntu 16.04 which is mysql 5.7

chris001 commented 7 years ago

The real missing ingredient to pinpoint the cause of this bug, is a proper modern error handling package....

gunnicom commented 7 years ago

@chris001 I could reproduce it on the demo, as you can read in my report.

ptwentypt commented 7 years ago

@ChangezKhan : The issue is related to the Leads Module. I can also export all the of the Accounts in the Account Module.

I think the problem is, when you select all records the query to mysql has this part "WHERE ()" which I believe is incorrect syntax on all MySQL versions.

Thank you.

SohanTirpude commented 7 years ago

Ok, the problem is with only 'Leads' module then? Please correct me if I am wrong. If yes, then I think the title should be corrected.

chris001 commented 7 years ago

No doubt, the Leads module search code is a copy-pasted variant of the Accounts module search code, this is why it's working differently.

Search code should be one centralized class, used by all modules, and not a copy-pasted variant of it, in every module.

SohanTirpude commented 7 years ago

I have found the error. It is in Person class located in include\SugarObjects\templates\person\Person.php file. On line no. 321, where it checks if $where is not equal to blank. It is using !== to check while in SugarCRM , it uses !=. I don't know how to explain this but this is happening due to checking the type of $where variable. If you remove extra = from if condition, it works fine. In SugarCRM also, it uses only != while comparing $where to blank.

chris001 commented 7 years ago

Excellent find @ChangezKhan . Have you ever made a Pull Request? If you could, make a PR with your one line change in the code, and it should get merged rather quickly.

SohanTirpude commented 7 years ago

Ok, I'll. But don't you think that we need to check why there is a extra = sign in SuiteCRM version only?

pgorod commented 7 years ago

This is the commit that broke it 3 months ago: https://github.com/salesagility/SuiteCRM/commit/c328dbe50c96365cf09147260b28795c5e78e31a

Since this commit had a ton of changes, mostly syntactical and coding-convention stuff, I would say this probably was just an accident - something that looked better in a quick consideration, but had an unintended negative side-effect.

chris001 commented 7 years ago

@ChangezKhan No reason for that line of code to be there. Human error, for sure.

The craziest thing is, if the automated testing tools had been activated on this github project like I've been clamoring for since quite a while, these automatic tools would have flagged the error in the code, preventing it from being released to the public with the error in it...

ptwentypt commented 7 years ago

@ChangezKhan : Thank you. I confirm this solves the issue.

SohanTirpude commented 7 years ago

@chris001, may I know, which automated testing tools you were asking for? Any particular name, you have in mind?

chris001 commented 7 years ago

@ChangezKhan My favorite free automated testing tools to add to github are:

  1. php-cs-fixer - this works by being called by Travis-CI. I provided a PR to activate it. SalesAgility has ignored it!
  2. Scrutinizer-CI
  3. Travis-ci currently very incomplete and disfunctional. It's not really doing anything at all!
  4. php-codesniffer
  5. CodeReviewHub.com This is also provided in an Issue here, which is totally ignored by SalesAgility!