Skip to content

Add companies to database #338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kevinlambchops
Copy link
Collaborator

Pull Request

Description

Adds companies to the database when batch-inserting emails

Related Issue

Fixes #294 (without the company size features)

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 Documentation update
  • 🎨 Style update (formatting, renaming)
  • ♻️ Code refactor (no functional changes)
  • ⚡️ Performance improvement
  • ✅ Test update
  • 🔨 Build configuration update
  • 🔒 Security update

Changes Made

  1. Added company_utils functions
  2. Modified fetch_emails_to_db to kick off a task to add company to the database

Screenshots

Testing

  1. Steps to reproduce:

  2. Test environment:
    • OS:
    • Node version:
    • Browser (if applicable):

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Dependencies

Database Changes

  • No database changes
  • [ ] Database changes included (describe below):

API Changes

  • No API changes
  • [ ] API changes included (describe below):

Performance Impact

  • No performance impact
  • [ ] Performance impact (describe below):

Security Considerations

  • No security implications
  • [ ] Security implications considered (describe below):

Additional Notes

Reviewer Notes

Post-Deployment Steps

  • No post-deployment steps needed
  • [ ] Post-deployment steps required (describe below):


By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. ⭐ goal: addition Addition of new feature labels Mar 23, 2025
@kevinlambchops kevinlambchops requested a review from lnovitz March 23, 2025 00:15
# Add company to the database if it doesn't exist
if company_name and company_email_domain:
if not company_exists(company_name, company_email_domain):
add_company(company_name, company_email_domain)
Copy link

Choose a reason for hiding this comment

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

this line and the one above it constitute an n+1 query. approach aside, it should at minimum act on a batch of name+email rather than each one individually

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed +1

from database import engine
with Session(engine) as session:
company = (
session.exec(select(Companies).where(Companies.company_name == company_name and
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to add a helper function. I am imagining scenarios where company name can vary based on the email - e.g. official application confirmation email says "Apero Health" but a later email about interview scheduling only refers to "Apero" but it's still referring to the same company so maybe we need a LIKE check (contains).

Though maybe we should be doing this similarity check beforehand, when processing the email... ok yeah that makes more sense. @jempio @elyseando

So never mind on this comment Kevin but will still leave this here as a TODO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐ goal: addition Addition of new feature size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT]: Track company size
3 participants