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

Show friendly message to install action mailbox if the related table does not exist #35581

Merged
merged 1 commit into from Mar 12, 2019

Conversation

prathamesh-sonpatki
Copy link
Member

@rails-bot rails-bot bot added the actionpack label Mar 12, 2019
@kaspth
Copy link
Contributor

kaspth commented Mar 12, 2019

Did you already have Active Storage enabled in the app you tested with? I’m not sure the error message would hit on Action Mailbox first?

@prathamesh-sonpatki
Copy link
Member Author

@kaspth No I didn't have active storage enabled. I was testing the conductor page and clicked on the Action Mailbox Inbound emails link: https://monosnap.com/file/RKUdXiXgy5fY4oAR6fGb6ksle33yRG

@@ -6,6 +6,8 @@
<%= @exception.message %>
<% if defined?(ActiveStorage) && @exception.message.match?(%r{#{ActiveStorage::Blob.table_name}|#{ActiveStorage::Attachment.table_name}}) %>
To resolve this issue run: rails active_storage:install
<% if defined?(ActionMailbox) && @exception.message.match?(%r{#{ActionMailbox::InboundEmail.table_name}}) %>
To resolve this issue run: rails action_mailbox:install
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably needs to be outdented like the line above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

@kaspth
Copy link
Contributor

kaspth commented Mar 12, 2019

Cool! I just wanted to make sure we didn't say that "Active Storage must be installed" for Action Mailbox being missing (yes, it's a dependency but the error should focus on Action Mailbox).

@kaspth
Copy link
Contributor

kaspth commented Mar 12, 2019

If you want to convert this later to use #34788, do feel free to translate them! Perhaps as a PR to that branch?

cc @gsamokovarov

…does not exist

- This is similar to the work done in rails#31534
@prathamesh-sonpatki
Copy link
Member Author

If you want to convert this to use #34788, do feel free to translate them! Perhaps as a PR to that branch?

cc @gsamokovarov

@kaspth Sure, I will do that. Can I do that once this PR is merged or are you suggesting to convert this PR itself?

@kaspth kaspth merged commit 4948663 into rails:master Mar 12, 2019
@kaspth
Copy link
Contributor

kaspth commented Mar 12, 2019

I meant after, but it wasn't clear in the message. But that's fixed now that I merged 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants