-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Create database via UI when ActiveRecord::NoDatabaseError #39723
Conversation
This looks like a job for |
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:
(I think it would be good to break these into separate PRs, too.) |
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. |
dde10f0
to
600b948
Compare
600b948
to
c9abc68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can tell people to run this command every time this specific exception happens.
The database may not exist because they changed by mistake the config/database.yml
to point to a different database. So to resolve the issue they should not be running bin/rails db:create
instead they should be fixing the database name.
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. If the same exception happens for those cases, we have more reason to not add this button by default, neither show an error message telling people to create the db since that would not fix the exception, it would probably just give another error when trying to create the database with the wrong credentials.
I feel we need to understand better the cases where this error happen.
I had a look into this and agree with you, I'm going to update this PR to include a detailed error message instead.
Perhaps a more detailed error like this would be user friendly?
EDIT: I dug into this a little more and checked out what happens when the username/password/hostname is wrong re:
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:
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? |
a95daf1
to
f4ee222
Compare
@@ -8,7 +8,10 @@ | |||
|
|||
module ActiveRecord | |||
module ConnectionHandling # :nodoc: | |||
ER_BAD_DB_ERROR = 1049 | |||
ER_BAD_DB_ERROR = 1049 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f4ee222
to
c5bb74e
Compare
@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? |
8813595
to
4371894
Compare
I rebased and found an error on CI (below), which seems to be related to the stuff I done. Will have a look...
|
f6feb9a
to
c16e94c
Compare
@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. |
@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. |
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. |
@hahmed This change seems like we were close to merging it before, so going to re-open and give it some thought. |
@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)? 🙇 |
mysql. Allow database to be create via a button on the UI for some connection issues.
Co-authored-by: Zachary Scott <[email protected]>
3eadb6a
to
1b8ceee
Compare
Thanks for reviewing @zzak Updated screenshot has the "Create database" button: |
@hahmed Thanks! I will check this out and give it a closer look soon 🙏 |
Summary
Create the database via the UI when database has not been created in development mode.
Related: https://discuss.rubyonrails.org/t/pending-migration-error-usability-concerns/75330/3
I have split out the errors for connection and no database errors and added the button to create the database.