Bugfix For Issue: 451 - Allow for either Name or CreatedDate as order by.

[nicholas-sullivan]

451 Harcoded Order by name fails with encrypted name.

Add fix so that selected fflib_SObjectSelectorTest tests pass in orgs where platform encryption is enabled on Account.Name.

Existing tests

• toSOQL_When_SystemModeAndChildRelationship_Expect_WellFormedSOQL
• testSOQL_defaultSorting

Fail in orgs with encryption enabled on Account.Name

The following code in fflib_SObjectSelector means the order field varies from Name in orgs where name isn't encrypted to CreatedDate where Name is encrypted.

 public virtual String getOrderBy()
 {
    if (m_orderBy == null)
    {
        Schema.SObjectField nameField = describeWrapper.getNameField();
        if (nameField != null && !nameField.getDescribe().isEncrypted())
        {
            m_orderBy = nameField.getDescribe().getName();
        }
        else
        {
            m_orderBy = DEFAULT_SORT_FIELD;
            try {
                if (describeWrapper.getField(m_orderBy) == null)
                {
                    m_orderBy = SF_ID_FIELD;
                }
            }

Updating the test regex to (?:Name|CreatedDate) ensures both tests pass in orgs regardless of the encryption status of Account.Name

---

This change is 

[nicholas-sullivan]

Hi @stohn777 ,

There's also another question regarding the specific usage of isEncrypted() in the getOrderBy() method, it's currently an assumption that encrypted fields can't be sorted however this may change, and hopefully isSortable() should be correct.

Would it better to rely on isSortable() and not isEncrypted() ? Unless there was a specific reason for the encryption check rather than checking it's sortable?

[nicholas-sullivan]

Also noticed the same issue applies to the toSOQL_When_SystemModeAndParentRelationshipAndDuplicateFields_Expect_WellFormedSOQL test as well.

As sorting is also done on Opportunity Name which will fail if this field is encrypted.

Given that there are multiple combinations between Account and Opportunity Name fields within a single query I believe it would be better to use String.format to replace the appropriate Account/Opportunity name fields rather than conditionaly changing the whole pattern itself.

[stohn777]

Hi @nicholas-sullivan.

I see three other tests are failing too because of a flaw in the fflib_SObjectSelectorTest.Testfflib_SObjectSelector.getOrderBy method which specifies "Name" in the order-by clause. Being related, would you like to provide a fix for that too?

The failing tests and errors are: testCRUDOff

Class.fflib_SObjectSelector.selectSObjectsById: line 336, column 1
Class.fflib_SObjectSelectorTest.testCRUDOff: line 151, column 1

testQueryLocatorById

Class.fflib_SObjectSelector.queryLocatorById: line 349, column 1
Class.fflib_SObjectSelectorTest.testQueryLocatorById: line 86, column 1

testSelectSObjectsById

Class.fflib_SObjectSelector.selectSObjectsById: line 336, column 1
Class.fflib_SObjectSelectorTest.testSelectSObjectsById: line 61, column 1

[nicholas-sullivan]

Hi @stohn777 ,

I've updated the tests mentioned however I haven't added any specific handling for encryption being enabled, instead using CreatedDate in place of Name where the order by was a static string.

In theory the following in Testfflib_SObjectSelector could be updated to alternate between Name and CreatedDate along with all the related tests, but not sure that there's any added value. Unlike the case where getOrderBy() is actually performing logic.

public override String getOrderBy()
{
    return 'CreatedDate DESC, AnnualRevenue ASC NULLS LAST';
}

[stohn777]

Hi again @nicholas-sullivan.

You've been busy!! I'm not sure you meant to do it, but your commit has many additions to the test class beyond the change in fflib_SObjectSelectorTest.Testfflib_SObjectSelector.getOrderBy. Did you mean to commit all of that? Looks like you have a bunch of more extensive test code, which all pass by the way, but I'm not sure it's needed by the framework.

❔

[nicholas-sullivan]

Hi @stohn777 ,

You're right there's probably more in there than is needed, I've instead updated to use AccountNumber instead which given the names were in the same order as the AccountNumber was didn't require any extra changes to the existing tests.

[nicholas-sullivan]

Hi @daveespo ,

This has been updated where applicable however for cases such as.

public override String getOrderBy()
{
    return 'AccountNumber DESC, AnnualRevenue ASC NULLS LAST';
}

Simply relying on the getOrederBy() result will fail as the generated SOQL will add in NULLS FIRST to the first order by, in those cases the exact value has been used.