🇮🇳India @shalini_jha

I have replicate this issue. i have checked both module & recipe and if the title is multi line then it is overlapping , after this changes verified that the multi line project title is showing properly. +RTBC

I believe this failure is not related to the changes I made. I am moving this for review. Please take a look and let me know if this aligns with the expectations of the ticket or if any updates are required.

I have attempted to add a check to skip the Package Manager install readiness checks specifically for recipes. However, I am unsure if this is causing the Nightwatch failure

@oily Thanks for your review . Based on the previous issue, i have tried to show the test coverage for vertical tab summary text is appended to all vertical tabs label. Additionally, I have implemented checks to confirm that clicking on any tab opens the corresponding form. we can wait for review whether we need to update this.

Rebased & fixed conflicts . Moving back to review.

shalini_jha → changed the visibility of the branch 2889486-has-taxonomy-term-10.3 to hidden.

I have reviewed the related issue and added test coverage to address the mentioned points. I have attempted to test all the scenarios outlined in the issue. Could you please review this and let me know if there’s anything that needs to be updated or improved?

shalini_jha → made their first commit to this issue’s fork.

shalini_jha → made their first commit to this issue’s fork.

shalini_jha → made their first commit to this issue’s fork.

Updated the code.

I Have checked the merge conflict and re base this MR & fixed merge conflict.according to #20 so moving this back to NR.

shalini_jha → made their first commit to this issue’s fork.

I Have addressed all the mentioned feedback, and moving this for your review.

Thank your for the review , i am working on feedback.

I Have checked the pipeline failure & updated the existing code for mail test. now pipeline is green and other feedback is addressed. so moving this NR. Kindly review.

I am checking for failing test.

Thank you for your review. Since this issue is related to the warning and the test already displays the warning message without any UI changes, is it still necessary to include a screenshot for this?

@chhavi.sharma I have reviewed the pipeline failure : https://git.drupalcode.org/issue/drupal-3485084/-/jobs/3674676
and it seems like a random failure not relevant with the changes in MR, try to re-run it will fix. hope that help.

Thank you, @Pameeela, for your review and guidance. I have updated the code accordingly and tested it locally. I am now moving this forward for your review. Please let me know if there are any additional changes required.

shalini_jha → made their first commit to this issue’s fork.

I have removed the 'Skip this step' action button from the second step (Site name settings) and tested the changes. I have created a merge request, and the pipeline is green. Moving this to 'Needs Review'. Kindly review and let me know if any further changes are required.

shalini_jha → made their first commit to this issue’s fork.

I have addressed the issue with the search result count by correcting the grammar for displaying "1 Result" when there is only one result. Additionally, I have added a test to verify this change. I am moving this to "Needs Review." Please review the changes and let me know if they are appropriate or if any further modifications are needed.

Currently, when no results are found, it displays "0 Results." Do we still need this text, given that "No modules found" is already displayed?

shalini_jha → created an issue.

I have added test coverage for this, following the approach mentioned in #48. Moving this to NR for review. Kindly review the changes.

I have been able to replicate and review all the comments. I am working on the test.

I Have addressed the feedback, moving this NR. kindly review.

shalini_jha → made their first commit to this issue’s fork.

Re based & fixed merge conflicts Also Tweak logic for hiding the icon.

Re based & fixed merge conflicts.

Thank you for your review and feedback,
re based & fixed conflict.

@sumeshsr AS discussed with you, i have checked the pipeline failure , it is just a random failure , you can fix this by re-run pipeline or re run failed job. it will fix.

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