Salesforce migration #629

djagoda881 · 2023-02-23T09:33:52Z

Summary

The Salesforce source was moved from viadot version 1. The source was moved along with the unit tests. The structure of the source has undergone the following changes compared to viadot 1:

Added class attribute config_key
Updated how secrets are retrieved. After the change, it looks the same as in other sources, e.g. exchange_rates.

Importance

Marge It is needed to use source in prefect-viadot container

Checklist

This PR:

[x ] adds new tests (if appropriate)
[ x] updates docstrings for any new functions or function arguments (if appropriate)
[ x] updates CHANGELOG.md with a summary of the changes

djagoda881 · 2023-03-02T12:11:27Z

I made some minor corrections, the code is now ready for review

trymzet

Some minor adjustments and missing docstrings

viadot/sources/salesforce.py

tests/unit/test_salesforce.py

trymzet · 2023-03-02T12:44:40Z

@djagoda881 random tip, you can just do - [x] and it will create a checked box, this is what these symbols in the checklist are for. Just remove the extra spaces around the x

This reverts commit d0f0416.

tests/unit/test_salesforce.py

viadot/sources/salesforce.py

trymzet · 2023-03-07T11:35:40Z

tests/unit/test_salesforce.py

+        "LastName": ["LastName"],
+    }
+    df_restored = pd.DataFrame(data=data_restored)
+    salesforce.upsert(df=df_restored, table=TABLE_TO_UPSERT)


The second point remains, I think it may be confusing because you're mixing 2 things together in these fixtures, you're creating a test df but also randomly upserting records. It's actually not easy to understand the tests since you already upsert records in the fixture, I don't think this should be happening?

Perhaps you meant to insert some records before running upsert() test in order to validate that they are correctly updated in the Salesforce.upsert() method (to validate the "update" part of upsert()'s functionality)? But in that case, I don't think this needs to be in a fixture, you can do this (note also there is a cleanup at the end, pls also add it - records should be created and removed in each test).

TEST_DF_EXTERNAL = pd.DataFrame({"LastName": [TEST_LAST_NAME], "SAPContactId__c": ["111"]}) def test_upsert(salesforce): # Setup. sf = salesforce.salesforce test_record = TEST_DF_EXTERNAL.to_records() # not sure if this is the data structure required by insert(), pls check sf.Contact.insert(test_record) # Test. salesforce.upsert( df=TEST_DF_EXTERNAL, table=TABLE_TO_UPSERT, external_id="SAPContactId__c" ) result = sf.query( f"SELECT ID, LastName FROM {TABLE_TO_UPSERT} WHERE ID='{ID_TO_UPSERT}'" ) assert result["records"][0]["LastName"] == TEST_LAST_NAME # Cleanup sf.delete(test_record)

trymzet

Added a couple of comments

viadot/sources/salesforce.py

djagoda881 · 2023-03-10T14:23:09Z

I decided to change the test structure by looking at the advice from 1:1 on Thursday.

I have added a bulk upsert test
Each test is self-contained, i.e. rows are created and deleted in each individual test
At the end of the session, it is checked whether any row remains in the database and is deleted if found
In an upsert/bulk_upsert test the insert is performed first and then the update is checked in the same test
I added a get_tested_records function which returns the result of the query. I added it because very often I had to call the same qyery to validate the tests

✅ Added bulk upsert test and refeactored tests

trymzet · 2023-03-13T14:13:07Z

tests/unit/test_salesforce.py

@@ -28,7 +28,7 @@ def test_row_creation(salesforce):
    sf.Contact.delete(result["records"][0]["Id"])


-def test_upsert_external_id_correct(salesforce, test_row_creation):
+def test_upsert_external_id_column(salesforce, test_row_creation):


Why not just test_upsert?

This is the name of the test after the latest changes

trymzet · 2023-03-13T14:14:57Z

tests/unit/test_salesforce.py

+    try:
+        salesforce.upsert(df=df, table="Contact", external_id_column="SAPContactId__c")
+    except Exception as exception:
+        raise exception


Why was this try/except here? Can you add a docstring explaining?

I have removed the all try except blocks from the tests. Inside the function in upsert and bulk upsert the commands are executed in the try except block so if they don't work correctly an error will appear.

✅ Removed try except from tests

trymzet · 2023-03-13T14:15:51Z

tests/unit/test_salesforce.py

+    assert created_rows["records"][0]["LastName"] == "viadot-insert-1"
+    assert created_rows["records"][1]["LastName"] == "viadot-insert-2"


Can't we do result_df.equals(expected_df)?

It is unfortunately not possible to compare in this way. Simple salesforce returns data in OrderedDict form, it can be converted into a data frame but another problem is the Id. The ID is given on insertion and takes the form of a unique hash e.g. ('Id', '0035E000031FfsMQAS'). I would have to add another query that returns the name itself, but this would involve further complications in the code

trymzet · 2023-03-13T14:19:11Z

tests/unit/test_salesforce.py

+    sf.Contact.delete(created_rows["records"][1]["Id"])
+
+
+def test_upsert_external_id_column_wrong(salesforce, test_row_creation):


Can you pls change the name to test_upsert_incorrect_external_id_column?

✅ Renamed test and fixed typos

trymzet · 2023-03-13T14:20:47Z

viadot/sources/salesforce.py


        Args:
-            df (pd.DataFrame): The DataFrame to upsert.
+            df (pd.DataFrame): Dataframe containing the rows to Bulk upsert.


Bulk -> bulk, given -> provided/specified, Dataframe -> DataFrame/pandas DataFrame