From 31cdc63e18f3ceda86ae4014469ac7ac70a09fe6 Mon Sep 17 00:00:00 2001 From: "Aswin Dev P.S" Date: Mon, 11 Apr 2022 19:37:20 +0530 Subject: [PATCH] fix: Remove IMAP and SMTP email validation (#4435) * Remove IMAP and SMTP email validation * Rename imap_email & smtp_email columns to imap_login & smtp_login respectively. * Use channel email domain if inbound email domain not present --- app/helpers/api/v1/inboxes_helper.rb | 4 ++-- .../dashboard/i18n/locale/en/inboxMgmt.json | 12 +++++----- .../dashboard/settings/inbox/ImapSettings.vue | 22 +++++++++---------- .../dashboard/settings/inbox/SmtpSettings.vue | 22 +++++++++---------- app/jobs/inboxes/fetch_imap_emails_job.rb | 2 +- app/mailers/conversation_reply_mailer.rb | 4 ++-- .../conversation_reply_mailer_helper.rb | 14 +++++++++--- app/models/channel/email.rb | 8 +++---- app/views/api/v1/models/_inbox.json.jbuilder | 4 ++-- config/schedule.yml | 2 +- ...ename_imap_email_and_smtp_email_columns.rb | 6 +++++ db/schema.rb | 6 ++--- .../v1/accounts/inboxes_controller_spec.rb | 6 ++--- .../fetch_imap_email_inboxes_job_spec.rb | 2 +- .../inboxes/fetch_imap_emails_job_spec.rb | 2 +- spec/mailboxes/imap/imap_mailbox_spec.rb | 2 +- .../mailers/conversation_reply_mailer_spec.rb | 21 +++++++++++++++++- 17 files changed, 86 insertions(+), 53 deletions(-) create mode 100644 db/migrate/20220409044943_rename_imap_email_and_smtp_email_columns.rb diff --git a/app/helpers/api/v1/inboxes_helper.rb b/app/helpers/api/v1/inboxes_helper.rb index 7f3b68953..56fb79908 100644 --- a/app/helpers/api/v1/inboxes_helper.rb +++ b/app/helpers/api/v1/inboxes_helper.rb @@ -14,7 +14,7 @@ module Api::V1::InboxesHelper Mail.defaults do retriever_method :imap, { address: channel_data[:imap_address], port: channel_data[:imap_port], - user_name: channel_data[:imap_email], + user_name: channel_data[:imap_login], password: channel_data[:imap_password], enable_ssl: channel_data[:imap_enable_ssl] } end @@ -33,7 +33,7 @@ module Api::V1::InboxesHelper end def check_smtp_connection(channel_data, smtp) - smtp.start(channel_data[:smtp_domain], channel_data[:smtp_email], channel_data[:smtp_password], + smtp.start(channel_data[:smtp_domain], channel_data[:smtp_login], channel_data[:smtp_password], channel_data[:smtp_authentication]&.to_sym || :login) smtp.finish unless smtp&.nil? end diff --git a/app/javascript/dashboard/i18n/locale/en/inboxMgmt.json b/app/javascript/dashboard/i18n/locale/en/inboxMgmt.json index b2a054ebc..365fd92f9 100644 --- a/app/javascript/dashboard/i18n/locale/en/inboxMgmt.json +++ b/app/javascript/dashboard/i18n/locale/en/inboxMgmt.json @@ -486,9 +486,9 @@ "LABEL": "Port", "PLACE_HOLDER": "Port" }, - "EMAIL": { - "LABEL": "Email", - "PLACE_HOLDER": "Email" + "LOGIN": { + "LABEL": "Login", + "PLACE_HOLDER": "Login" }, "PASSWORD": { "LABEL": "Password", @@ -514,9 +514,9 @@ "LABEL": "Port", "PLACE_HOLDER": "Port" }, - "EMAIL": { - "LABEL": "Email", - "PLACE_HOLDER": "Email" + "LOGIN": { + "LABEL": "Login", + "PLACE_HOLDER": "Login" }, "PASSWORD": { "LABEL": "Password", diff --git a/app/javascript/dashboard/routes/dashboard/settings/inbox/ImapSettings.vue b/app/javascript/dashboard/routes/dashboard/settings/inbox/ImapSettings.vue index c404c2872..af2d5dd5c 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/inbox/ImapSettings.vue +++ b/app/javascript/dashboard/routes/dashboard/settings/inbox/ImapSettings.vue @@ -34,12 +34,12 @@ @blur="$v.port.$touch" /> " + "" end def in_reply_to_email - conversation_reply_email_id || "" + conversation_reply_email_id || "" end def conversation_reply_email_id diff --git a/app/mailers/conversation_reply_mailer_helper.rb b/app/mailers/conversation_reply_mailer_helper.rb index f95150232..21bc9b939 100644 --- a/app/mailers/conversation_reply_mailer_helper.rb +++ b/app/mailers/conversation_reply_mailer_helper.rb @@ -26,7 +26,7 @@ module ConversationReplyMailerHelper smtp_settings = { address: @channel.smtp_address, port: @channel.smtp_port, - user_name: @channel.smtp_email, + user_name: @channel.smtp_login, password: @channel.smtp_password, domain: @channel.smtp_domain, tls: @channel.smtp_enable_ssl_tls, @@ -48,10 +48,18 @@ module ConversationReplyMailerHelper end def email_from - email_smtp_enabled ? @channel.smtp_email : from_email_with_name + email_smtp_enabled ? @channel.email : from_email_with_name end def email_reply_to - email_imap_enabled ? @channel.imap_email : reply_email + email_imap_enabled ? @channel.email : reply_email + end + + # Use channel email domain in case of account email domain is not set for custom message_id and in_reply_to + def channel_email_domain + return @account.inbound_email_domain if @account.inbound_email_domain.present? + + email = @inbox.channel.try(:email) + email.present? ? email.split('@').last : raise(StandardError, 'Channel email domain not present.') end end diff --git a/app/models/channel/email.rb b/app/models/channel/email.rb index 1c7f534a4..5b1cc0c91 100644 --- a/app/models/channel/email.rb +++ b/app/models/channel/email.rb @@ -6,19 +6,19 @@ # email :string not null # forward_to_email :string not null # imap_address :string default("") -# imap_email :string default("") # imap_enable_ssl :boolean default(TRUE) # imap_enabled :boolean default(FALSE) # imap_inbox_synced_at :datetime +# imap_login :string default("") # imap_password :string default("") # imap_port :integer default(0) # smtp_address :string default("") # smtp_authentication :string default("login") # smtp_domain :string default("") -# smtp_email :string default("") # smtp_enable_ssl_tls :boolean default(FALSE) # smtp_enable_starttls_auto :boolean default(TRUE) # smtp_enabled :boolean default(FALSE) +# smtp_login :string default("") # smtp_openssl_verify_mode :string default("none") # smtp_password :string default("") # smtp_port :integer default(0) @@ -37,8 +37,8 @@ class Channel::Email < ApplicationRecord include Reauthorizable self.table_name = 'channel_email' - EDITABLE_ATTRS = [:email, :imap_enabled, :imap_email, :imap_password, :imap_address, :imap_port, :imap_enable_ssl, :imap_inbox_synced_at, - :smtp_enabled, :smtp_email, :smtp_password, :smtp_address, :smtp_port, :smtp_domain, :smtp_enable_starttls_auto, + EDITABLE_ATTRS = [:email, :imap_enabled, :imap_login, :imap_password, :imap_address, :imap_port, :imap_enable_ssl, :imap_inbox_synced_at, + :smtp_enabled, :smtp_login, :smtp_password, :smtp_address, :smtp_port, :smtp_domain, :smtp_enable_starttls_auto, :smtp_enable_ssl_tls, :smtp_openssl_verify_mode, :smtp_authentication].freeze validates :email, uniqueness: true diff --git a/app/views/api/v1/models/_inbox.json.jbuilder b/app/views/api/v1/models/_inbox.json.jbuilder index dded0ec0d..17bde23e0 100644 --- a/app/views/api/v1/models/_inbox.json.jbuilder +++ b/app/views/api/v1/models/_inbox.json.jbuilder @@ -53,7 +53,7 @@ if resource.email? json.email resource.channel.try(:email) ## IMAP - json.imap_email resource.channel.try(:imap_email) + json.imap_login resource.channel.try(:imap_login) json.imap_password resource.channel.try(:imap_password) json.imap_address resource.channel.try(:imap_address) json.imap_port resource.channel.try(:imap_port) @@ -61,7 +61,7 @@ if resource.email? json.imap_enable_ssl resource.channel.try(:imap_enable_ssl) ## SMTP - json.smtp_email resource.channel.try(:smtp_email) + json.smtp_login resource.channel.try(:smtp_login) json.smtp_password resource.channel.try(:smtp_password) json.smtp_address resource.channel.try(:smtp_address) json.smtp_port resource.channel.try(:smtp_port) diff --git a/config/schedule.yml b/config/schedule.yml index 156a19787..94aca4896 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -16,6 +16,6 @@ trigger_scheduled_items_job: # executed At every 5th minute.. trigger_imap_email_inboxes_job: - cron: '*/5 * * * *' + cron: '*/1 * * * *' class: 'Inboxes::FetchImapEmailInboxesJob' queue: scheduled_jobs diff --git a/db/migrate/20220409044943_rename_imap_email_and_smtp_email_columns.rb b/db/migrate/20220409044943_rename_imap_email_and_smtp_email_columns.rb new file mode 100644 index 000000000..4518647b4 --- /dev/null +++ b/db/migrate/20220409044943_rename_imap_email_and_smtp_email_columns.rb @@ -0,0 +1,6 @@ +class RenameImapEmailAndSmtpEmailColumns < ActiveRecord::Migration[6.1] + def change + rename_column :channel_email, :imap_email, :imap_login + rename_column :channel_email, :smtp_email, :smtp_login + end +end diff --git a/db/schema.rb b/db/schema.rb index 84619b696..93b96cec7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_04_05_092033) do +ActiveRecord::Schema.define(version: 2022_04_09_044943) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" @@ -190,14 +190,14 @@ ActiveRecord::Schema.define(version: 2022_04_05_092033) do t.boolean "imap_enabled", default: false t.string "imap_address", default: "" t.integer "imap_port", default: 0 - t.string "imap_email", default: "" + t.string "imap_login", default: "" t.string "imap_password", default: "" t.boolean "imap_enable_ssl", default: true t.datetime "imap_inbox_synced_at" t.boolean "smtp_enabled", default: false t.string "smtp_address", default: "" t.integer "smtp_port", default: 0 - t.string "smtp_email", default: "" + t.string "smtp_login", default: "" t.string "smtp_password", default: "" t.string "smtp_domain", default: "" t.boolean "smtp_enable_starttls_auto", default: true diff --git a/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb b/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb index 420a94835..6cf33e7ad 100644 --- a/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb @@ -426,7 +426,7 @@ RSpec.describe 'Inboxes API', type: :request do imap_enabled: true, imap_address: 'imap.gmail.com', imap_port: 993, - imap_email: 'imaptest@gmail.com' + imap_login: 'imaptest@gmail.com' } }, as: :json @@ -496,7 +496,7 @@ RSpec.describe 'Inboxes API', type: :request do smtp_enabled: true, smtp_address: 'smtp.gmail.com', smtp_port: 587, - smtp_email: 'smtptest@gmail.com', + smtp_login: 'smtptest@gmail.com', smtp_enable_starttls_auto: true, smtp_openssl_verify_mode: 'peer' } @@ -525,7 +525,7 @@ RSpec.describe 'Inboxes API', type: :request do channel: { smtp_enabled: true, smtp_address: 'smtp.gmail.com', - smtp_email: 'smtptest@gmail.com', + smtp_login: 'smtptest@gmail.com', smtp_port: 587, smtp_enable_ssl_tls: true, smtp_openssl_verify_mode: 'none' diff --git a/spec/jobs/inboxes/fetch_imap_email_inboxes_job_spec.rb b/spec/jobs/inboxes/fetch_imap_email_inboxes_job_spec.rb index 210602ac9..0b23ef9c4 100644 --- a/spec/jobs/inboxes/fetch_imap_email_inboxes_job_spec.rb +++ b/spec/jobs/inboxes/fetch_imap_email_inboxes_job_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Inboxes::FetchImapEmailInboxesJob, type: :job do let(:account) { create(:account) } let(:imap_email_channel) do - create(:channel_email, imap_enabled: true, imap_address: 'imap.gmail.com', imap_port: 993, imap_email: 'imap@gmail.com', + create(:channel_email, imap_enabled: true, imap_address: 'imap.gmail.com', imap_port: 993, imap_login: 'imap@gmail.com', imap_password: 'password', account: account) end let(:email_inbox) { create(:inbox, channel: imap_email_channel, account: account) } diff --git a/spec/jobs/inboxes/fetch_imap_emails_job_spec.rb b/spec/jobs/inboxes/fetch_imap_emails_job_spec.rb index 6c1beecaf..e61198bed 100644 --- a/spec/jobs/inboxes/fetch_imap_emails_job_spec.rb +++ b/spec/jobs/inboxes/fetch_imap_emails_job_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Inboxes::FetchImapEmailsJob, type: :job do let(:account) { create(:account) } let(:imap_email_channel) do - create(:channel_email, imap_enabled: true, imap_address: 'imap.gmail.com', imap_port: 993, imap_email: 'imap@gmail.com', + create(:channel_email, imap_enabled: true, imap_address: 'imap.gmail.com', imap_port: 993, imap_login: 'imap@gmail.com', imap_password: 'password', imap_inbox_synced_at: Time.now.utc - 10, account: account) end let(:email_inbox) { create(:inbox, channel: imap_email_channel, account: account) } diff --git a/spec/mailboxes/imap/imap_mailbox_spec.rb b/spec/mailboxes/imap/imap_mailbox_spec.rb index 0f44e2a40..a78c4fcf1 100644 --- a/spec/mailboxes/imap/imap_mailbox_spec.rb +++ b/spec/mailboxes/imap/imap_mailbox_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Imap::ImapMailbox, type: :mailbox do let(:agent) { create(:user, email: 'agent@example.com', account: account) } let(:channel) do create(:channel_email, imap_enabled: true, imap_address: 'imap.gmail.com', - imap_port: 993, imap_email: 'imap@gmail.com', imap_password: 'password', + imap_port: 993, imap_login: 'imap@gmail.com', imap_password: 'password', account: account) end let(:inbox) { create(:inbox, channel: channel, account: account) } diff --git a/spec/mailers/conversation_reply_mailer_spec.rb b/spec/mailers/conversation_reply_mailer_spec.rb index c1d6d18dd..c64b2a28a 100644 --- a/spec/mailers/conversation_reply_mailer_spec.rb +++ b/spec/mailers/conversation_reply_mailer_spec.rb @@ -156,7 +156,7 @@ RSpec.describe ConversationReplyMailer, type: :mailer do context 'when smtp enabled for email channel' do let(:smtp_email_channel) do - create(:channel_email, smtp_enabled: true, smtp_address: 'smtp.gmail.com', smtp_port: 587, smtp_email: 'smtp@gmail.com', + create(:channel_email, smtp_enabled: true, smtp_address: 'smtp.gmail.com', smtp_port: 587, smtp_login: 'smtp@gmail.com', smtp_password: 'password', smtp_domain: 'smtp.gmail.com', account: account) end let(:conversation) { create(:conversation, assignee: agent, inbox: smtp_email_channel.inbox, account: account).reload } @@ -251,5 +251,24 @@ RSpec.describe ConversationReplyMailer, type: :mailer do expect(mail.in_reply_to).to eq("account/#{conversation.account.id}/conversation/#{conversation.uuid}@#{conversation.account.domain}") end end + + context 'when inbound email domain is not enabled' do + let(:new_account) { create(:account, domain: nil) } + let!(:email_channel) { create(:channel_email, account: new_account) } + let!(:inbox) { create(:inbox, channel: email_channel, account: new_account) } + let(:inbox_member) { create(:inbox_member, user: agent, inbox: inbox) } + let(:conversation) { create(:conversation, assignee: agent, inbox: inbox_member.inbox, account: new_account) } + let!(:message) { create(:message, conversation: conversation, account: new_account) } + let(:mail) { described_class.reply_with_summary(message.conversation, message.id).deliver_now } + let(:domain) { inbox.channel.email.split('@').last } + + it 'sets the correct custom message id' do + expect(mail.message_id).to eq("conversation/#{conversation.uuid}/messages/#{message.id}@#{domain}") + end + + it 'sets the correct in reply to id' do + expect(mail.in_reply_to).to eq("account/#{conversation.account.id}/conversation/#{conversation.uuid}@#{domain}") + end + end end end