I have moved the changes from ProjectBrowserUiTestJsonApi::testMultiplePlugins to ProjectBrowserUiTest::testMultiplePlugins. I verified the output from the old testMultiplePlugins function and added the same flow to the ProjectBrowserUiTest::testMultiplePlugins.
Marking this as "Needs Review." Please review and let me know if this approach makes sense.
Rebased and resolved conflicts. Test changes in tests/src/FunctionalJavascript/ProjectBrowserUiTestJsonApi.php are still pending and need to be moved to ProjectBrowserUiTest::testMultiplePlugins.
shalini_jha → made their first commit to this issue’s fork.
I have added the '_blank' target to the "Learn more" link to ensure it opens in a new tab. Moving this to NR for review. Kindly review the changes.
@sayan_k_dutta are you working on this ?
I was able to replicate this warning, and it seems to occur in scenarios where certain specific modules are involved. For example, if you search for OpenWeather, the warning is reproduced.
Upon further investigation, it appears that in the response from drupalorg_jsonapi, the author name is null. This causes the author name key to also be null, leading to the warning.
@narendrar Thank you for your valuable suggestion. I have removed the test case as per your feedback and made the necessary adjustments. I have validated that the functionality is working correctly after these changes. I am moving this to 'Needs Review'. Also, I noticed that eslint is failing, but I believe that issue is not related to the changes made here.
I have reviewed the suggestions provided in this MR and tested the non-convertible value case. It appears that the previous solution didn't work as expected. To address this, I made some adjustments to the $text logic to handle both scenarios correctly.
Additionally, I’ve added an extra assertion to account for the object case, ensuring that both scenarios are now properly handled. Please take a look and let me know if this approach makes sense.
Moving this to NR.
I have reviewed all the feedback and implemented the necessary changes accordingly. After applying the updates, I re-ran the pipeline, and it is working as expected. I have also verified the empty string case, and it is functioning correctly without any issues. Moving this forward for your review. Kindly review.
I included the project icon in the existing condition to ensure it is displayed only when the module usage is not zero. This hides the project icon when the module usage is zero. However, the pipeline is failing with an ESLint error. Even after reverting the code, the ESLint failure persists, suggesting that the issue might not be related to this specific code change. I am unsure about the root cause of the failure and am moving this to NR to seek guidance on resolving the pipeline issue.
I am able to replicate this issue , working on it.
I’ve updated the test to expect a cache get count of 115 instead of 116. After debugging, I found that getCacheGetCount returned 115, which is different from the expected 116. Since I’m new to this type numbers count test, I’m not sure about the cause of the discrepancy, so I’ve adjusted the test based on what I observed and run the test locally and it is passed. I would appreciate any guidance on whether this change is correct, or if further adjustments are needed.
Moving this to NR, Kindly review.
I have run this locally and this failing same as mentioned #17, i am working on it.
The method name mergeCommands was chosen to align with the existing structure of the AjaxResponse class, which focuses on handling and manipulating commands, as seen in methods like addCommand and getCommands. Keeping the name consistent with the rest of the class felt logical. However, if there’s a preference to rename it to mergeResponse for better clarity or to better reflect its purpose, I’m open to making that change. Let me know your thoughts!
I re-ran the failed job, and it passed. It seems to be a random test failure. I am verifying this locally to confirm.
I have tried to replicate this issue locally. Can the patch be added to the MR?
shalini_jha → made their first commit to this issue’s fork.
I have rebased and resolved the conflicts in the MR. Additionally, I have updated the list view case as part of the changes. I realized I had missed updating the second instance of the same code earlier, which was highlighted in the feedback. This has now been corrected in both places. Kindly review the updated changes.
"The pipeline issue has been resolved.
@Sayan_k_dutta, sometimes random tests fail, which are unrelated to our changes. In such cases, you can re-run the failing job or restart the pipeline by rebasing your MR or using the 'Run Pipeline' option. In most cases, this resolves the pipeline issues."
Re base & fixed conflict.
I am checking for test failure in this MR.
I have addressed the issue related to special characters by implementing the necessary fixes on both the project list page and the project detail modal. The issue has now been resolved, ensuring that special characters are handled correctly in both areas. Kindly review the changes.
project list page
→
project detail modal
https://www.drupal.org/files/issues/2024-12-04/Screenshot%20from%202024-... →
Addressed the feedback for the install case as well. After applying the same approach for counting, the count is now displayed correctly, such as '590,949 Active Installs'. Kindly review.
Thank you, @narendrar, for your review and guidance. I have addressed the feedback and updated the test coverage accordingly. Moving this back to NR for your review. Kindly take a look at your convenience.
I am able to replicate this issue .
Sure , let me check.
I have reviewed the feedback mentioned in #20 and have tried to update the implementation accordingly, following the formatPlural same as for install case. Since the numberFormatter is not needed in this case, I have removed it. The pipeline has passed successfully, so I am moving this back to 'Needs Review.' Kindly review the changes.
shalini_jha → made their first commit to this issue’s fork.
AS bot reported issue is fixed , and rebased the MR, so Moving this Back to NR. Kindly review.
I have reviewed the recent changes and found that the removal of the messenger service is incorrect because it is used in addStatus(). This caused the test to fail. Additionally, after updating the type hint, the pipeline failed. To resolve this, I first fixed the PHPStan issues, regenerated the baseline, and addressed the test coverage issues related to addStatus(). The pipeline is now fixed. As mentioned in #12, I am moving the project back to "Needs Review."
I missed noticing the spacing issues in the initial work of this MR. I have now addressed all comments on the MR. The pipeline is green, so moving this back to 'Needs Review.' Kindly review
I checked the pipeline failure issue and added return type hint & regenerated the Baseline as mentioned in issue summary. so pipeline is green now.
Moving this to NR, Kindly review.
shalini_jha → made their first commit to this issue’s fork.
Rebased and resolved the conflicts & pipeline is also green. But two additional commands were added, so need to check the documentation.
shalini_jha → made their first commit to this issue’s fork.
I searched for hard coded instances of the path /admin/modules/browse in the Svelte code. It appears that all the hard coded URLs have been addressed, and I could only find one instance of this URL, which has already been covered in this ticket. : https://www.drupal.org/project/project_browser/issues/3476818 📌 [PP-1] Use actual page URL instead of hardcoding the query string in unlock URL Active
The test coverage is not failing in either scenario. After installing the Project Browser Devel module in test coverage , the problem is no longer replicating. I have also attempted to add a functional test in testInstallation, but it is not showing any failing tests in the same way.
Please Let me know if i am missing something for this test.
I Have tried to add test coverage for now method.Moving this to NR , Kindly review & let me know if any updates are needed.
Re run the random failed pipeline , now its green . AS i have just re base & resolved conflict. so moving this Back to RTBC.
shalini_jha → made their first commit to this issue’s fork.
I have checked this issue and issue summary. By calling the newly added toLink() method, I was able to get the link object as expected. I also verified the added test coverage and did not find any additional tags for this issue. I have rebased the MR since it was significantly behind, and the pipeline is now green. Moving this MR for review. Kindly review.
shalini_jha → made their first commit to this issue’s fork.
I Have checked the MR and fifth select(Sort by) list style is same as other select list. and blue outline is also showing for active case same as other filters.
Moving this +RTBC
sure
Thank you for the review, @narendrar.
While the single tag invalidation seemed to work during my initial testing, I have implemented your suggested change to invalidate the cache.
I have verified the steps again, and this is working as expected.
Please review the updated code and let me know if further changes are required.
Thanks for your review . i am working on it.
shalini_jha → created an issue.
I have checked the pipeline failure, replicated it locally, and fixed the issue. The pipeline is now green. Moving this to "Needs Review." Kindly review.
checking for test failures.
Fixed the conflict and rebased MR, seems like some random test failure.
shalini_jha → made their first commit to this issue’s fork.
I have added test coverage for showing this issue, pipeline is green so moving this NR.
Kindly review.
Failing test :
There was 1 failure:
1) Drupal\Tests\workspaces\Functional\PathWorkspacesTest::testPathAliasesWithTranslation
Behat\Mink\Exception\ExpectationException: Current response status code is 404, but 200 expected.
/var/www/html/vendor/behat/mink/src/WebAssert.php:888
/var/www/html/vendor/behat/mink/src/WebAssert.php:145
/var/www/html/core/modules/workspaces/tests/src/Functional/PathWorkspacesTest.php:345
/var/www/html/core/modules/workspaces/tests/src/Functional/PathWorkspacesTest.php:331
I have replicated this issue, and the merge request fixes it. I will work for the test coverage.
Moving this for NR.
I have checked this MR and found some conflicts, which I have addressed. The pipeline was failing due to an unknown word. After reviewing the existing test method, I added it to the cspell:ignore list, and the pipeline passed successfully. After that, I revalidated the test coverage, and it is working as expected. Let me know if you need any further adjustments!
Kindly review.
Failing test :
There was 1 failure:
1) Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testExtraFields
Behat\Mink\Exception\ExpectationException: An element matching css ".block-extra-field-blocknodebundle-with-section-fieldlayout-builder-test-empty" appears on this page, but it should not.
/var/www/html/vendor/behat/mink/src/WebAssert.php:888
/var/www/html/vendor/behat/mink/src/WebAssert.php:492
/var/www/html/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php:505
moving this for NR.
Thanks
I have address the feedback, also verified with another method no type cast is added, also again re ran the test and its working as expected, pipeline is also green so moving this for NR.
Kindly review.
I was able to replicate the issue and after debugging, I found that the data was not updating on module installation due to caching. To resolve this, I invalidated the configuration cache tag in the hook_install(), and it is working as expected now. Moving forward for NR. Let me know if anything else needs to be updated regarding this.
Kindly review.
shalini_jha → changed the visibility of the branch 3479924-project-browser-devel to active.
shalini_jha → changed the visibility of the branch 3479924-project-browser-devel to hidden.
shalini_jha → made their first commit to this issue’s fork.
I Have addressed the feedback also checked the https://3v4l.org/UuKsj and re-ran the test and its is working as expected. pipeline is also green so moving this NR.
Kindly review.
shalini_jha → made their first commit to this issue’s fork.
@ankitv18, thanks for your review. However, I started with the Project Browser issue because I wanted to begin with a simple problem to understand the flow better."
@ankitv18, thanks for your review. However, I started with the Project Browser issue because I wanted to begin with a simple problem to understand the flow better."
Removed the legacy code from DrupalDotOrgJsonApi.php. and raised a MR for this .moving this NR. Kindly review.
shalini_jha → made their first commit to this issue’s fork.
I have updated the route URL for drupal-org-proxy to /project-browser/data, and have made the necessary updates across all relevant occurrences. I have also generated the build to reflect these changes and raised a MR for review. Could you please review the changes and let me know if I have missed anything that needs to be updated?
Fixed issue reported by bot but seems some phpstan failure.
shalini_jha → made their first commit to this issue’s fork.
I have consolidated both test methods into a single one and tested it. It is working as expected. Moving this to NR for review. Kindly review.
Failing test :
PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.12
Configuration: /var/www/html/core/phpunit.xml.dist
F 1 / 1 (100%)
Time: 00:04.851, Memory: 186.00 MB
There was 1 failure:
1) Drupal\Tests\Core\Controller\TitleResolverTest::testStaticTitleWithArguments
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
Drupal\Core\StringTranslation\TranslatableMarkup Object (
'string' => 'static title @test @test2'
'arguments' => Array (
- '@test' => 'override value'
+ '@test' => 'value'
'%test' => 'override value'
'@test2' => 'value2'
)
/var/www/html/core/tests/Drupal/Tests/Core/Controller/TitleResolverTest.php:153
I have checked the related issues.
I will look into the pipeline failure issues
I reviewed the merge request and found some conflicts, so I rebased and resolved them. The pipeline has also passed successfully, so I’m moving it to "Needs Review."
Thank you for your review and feedback; I'll incorporate this. I've updated the issue summary and added a draft mode
change request →
for review.
Please take a look and let me know if there’s anything further needed on my end.
shalini_jha → made their first commit to this issue’s fork.
Moving this for Review.
Hi,
I was able to replicate the issue and reviewed the existing fix in this MR. The solution appears correct, and functionality works as expected based on my testing. To validate this, I have tried to add test coverage in the FilterStringTest class, aiming to demonstrate both the issue and its resolution.
I’ve submitted a new MR against 11.x. Since there was an existing MR on 10, I’ve hidden that one to avoid any confusion. I wasn’t sure about the status of the existing MR, Apologies if you would have preferred updating the existing MR.
Please review when possible, and let me know if there’s anything further I should address.
Updated issue summary also.
Thank you!
shalini_jha → changed the visibility of the branch 3055152-views-stringfilter-doesnt to hidden.
shalini_jha → changed the visibility of the branch 3055152-views-stringfilter-doesnt-11x to active.
shalini_jha → changed the visibility of the branch 3055152-views-stringfilter-doesnt-11x to hidden.