Skip to content
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

feat: Contact Exports #7258

Merged
merged 25 commits into from
Jun 13, 2023
Merged

feat: Contact Exports #7258

merged 25 commits into from
Jun 13, 2023

Conversation

tejaswinichile
Copy link
Contributor

@tejaswinichile tejaswinichile commented Jun 6, 2023

@netlify
Copy link

netlify bot commented Jun 6, 2023

Deploy Preview for chatwoot-storybook canceled.

Name Link
🔨 Latest commit 4642475
🔍 Latest deploy log https://app.netlify.com/sites/chatwoot-storybook/deploys/6487270ed118c30008312aa7

.gitignore Outdated
@@ -31,6 +31,7 @@ master.key
.env

public/uploads
public/contacts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sojan-official Need your thoughts on where we should save the file for contacts export.

Copy link
Member

Choose a reason for hiding this comment

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

wouldn't need this folder / the file doesn't need to be stores in an open folder.

Copy link
Member

@sojan-official sojan-official left a comment

Choose a reason for hiding this comment

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

We need to change the way the export files are stored.
ref comments.


def send_an_email_to_team(account)
subject = "Your contact's export file is available to download."
@action_url = "#{Rails.root}/public/contacts/#{account.name}_#{account.id}_contacts.csv"
Copy link
Member

Choose a reason for hiding this comment

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

@tejaswinichile This wouldn't work well on production as we use multiple nodes to serve the requests. Also, this creates the file in the sidekiq pod and won't be available for the rails server.

So the flow should be that we create a temp file in the job, create an active record object uploading that file to the filestore, ie ( AWS, GCS etc ) .. and use that URL in the email.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sojan-official Should we attach file this to the account or contact if we are creating an active storage object, or just an individual object, is fine?

Copy link
Member

Choose a reason for hiding this comment

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

@tejaswinichile it will be a nice idea to attach to account, as we can delete these objects on account destroy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sojan-official Attaching it to the account.

@voko3456
Copy link

voko3456 commented Jun 6, 2023

Fixes: https://linear.app/chatwoot/issue/CW-1795/export-of-contacts

When I select lable it doesn't show the contacts assigned with that label because the label is on conversation not on contact

Copy link
Member

@sojan-official sojan-official left a comment

Choose a reason for hiding this comment

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

Mostly there. few comments

.gitignore Outdated
@@ -31,6 +31,7 @@ master.key
.env

public/uploads
public/contacts
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't need this folder / the file doesn't need to be stores in an open folder.

Comment on lines 17 to 21

path_to_file = "#{Rails.root}/public/contacts/#{account.id}/#{account.name}_#{account.id}_contacts.csv"

expect(CSV.open(path_to_file, 'r')).to be_truthy
File.delete(path_to_file)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right ? 😅

Copy link
Member

Choose a reason for hiding this comment

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

lets also just verify the csv file contains the legit data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets also just verify the csv file contains the legit data

Open and read downloaded file.

This doesn't look right ? 😅

It was persistent in the machine after the test are done running :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sojan-official

Addressed this.

@tejaswinichile tejaswinichile temporarily deployed to staging-chatwoot June 12, 2023 08:51 Inactive
@tejaswinichile
Copy link
Contributor Author

@sojan-official I Tested the export on staging, please have a look.

https://www.loom.com/share/ea556d2a054f40db9f9cb3ffcbe4dce5
https://www.loom.com/share/bab12d19df8640cb820f2288fd792bb5

def attach_export_file(account, csv_data)
return if csv_data.blank?

account.contacts_export.attach(
Copy link
Member

Choose a reason for hiding this comment

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

What happens in cases where the account already has a contact_export attached?
Will calling .attach again purge the file from our storage? or should we do it explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It replaces the same file @sojan-official and purges the old one automatically.

Copy link
Member

@sojan-official sojan-official left a comment

Choose a reason for hiding this comment

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

  • PTAL the added comment
  • LGTM otherwise

@tejaswinichile tejaswinichile merged commit 23ca6d5 into develop Jun 13, 2023
@tejaswinichile tejaswinichile deleted the CW-1795 branch June 13, 2023 03:48
tejaswinichile added a commit that referenced this pull request Jun 14, 2023
scmmishra pushed a commit that referenced this pull request Jun 16, 2023
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants