Create database via UI when ActiveRecord::NoDatabaseError by hahmed · Pull Reque...

Copy link

Contributor

This looks like a job for ActionableError! (See PendingMigrationError for an example of usage.)

hahmed commented on Jun 26, 2020 •

I have uploaded screenshots for the 2 interfaces.

Would love some feedback on how I can make the pages friendly and welcoming?

As this style of "error pages" are different than the existing ones, happy to move stuff like the logo into a partial once we have an agreed upon layout.

This is only my opinion, but I'm not sure I see the benefit of dedicated error pages. I feel like a combination of the following would accomplish our goals:

• automatically handling errors, e.g. automatically running migrations in dev / test
• using ActionableError
• improving error messages on the exceptions themselves
• improving the friendliness of the shared error template, if necessary

(I think it would be good to break these into separate PRs, too.)

hahmed commented on Jun 29, 2020

Agreed

I'm going to update this PR and description shortly, to only include the action for creating the database via the interface, similar to migrations.

hahmed commented on Jul 15, 2020 •

I had a look into this and agree with you, I'm going to update this PR to include a detailed error message instead.

The database may not exist because they changed by mistake the config/database.yml to point to a different database.

Perhaps a more detailed error like this would be user friendly?

We could not find your database your_app_database_name_here, which can be found in the database configuration file config/database.yml.

What you could do to resolve this issue:

- Has your database name inadvertently been changed? If so, your config file would need to be the name of your database.
- Perhaps you have never created a database for this app, or deleted it? If so you could run `bin/rails db:create` to create your database.

EDIT:

I dug into this a little more and checked out what happens when the username/password/hostname is wrong re:

I also not sure if this error would not also happen if the username or the password is wrong, or the database host is also wrong.

When the username is wrong, the error message we get back is:

# pg error
PG::ConnectionBad (#<PG::ConnectionBad: FATAL:  role "randomuserrole" does not exist>)
# mysql error
Mysql2::Error::ConnectionError

When the hostname is wrong, we get the same exception as above:

# pg error
#<PG::ConnectionBad: could not connect to server: Operation timed out
Is the server running on host "notlocalhost" (92.242.132.24) and accepting
TCP/IP connections on port 5432?
# mysql error
Mysql2::Error::ConnectionError (Can't connect to MySQL server on 'notlocalhost' (60))

For sqlite3 when the username/password or hostname was random, there was no errors thrown.

Based on the errors that are returned, I feel like we could add a better error message and show the button to create the database, what do you think?

hahmed commented on Sep 22, 2020

@rafaelfranca wonder if you have any more feedback on this, I ended up looking at your suggestion about better understanding the error message and I have posted the results in my previous comment -- #39723 (comment).

With mysql we get back a number of different error messages, which I have highlighted here https://github.com/rails/rails/pull/39723/files#r456110152 and only show the button to create the database via the UI when we get a bad connection error.

Similarly to postgresql we only show the create database button in the UI when the database name exists in the error message from pg here https://github.com/rails/rails/pull/39723/files#diff-cc31fc1dd1e78db64638200f710b2f59R40

Let's move this forward. Can you rebase?

hahmed commented on Sep 24, 2020 •

I rebased and found an error on CI (below), which seems to be related to the stuff I done. Will have a look...

Failure:
ActiveRecord::ConnectionAdapters::PostgreSQLAdapterTest#test_reconnection_error [/Users/haroon/projects/rails/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb:56]:
ActiveRecord::ConnectionNotEstablished expected but nothing was raised.


rails test Users/haroon/projects/rails/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb:25

hahmed commented on Oct 21, 2020

@rafaelfranca I ended up reverting the changes for the formatted error message, as it broke a few other tests. I think I will add an attribute onto the exception's to opt-into the simple_formatted display, that way other tests are not impacted. Will follow that up in another PR if that's ok?

Let me know if there is there anything else I need to do on this PR.

hahmed commented on Feb 8

@rafaelfranca is there anything else I can do on this PR to push it forward?

I split out the changes for the formatting which got merged, this PR now only deals with creating the database.

rails-bot bot commented on May 9

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

zzak commented on May 16

@hahmed This change seems like we were close to merging it before, so going to re-open and give it some thought.

zzak commented on May 16

@hahmed Is the button still there to run the rake task for you? If not can you update the screenshots (after updating the knits I suggested)?

hahmed commented on May 17 •

Thanks for reviewing @zzak

Updated screenshot has the "Create database" button:

zzak commented on May 17

@hahmed Thanks! I will check this out and give it a closer look soon