Radio buttons on Available updates report do nothing

I know that the 'available updates' page does not have any test coverage. We are slowly improving, see #3328058: Expand the phpunit test coverage for module_filter

When using the previous module_filter version 4.0.1 onn core 10.2 the radio buttons do not work and the t.nodeName error is shown.

The new Module Filter 4.1.0 on core 10.1 works fine, the buttons efffect the page filtering and there is no console error.

So the problem is with how Module Filter interacts with Core 10.2, and not the commits recently made for 4.1.0

This was caused by a change in the `this` scope between the anonymous function and the lambda function. In the anonymous function `this` is the currently selected input, in the lambda function `this` is the attach abject. Adding the event argument and using the current target correctly pulls the currently selected input and allows the function to behave as it used to.

I'm testing this in 10.1.7 prior to upgrading to 10.2

I can confirm the patch in comment #6 fixes the issue on core 10.2.1, thank you very much!

Am I reading the failure correctly that there is an issue in the test that is not related to this ticket? It looks like it's a quick update to the form class instantiation, though I don't have phpunit running yet to fire the test off locally. I'm not seeing a separate ticket, would that be helpful? This is similar to what I am seeing in some of the ConfigFormBase Tests in core.

diff --git a/docroot/modules/contrib/module_filter/tests/src/Kernel/Form/ModuleFilterSettingsFormTest.php b/docroot/modules/contrib/module_filter/tests/src/Kernel/Form/ModuleFilterSettingsFormTest.php
index f8d62f514..bd5ef7cf6 100644
--- a/docroot/modules/contrib/module_filter/tests/src/Kernel/Form/ModuleFilterSettingsFormTest.php
+++ b/docroot/modules/contrib/module_filter/tests/src/Kernel/Form/ModuleFilterSettingsFormTest.php
@@ -27,7 +27,8 @@ protected function setUp(): void {
parent::setUp();

$this->moduleFilterSettingsForm = new ModuleFilterSettingsForm(
- $this->container->get('config.factory')
+ $this->container->get('config.factory'),
+ $this->container->get('config.typed')
);
}

@wolffereast yes the test failure is unrelated and happens on the main branch. It started when d.o. moved from 10.1 to 10.2 testing. Yes please open a separate issue for that, as it needs fixing. Our gitlab-ci pipeline testing at 10.2 did not flag up the deprecation, that's why it got missed. We need to find out how it could have been detected earlier.

Also note that the Updates page is not covered by any phpunit tests yet, so any change to that page will have to be checked manually, we can't reply on a green test pass.

@jonathan1055 sounds good, I have a ticket up with a quick patch and green tests that should allow this patch to get through once it's on dev.

My first shot at a browser test for the updates page. This patch does not include the fix for this ticket and should fail in testUpdateStatusPageUpdateShowChange. I'm getting odd behavior while testing locally, checking the test bot to see if it agrees or is something is wrong on my side.

Fixed the coding standard errors and added the fix for this issue into the patch to see if the filter change will succeed. I'm getting a failure in testUpdateStatusPageTextFiltering that doesn't make sense to me.
It mimics the behavior in the other tests by updating the text filter and looking to see if the Drupal Core listing has disappeared. Dumping the value of the field shows that the field value matches what was set, and testing that value in the browser behaves as expected. Assuming this fails in that method again I would appreciate it if someone with more experience in writing browser unit tests could take a moment to review.

Excellent work @wolffereast Thank you for adding this new test.

One idea I had about why the test fails - when it filters out, you have $this->waitForNoText('Drupal core'); and the text still seems to be shown. I recently discovered on #3412061: Permissions filter test - waitForNoText() also needs waitForText() that we needed an extra waitForText() in 10.2 to positively wait for something, which was not required before.

You can also add screengrabs in the javascript tests, using

$this->createScreenshot(\Drupal::root() . '/sites/default/files/simpletest/screen1.png');

Then you can see what the test is seeing. Hope that helps. Apologies if you already knew this.

@jonathan1055 Thanks for the review and especially the pointer about the screenshots, that was key to figuring this one out. As an aside, is there a way to dump console errors from a running test? That would have made finding the culprit here much easier.

This was a bit of a rabbit hole. The issue ended up being an undeclared dependency on the once library killing the entire bind on the update status page. Adding that dependency allowed the winnow bind to fire and the tests started clearing. I added the dependency to all libraries that use it.

