Problem/Motivation

This is a follow-up to #2894270: Users unable to add an extension to the file upload field

Okay, well, so there isn't test coverage for ensuring the thing can't be deleted. So I'd recommend a separate issue, "Add test coverage ensuring that the media source field cannot be removed from a media type (and remove unneeded media_entity_operations_alter())"

Proposed resolution

  • Write tests
  • Remove media_entity_operations_alter implementation

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#7 interdiff-2895581-5-7.txt871 byteschr.fritsch
#7 2895581-7.patch942 byteschr.fritsch
#5 2895581-5.patch1.04 KBchr.fritsch
#2 2895581.patch2.1 KBchr.fritsch

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
StatusFileSize
new 2895581.patch2.1 KB
marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

In #2827014: Throw an exception when testing status code or response headers in functional JavaScript tests I believe getStatusCode() is being deprecated from JavascriptTests, but we still can use it in Functional tests. Can't we just create the type programmatically, then visit the page and check the status code using a functional test?

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Assigned: Unassigned » chr.fritsch
Status: Postponed » Needs work

This is now unblocked. I will work on the tests

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2895581-5.patch1.04 KB

Here is the patch now as a BTB Test.

And i removed the deletion of media_entity_operations_alter, because it will happen in #2895857: Remove media_entity_operation_alter() as planned earlier

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Only nits:

  1. +++ b/core/modules/media/tests/src/Functional/MediaSourceFileTest.php
@@ -51,4 +51,19 @@ public function testSourceFieldSettingsEditing() {
+   * Ensure source field deletion is not possible.
...
+    // It shouldn't be possible to access the delete page of the source field.

    Just having a comment for the entire function seems sufficient to me. 

  2. +++ b/core/modules/media/tests/src/Functional/MediaSourceFileTest.php
@@ -51,4 +51,19 @@ public function testSourceFieldSettingsEditing() {
+  public function testPreventSourceFieldDeletion() {
+

    Nit: extraneous newline. 

  3. +++ b/core/modules/media/tests/src/Functional/MediaSourceFileTest.php
@@ -51,4 +51,19 @@ public function testSourceFieldSettingsEditing() {
+    $assert_session = $this->assertSession();
...
+    $assert_session->statusCodeEquals(403);

    This is used only once, can be put on a single line, without assigning to a temporary variable.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Assigned: chr.fritsch » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2895581-7.patch942 bytes
new interdiff-2895581-5-7.txt871 bytes

Fixed everything from #6

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

#2895857: Remove media_entity_operation_alter() as planned earlier landed, so that part of #2 is already in :)

Because of that, this is only adding testing coverage. #7 looks perfect.

The last submitted patch, 2: 2895581.patch, failed testing. View results

  • Gábor Hojtsy committed 337963d on 8.4.x 
    Issue #2895581 by chr.fritsch, Wim Leers: Add test coverage ensuring...
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Yay, love when things fall into place like that. Thanks a lot!

Status: Fixed » Closed (fixed)

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