Account created on 7 May 2021, over 3 years ago
  • Engineer-drupal backend at QED42 
#

Merge Requests

More

Recent comments

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Rebased and resolved conflicts. Test changes in tests/src/FunctionalJavascript/ProjectBrowserUiTestJsonApi.php are still pending and need to be moved to ProjectBrowserUiTest::testMultiplePlugins.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

@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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

I have run this locally and this failing same as mentioned #17, i am working on it.

🇮🇳India shalini_jha

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!

🇮🇳India shalini_jha

I re-ran the failed job, and it passed. It seems to be a random test failure. I am verifying this locally to confirm.

🇮🇳India shalini_jha

I have tried to replicate this issue locally. Can the patch be added to the MR?

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

"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."

🇮🇳India shalini_jha

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-...

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

AS bot reported issue is fixed , and rebased the MR, so Moving this Back to NR. Kindly review.

🇮🇳India shalini_jha

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."

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Rebased and resolved the conflicts & pipeline is also green. But two additional commands were added, so need to check the documentation.

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

I Have tried to add test coverage for now method.Moving this to NR , Kindly review & let me know if any updates are needed.

🇮🇳India shalini_jha

Re run the random failed pipeline , now its green . AS i have just re base & resolved conflict. so moving this Back to RTBC.

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Fixed the conflict and rebased MR, seems like some random test failure.

🇮🇳India shalini_jha

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
🇮🇳India shalini_jha

I have replicated this issue, and the merge request fixes it. I will work for the test coverage.

🇮🇳India shalini_jha

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
🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

shalini_jha changed the visibility of the branch 3479924-project-browser-devel to active.

🇮🇳India shalini_jha

shalini_jha changed the visibility of the branch 3479924-project-browser-devel to hidden.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

@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."

🇮🇳India shalini_jha

@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."

🇮🇳India shalini_jha

Removed the legacy code from DrupalDotOrgJsonApi.php. and raised a MR for this .moving this NR. Kindly review.

🇮🇳India shalini_jha

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?

🇮🇳India shalini_jha

Fixed issue reported by bot but seems some phpstan failure.

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

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
🇮🇳India shalini_jha

I will look into the pipeline failure issues

🇮🇳India shalini_jha

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."

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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!

🇮🇳India shalini_jha

shalini_jha changed the visibility of the branch 3055152-views-stringfilter-doesnt to hidden.

🇮🇳India shalini_jha

shalini_jha changed the visibility of the branch 3055152-views-stringfilter-doesnt-11x to active.

🇮🇳India shalini_jha

shalini_jha changed the visibility of the branch 3055152-views-stringfilter-doesnt-11x to hidden.

Production build 0.71.5 2024