Skip to content

Conversation

@anish-work
Copy link
Contributor

@anish-work anish-work commented Jan 30, 2025

Q/A checklist

  • If you add new dependencies, did you update the lock file?
poetry lock --no-update
  • Run tests
ulimit -n unlimited && ./scripts/run-tests.sh
  • Do a self code review of the changes - Read the diff at least twice.
  • Carefully think about the stuff that might break because of this change - this sounds obvious but it's easy to forget to do "Go to references" on each function you're changing and see if it's used in a way you didn't expect.
  • The relevant pages still run when you press submit
  • The API for those pages still work (API tab)
  • The public API interface doesn't change if you didn't want it to (check API tab > docs page)
  • Do your UI changes (if applicable) look acceptable on mobile?
  • Ensure you have not regressed the import time unless you have a good reason to do so.
    You can visualize this using tuna:
python3 -X importtime -c 'import server' 2> out.log && tuna out.log

To measure import time for a specific library:

$ time python -c 'import pandas'

________________________________________________________
Executed in    1.15 secs    fish           external
   usr time    2.22 secs   86.00 micros    2.22 secs
   sys time    0.72 secs  613.00 micros    0.72 secs

To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:

def my_function():
    import pandas as pd
    ...

Legal Boilerplate

Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@anish-work anish-work requested a review from devxpy February 3, 2025 19:50
@devxpy
Copy link
Member

devxpy commented Feb 5, 2025

use the django extensions runscript format for the script https://django-extensions.readthedocs.io/en/latest/runscript.html

@devxpy
Copy link
Member

devxpy commented Feb 5, 2025

I also don't see any use for the classes here, I think it would be better to just use functions. For the logger you can just use loguru like we do everywhere else in code. The mapping_config & bulk_uploader you're only initializing once, so it makes sense to keep it in a global scope (function local scope?) and pass it around instead of storing it in class.

):
stats = {"processed": 0, "successful": 0, "failed": 0, "errors": []}
# Build transaction query with optional date filtering
transaction_query = AppUserTransaction.objects.all().order_by("created_at")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppUserTransaction.objects.select_related("user", "workspace__created_by").all().order_by(...)

(fewer database queries)

@devxpy
Copy link
Member

devxpy commented Feb 26, 2025

bump

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR adds a new script for syncing transaction data with Zoho CRM, implementing a comprehensive integration system with bulk upload capabilities.

  • Added scripts/zoho_crm.py with three main classes: ConfigurableFieldMapper for field mapping, ZohoBulkUploader for handling bulk uploads, and ZOHOSync for orchestrating the synchronization process
  • Implemented batch processing to handle large datasets efficiently with configurable batch sizes
  • Added proper error handling and logging throughout the synchronization process
  • Added Zoho CRM configuration settings (ZOHO_AUTH_CODE and ZOHO_ORG_ID) to Django settings
  • Created a modular design with separate mapping configurations for contacts, accounts, and deals

Gooey AI: Gooey-server PR Review Bot

2 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +5 to +10
import json
from typing import List, Dict, Optional, Any, Callable
from datetime import datetime, timedelta
import requests
import json
import csv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: json is imported twice (lines 5 and 9) and List, Dict, Optional, Any are imported twice (lines 6 and 12)

Suggested change
import json
from typing import List, Dict, Optional, Any, Callable
from datetime import datetime, timedelta
import requests
import json
import csv
import json
from typing import List, Dict, Optional, Any, Callable, Tuple
from datetime import datetime, timedelta
import requests
import csv

Comment on lines +545 to +546
def _get_user_confirmation(self, label: str) -> bool:
f"Are you sure you want to proceed with the upload for batch {label}?"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Line 546 is an f-string but it's not assigned to anything - should be a docstring or comment

Suggested change
def _get_user_confirmation(self, label: str) -> bool:
f"Are you sure you want to proceed with the upload for batch {label}?"
def _get_user_confirmation(self, label: str) -> bool:
"""Ask user for confirmation before proceeding with batch upload."""

Comment on lines +582 to +583
start_date = datetime.strptime(argv[0], "%Y-%m-%d") if argv else None
run_optimized_sync()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: start_date is parsed from command line but not passed to run_optimized_sync()

Suggested change
start_date = datetime.strptime(argv[0], "%Y-%m-%d") if argv else None
run_optimized_sync()
start_date = datetime.strptime(argv[0], "%Y-%m-%d") if argv else None
run_optimized_sync(start_date=start_date)

Comment on lines +499 to +512
if accounts:
account_file = self.bulk_uploader.create_bulk_upload_file(
accounts, "Accounts", batch_label
)

if contacts:
contact_file = self.bulk_uploader.create_bulk_upload_file(
contacts, "Contacts", batch_label
)

if deals:
deal_file = self.bulk_uploader.create_bulk_upload_file(
deals, "Deals", batch_label
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Missing initialization of variables before conditional blocks. If any of these conditions are false, the variables will be undefined when used later

Suggested change
if accounts:
account_file = self.bulk_uploader.create_bulk_upload_file(
accounts, "Accounts", batch_label
)
if contacts:
contact_file = self.bulk_uploader.create_bulk_upload_file(
contacts, "Contacts", batch_label
)
if deals:
deal_file = self.bulk_uploader.create_bulk_upload_file(
deals, "Deals", batch_label
)
account_file = None
contact_file = None
deal_file = None
if accounts:
account_file = self.bulk_uploader.create_bulk_upload_file(
accounts, "Accounts", batch_label
)
if contacts:
contact_file = self.bulk_uploader.create_bulk_upload_file(
contacts, "Contacts", batch_label
)
if deals:
deal_file = self.bulk_uploader.create_bulk_upload_file(
deals, "Deals", batch_label
)

"contact_mapping": {
"Gooey_User_ID": {"db_key": "uid"},
"Gooey_Admin_Link": {
"db_key": "id",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use named tuple instead of dict

:return: Mapping configuration dictionary
"""
default_config = {
"contact_mapping": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contact_mapping, transaction_mapping & workspace_mapping can be 3 top level constants instead of a dynamic string inside a dict generated from a class method which is very un-necessary and adds cruft


ZOHO_CONTACT_API = "https://www.zohoapis.com/crm/v2/Contacts"
ZOHO_DEAL_API = "https://www.zohoapis.com/crm/v7/Deals"
ZOHO_HEADERS = {"Authorization": f"Bearer {settings.ZOHO_AUTH_CODE}"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generate this inside a method like we do in azure_image_moderation so that settings can be loaded at runtime if needed

ZOHO_HEADERS = {"Authorization": f"Bearer {settings.ZOHO_AUTH_CODE}"}
ZOHO_BULK_FILE_UPLOAD_API = "https://content.zohoapis.com/crm/v7/upload"
ZOHO_BULK_CREATE_JOB = "https://www.zohoapis.com/crm/bulk/v7/write"
ZOHO_ORG_ID = settings.ZOHO_ORG_ID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

un-necessary to re-declare this variable

else:
zoho_fields[zoho_field] = field_config.get("default") or "None"

except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


return default_config

def _deep_merge(self, base: Dict, update: Dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused method

@devxpy devxpy requested a review from Copilot April 15, 2025 02:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@devxpy devxpy marked this pull request as draft April 29, 2025 00:02
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.

4 participants