Fix #17404 - Handling and Redirecting to the database list page after dropping the current Database by vimalMK · Pull Request #17886 · phpmyadmin/phpmyadmin

vimalMK · 2022-11-09T06:24:58Z

Signed-off-by: Vimal K vimalinfo10@gmail.com

Description

Handling and Redirecting to the database list page after dropping the current database.

After dropping the DB and redirecting, a gentle message is shown saying 'No database selected'. No error message pop up.

drop_db_pma.webm

Fixes #17404

Before submitting pull request, please review the following checklist:

Make sure you have read our CONTRIBUTING.md document.
Make sure you are making a pull request against the correct branch. For example, for bug fixes in a released version use the corresponding QA branch and for new features use the master branch. If you have a doubt, you can ask as a comment in the bug report or on the mailing list.
Every commit has proper Signed-off-by line as described in our DCO. This ensures that the work you're submitting is your own creation.
Every commit has a descriptive commit message.
Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them.
Any new functionality is covered by tests.

Signed-off-by: Vimal K <vimalinfo10@gmail.com>

codecov · 2022-11-09T06:29:31Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 49.78%. Comparing base (b3498a2) to head (7841d38).
Report is 4189 commits behind head on master.

Files with missing lines	Patch %	Lines
...braries/classes/Controllers/AbstractController.php	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #17886      +/-   ##
============================================
- Coverage     54.59%   49.78%   -4.82%     
- Complexity    16602    16741     +139     
============================================
  Files           617      602      -15     
  Lines         52448    59823    +7375     
============================================
+ Hits          28635    29780    +1145     
- Misses        23813    30043    +6230

Flag	Coverage Δ
dbase-extension	`49.56% <0.00%> (?)`
recode-extension	`48.92% <0.00%> (?)`
unit-7.2-ubuntu-latest	`48.36% <0.00%> (?)`
unit-7.3-ubuntu-latest	`49.64% <0.00%> (-4.85%)`	⬇️
unit-7.4-ubuntu-latest	`50.82% <0.00%> (?)`
unit-8.0-ubuntu-latest	`49.62% <0.00%> (-4.93%)`	⬇️
unit-8.1-ubuntu-latest	`49.69% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

williamdes

Looks good!

OlafvdSpek

Is this the right place for the redirect?

williamdes · 2022-11-09T13:46:37Z

-                    Message::error(__('No databases selected.'))
-                );
+                //Redirecting to the server page and exiting the AJAX control
+                $param = ['ajax_request' => 'false', 'message' => __('No databases selected.')];


after viewing the video it's maybe not the proper message to display 🤔

can you suggest a message you think might fit well . I can update it.

something like "Please select a database" instead of "Database not selected"

vimalMK · 2022-11-09T15:33:42Z

@OlafvdSpek thank you for the review. what page you think might make more sense for redirect ?

OlafvdSpek · 2022-11-09T19:33:03Z

@OlafvdSpek thank you for the review. what page you think might make more sense for redirect ?

If the query is executed in the context of ?route=/server/sql rather than ?route=/database/sql&db=xyz there's no need for special handling. But that'd only work if the operation is performed via /database/operations&db=xyz

vimalMK · 2022-11-09T20:21:51Z

@OlafvdSpek thank you for the review. what page you think might make more sense for redirect ?

If the query is executed in the context of ?route=/server/sql rather than ?route=/database/sql&db=xyz there's no need for special handling. But that'd only work if the operation is performed via /database/operations&db=xyz

The PR will reflect the behavior of redirecting only when the query is executed from /database/sql&db=xyz (Query section)or /database/operations&db=xyz.

Anything performed via ?route=/server/sql will not have any change and will remain as it use to be. That's why I placed the fix inside libraries/classes/Controllers/AbstractController.php protected function hasDatabase(): bool which does the job for that specific use case without effecting anything else. The function hasDatabase() is called every time to verify if the database exist or not throughout the application. I though might be a right place as it uniformly solved all the similar issue in the application at once.

Please let me know what you think about it. :)

MauricioFauth

I also not sure if this is the right change. But I have no better idea. I'd rather add this to the master branch instead. What do you think @williamdes?

MauricioFauth · 2022-11-30T20:55:19Z

@vimalMK Unrelated to this PR. Looks like you are using the wrong video codec for the webm video. That's why it doesn't work on Firefox.

vimalMK · 2022-12-01T04:18:02Z

@vimalMK Unrelated to this PR. Looks like you are using the wrong video codec for the webm video. That's why it doesn't work on Firefox.

Thanks for letting me know. I had no idea. :)

williamdes · 2022-12-01T23:30:17Z

I also not sure if this is the right change. But I have no better idea.

Same

I'd rather add this to the master branch instead. What do you think @williamdes?

Yes, I agree. It's not fixing any real bug. I updated the labels on the issue

MauricioFauth · 2023-08-31T15:31:21Z

Replaced by #18675.

Redirecting to database list page after dropping a database

7841d38

Signed-off-by: Vimal K <vimalinfo10@gmail.com>

vimalMK mentioned this pull request Nov 9, 2022

No database selected error after dropping a database #17404

Closed

williamdes approved these changes Nov 9, 2022

View reviewed changes

OlafvdSpek reviewed Nov 9, 2022

View reviewed changes

williamdes reviewed Nov 9, 2022

View reviewed changes

MauricioFauth approved these changes Nov 30, 2022

View reviewed changes

williamdes changed the base branch from QA_5_2 to master December 1, 2022 23:30

williamdes changed the title ~~Fix #17404. Handling and Redirecting to the database list page after dropping the current Database~~ Fix #17404 - Handling and Redirecting to the database list page after dropping the current Database Dec 29, 2022

MauricioFauth mentioned this pull request Aug 31, 2023

Force a full page reload for top menu links (5.2) #18675

Merged

MauricioFauth self-assigned this Aug 31, 2023

MauricioFauth closed this Aug 31, 2023

MauricioFauth added the wontfix label Sep 24, 2024

Uh oh!

Conversation

vimalMK commented Nov 9, 2022

Description

Uh oh!

codecov Bot commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

williamdes left a comment

Choose a reason for hiding this comment

Uh oh!

OlafvdSpek left a comment

Choose a reason for hiding this comment

Uh oh!

williamdes Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

vimalMK Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

vimalMK Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

vimalMK commented Nov 9, 2022

Uh oh!

OlafvdSpek commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vimalMK commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MauricioFauth left a comment

Choose a reason for hiding this comment

Uh oh!

MauricioFauth commented Nov 30, 2022

Uh oh!

vimalMK commented Dec 1, 2022

Uh oh!

williamdes commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MauricioFauth commented Aug 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Nov 9, 2022 •

edited

Loading

OlafvdSpek commented Nov 9, 2022 •

edited

Loading

vimalMK commented Nov 9, 2022 •

edited

Loading

williamdes commented Dec 1, 2022 •

edited

Loading