centralnicgroup-opensource / rtldev-middleware-whmcs

CentralNic's WHMCS Software Bundle
https://centralnicreseller.com
Other
35 stars 15 forks source link

IBS: Improve Contact Lookup in hook #268

Closed Kian987 closed 11 months ago

Kian987 commented 12 months ago

Hi,

3 or 4 years ago when IBS was in charge of the module, I discovered a serious bug. As soon as I managed to find the cause and implement the fix, I reported everything to IBS and they fixed the module for everyone.

Today after hours of debugging I found out that this same bug is back again 😞 That said, I suspect that you're basing your work on a very old module or a strange latest version that misses old fixes. Over the years I reported and fixed so many bugs and the idea that IBS throw them away is depressing.

Ok now let me describe and fix the bug again.

The file is /hooks.php. The bug is here:

        } elseif ($params["contact"]) {
            // identify the maximum numbers of contacts first
            $result = DB::table("tblcontacts")
                ->select(DB::raw("count(*) as contact_count"))
                ->where("userid", 1)
                ->first();
            if (is_object($result)) {
                $result = json_decode(json_encode($result), true);
            }

            // load all contacts
            $value["userid"] = $_SESSION["uid"];
            $value["limit"] = $result["contact_count"];

            $registrantDetails = localAPI("getcontacts", $value, $adminid);
            $contacts = $registrantDetails["contacts"]["contact"];
            //Get selected contact details from available contacts
            for ($i = 0, $iMax = count($contacts); $i < $iMax; $i++) {
                if ($contacts[$i]["id"] === $params["contact"]) {
                    $countryCode = $contacts[$i]["country"];
                    $phoneNumber = $contacts[$i]["phonenumber"];
                    $formattedPhoneNumber = ibs_reformatPhone($phoneNumber, $countryCode);
                    $isValidPhone = ibs_validatePhone($formattedPhoneNumber);
                    if (!$isValidPhone) {
                        $errors[] = "Invalid phone Number " . $phoneNumber;
                    }
                }
            }
        }

This block of code does the following.

Every time a user register/transfer a domain and requests it to be assigned to a contact the code:

This is terribly wrong for two reasons.

First. GetContacts returns a maximum of 25 records no matter what. If you set 70 the command still returns 25 records. To retreive all contacts you must play with offsets (limitstart). The code doesn't do that therefore every user with more than 25 contacts couldn't be able to transfer/register any domain if they need to be assigned to a contact. Why?

The place where users assign contacts to domain transfers/registrations is cart.php?a=checkout. Here we have a select with all our 70 contacts sorted alphabetically. As we said earlier GetContacts returns 25 ordered by Id. Long story short if you happen to select contacts from 26th to 70th position it's gameover. Contact data will be empty and so errors.

Second. Even with pagination the whole idea is pretty stupid. Given that we aready know the Id of the selected contact, what's the point of perfoming a query to count how many contacts we have and then (indirectly) run additional 3 queries via GetContacts API to retreive data for 25 contacts (25 + 25 + 20). I mean 4 queries to get 70 contacts vs 1 query to get the single contact you really need.

Here is the fix.


        } elseif ($params["contact"]) {

            $contact = DB::table('tblcontacts')->where('id', $params['contact'])->first();

            if (is_object($contact)) {

                $contact = json_decode(json_encode($contact), true);
            }

            $countryCode = $contact['country'];
            $phoneNumber = $contact['phonenumber'];
            $formattedPhoneNumber = ibs_reformatPhone($phoneNumber, $countryCode);
            $isValidPhone = ibs_validatePhone($formattedPhoneNumber);

            if (!$isValidPhone) {

                $errors[] = "Invalid phone Number " . $phoneNumber;
            }
        }
KaiSchwarz-cnic commented 12 months ago

Hey @Kian987!

Thanks for supporting us - very much appreciated!

Honestly, I am not having insights to the past. Team internet.bs has forwarded their registrar module's repository to my department's hands (and later all the related support tickets as well) and thus we can only deal with what we got. I totally understand that this is annoying then to you. That's why I am forwarding my thanks again explicitely to you for again reporting issues plus the solution to us.

Let me add your finding to our sprint for review. I'll keep you in the loop.

Kind Regards Kai

AsifNawaz-cnic commented 11 months ago

Hi Davide,

Thank you for bringing this to our attention. Your input is highly valued.

I'm pleased to inform you that the bug you described has been addressed and implemented in the latest version of the IBS Registrar Module (v4.0.16). This release ensures that users with a higher number of contacts can seamlessly register/transfer domains while assigning them to specific contacts.

If you encounter any further issues or have additional suggestions, please don't hesitate to let us know.

Thank you once again for your contribution.

Best regards, Asif Nawaz