Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcoscano created an issue. See original summary.

Krilo_89’s picture

Looks like the action link from the media_library is wrong. Changed "appears_on" to -view.media.media_page_list.
Added patch to display the "Add Media" button to the media library table display.

-edit-

Will also create a test to check if the 'Add media' button exists on the library table page.

Krilo_89’s picture

Status: Active » Needs review
sjerdo’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Seems good. Add media button appears on the media library view page.
We should add a test for this.

Krilo_89’s picture

Krilo_89’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
Krilo_89’s picture

Small error on the test and changed the description. Added a new patch with interdiff.

marcoscano’s picture

Status: Needs review » Needs work

Thanks for working on this! Just a minor consideration:

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -66,9 +66,6 @@ public function testAdministrationPage() {
-    // Verify that the "Add media" link is present.
-    $assert_session->linkExists('Add media');
-

@@ -99,4 +96,25 @@ public function testAdministrationPage() {
+  /**
+   * Tests if Add Media buttons exists on Media Library's administration pages.
+   */
+  public function testAddMediaButtonExistsOnAdministrationPages() {

Why don't we leave the original assertion there, and just add some lines at the end of the existing test that:
- Assert that you can click on the "Table" tab
- Assert that after clicking there, the URL is /admin/content/media-table
- Assert that the "Add media" link is present there too.

Creating a new test just for that would prepare all the environment again, with the setup, etc., and I have the feeling that it would be overkill in this case.

Also, could you please upload a patch only with the test? That would prove that the bug exists and that the fix is a real fix.

Thanks!

Krilo_89’s picture

I did what you asked and removed the second test and added it at the end of the existing test. (I also added some variables for the page and the session on the top of the test)

The first patch is the one with the test only, so this will fail. The second patch is the one with test and solution, which wil pass. Also added a interdiff between 7 and 11.

Krilo_89’s picture

Status: Needs work » Needs review

marcoscano’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! I don't see anything else to complain about here.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs to be rerolled.

pguillard’s picture

Assigned: Unassigned » pguillard
pguillard’s picture

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Looking great! Thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs reroll

Crediting @sjerdo, @marcoscano for reviews.

Committed 91ab96a and pushed to 8.6.x. Thanks!

  • alexpott committed 91ab96a on 8.6.x
    Issue #2983346 by Krilo_89, pguillard, marcoscano, sjerdo: "Add media"...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.