The show test is correctly failing when I remove the original fix, reapplying the fix allows the test to complete with no issues.

I moved the show test from its own method into the main test method, it looks like tests are combined like this in the core tests I was using as a reference.

I'm clearly doing something wrong trying to push these updates to the merge request so I'm uploading another patch. The patch following naming conventions is the full issue patch, the txt file is the updates to the merge request.

Can the fix be added to the MR.

Before we fix anything, we must understand what caused it. I am not certain it was a core change between 10.1 and 10.2, it could very well have been one of the commits here, given that we did not previously have any test coverage for this page.

I would like to go back through the commit log, and check this. It might make sense to fix the module code in a different way? Or at least it will give us confidence that we know what caused the problem. It may therefore allow a new 2.x release which can be compatible with D9 and is not broken, then we can gracefully turn off D9 support if we really want to.

The issue was introduced in the coding standards update, the anonymous function to lambda swap was likely flagged in one of the automated tools.

commit 0e023524be0d6efd063f68871cede70c23f27b47

    Issue #3338336: Javascript coding standards (uninstall, permissions, update status)

I just spun up a simplytest.me instance on 9.5.11 with module filter 4.1.0 and the no test patch from #6 and have no issues using the filters on the update status page. This patch should fix the issues on 9 as well as 10, the patch in 17 tests cleanly against 9 and 10.

Excellent, thank you @wolffereast for the investigation and info. That is really helpful. I did suspect that it was the coding standards fix.

@smustgrave I will update the MR for this as requested, since @wolffereast mentioned in #17 that their set-up is not quite working for that yet.

MR altered to just have the new correctly running test, as per #17. This will fail on 10.1 and 10.2 on the new test only (hopefully).

The test run on 9.5 will also fail on the machine_name filtering because we dont have a core patch/mr for that on #2895388: Modules uninstall filter does not filter by machine name

The screen grab was useful
https://git.drupalcode.org/issue/module_filter-3413467/-/jobs/647451/art...
It showed 'permission denied' because I had missed adding the extra permission (it was in the patch, but I only added the new test file)

That's better, we now get down to screen grab2 which shows that filter is not working.
Testing in the UI locally the title "Drupal Core" is still shown even when the core item is filtered out, so it will be interesting to see if this is different on the gitlab pipeline. Next commit will add the full fix as per patch #17

Tests pass at 10.1 and 10.2
The screen grabs show that the title 'Drupal Core' is also removed. That's good. I was testing locally on Gin theme (which is based on Claro) but the section titles were not removed. Luckily Claro does do it right.

The pipeline failure at 9.5 is for the uninstall filtering because Core 9.5 has no patch for that.

Ha ha. The test at 9.5 has two failures. The new updates report test failed because it detected 9.5 as 'unsupported' when testing the radio button, because the test was expecting "no result".

That part of the new test could be altered so that it can run at 9.5. I think it is useful to be able to know that this module is still compatible with Core 9.5 until it comes time to do otherwise.

I'm really not going to sweat it if 9.5.x tests are failing.

He he! All green even at 9.5 https://git.drupalcode.org/issue/module_filter-3413467/-/pipelines/78793

If you're confident in the gitlab changes I can merge. Only concern is the gitlab file gets complex enough that it starts to break more.

Yes I am, but thanks for asking. It was a good question. We did break something on the gitlab templates project recently, it was reported 26 mins after the commit, and was reverted within 6 mins. Fjgarlin is quick on the spot. Plus I have been working quite a bit there and know my way round the .gitlab-ci.yml file, so I'm always here to sort it out. Also we now have proper versioning - see #3404175: Adopt (semver) versioning for gitlab_templates, which means the new default is to use a stable proven version, not the latest fixes immediately.

Fixed

It looks like it's working on most of my websites except one site. I'm getting this error in Console:

jquery.min.js?v=3.7.1:2 Uncaught TypeError: Cannot read properties of undefined (reading 'toLowerCase')
at ce.fn.init.val (jquery.min.js?v=3.7.1:2:67761)
at HTMLInputElement. (module_filter.update_status.js?v=10.2.2:118:26)
at HTMLInputElement.dispatch (jquery.min.js?v=3.7.1:2:40035)
at v.handle (jquery.min.js?v=3.7.1:2:38006)

Any idea what could be the issue?

EDIT: It's working now after leaving it alone overnight. Don't know what happened cause I tried clearing the cache and using incognito to test.