Account created on 9 May 2022, about 2 years ago
  • Engineer Drupal - Backend at QED42Β 
#

Merge Requests

More

Recent comments

Observed test failures, found there are some PHP coding standard related test failures , fixed those.

pipeline passed successfully & MR is mergeable now.

Replied on the comment, as there are not any changes done, so put back to prev RTBC status

Applied the suggestions, Moved test coverage code from MediaSourceImageTest to MediaOverviewPageTest

Applied mentioned suggestion to the MR.

Please review, moving NR

Addressed the test coverage for DialogRenderer::getTitleAsStringable()

Please review , moving NR

Addressed the feedback , Rebased the MR with latest code & pipeline pass successfully.

keeping it in RTBC as there is only minor change done.

Applied the mentioned suggestions

Please review, moving NR.

Applied the mentioned suggestions.

Please review, moving NR

Applied the mentioned suggestions.

Please review, moving NR

I have applied the suggestion, there was bit confusion earlier thought of that specific 'll handle in other followup thanks for clarity got your concerns as well.

Please review, moving NR.

@FeyP There is follow-up already created for it, may be we can keep here only minimal changes & cover these changes other follow up. Any thoughts/concerns over it?

Applied the mentioned suggestions.

Please review, moving NR

Thanks for reviewing it, Applied the mentioned suggestions

Please review , moving NR

MR conflicts are visible on MR, addressed those

Rebased MR with latest code, seems working fine.

Addressed the mentioned changes & updated the MR

Please review , moving NR

@FeyP Thanks for reviewing, there is already displaying conflicts errors on MR with error message: need to resolve conflicts locally, due to which I have resolved the conflicts & updated MR as well

Addressed the mentioned changes & updated the MR

Please review , moving NR

This issue is specifically related to the remove the superuser dependency code, so there is only one file detect where with superuser code exist:
protected bool $usesSuperUserAccessPolicy = TRUE;

After removing the superuser code, this further leads to test failure for uid attribute in File::create() API, so updated uid variable only in the : file/tests/src/Kernel/SaveTest.php

setting back to RTBC

Addressed the mentioned suggestions & removed the newly created test file as test code moved to existing suggested test file.

Please review , moving NR

I quickly tried to replicate title missing issue on local & able to recall it, I have fixed that title missing issue by adding 'access content' permission, however forgot to remove $this->drupalPlaceBlock('page_title_block'); from MR.

So Removed the unrelated change , Please review , moving NR.

Removed the superuser dependency code & added respective permissions.

Please review, move to NR.

It was quite challenging to fix it as compare to the other similar child issues, as well interesting to make it working with required changes.

Addressed the mentioned changes superuser dependency code needs to remove & added respective permissions.

Please review, move NR.

Yeah, Thanks @b_sharpe you got it right. , however for the clarity, @nicxvan I have updated comment.

Ahh, even I tried to replicate on local to work on it, for me test pass on local.

I have tested the issue & it's replicable on local

Test both test scenario pass & fail & attached the screenshots for reference:

A. Keep the newly added test code on local , but not fixation code: issue replicable (test fail)

B. Keep the newly added test code on local , also added fixation code: issue fixed (test pass)

RTBC++1, also reviewed test code , written with minimal required changes

Addressed the superuser code needs to remove & added relevant code for test.

Please review MR 8651, move NR

Addressed the superuser code needs to remove & added respective code.

Please review MR 8651, move NR

Addressed the superuser code needs to remove & added respective permissions.

Please review MR 8643, move NR

pooja_sharma β†’ changed the visibility of the branch 3439832-fix-content-moderation-test to hidden.

Addressed the superuser code needs to remove & added respective permissions.

Please review MR 8637, move NR

Addressed changes in issue summary, removed the superuser code & added respective permissions.

Please review MR 8628, move NR

Thanks for review, recent code changes lead to pass test case in every scenario which is not correct , so revert back changes.

Alt attribute field is bind with image media entity if alt attribute not attach with empty string then it 'll not be use of adding this specific test case as in manual test if we keep empty "attribute field" then alt tag not render on FE which leads to issue.

PLease review, moving to NR

Added test coverage in a distinct test file to assess specific scenario.

Please review, move NR

Used renderText() to fix the issue, it does not lead cache regression in views/tests/src/Kernel/Plugin/RowRenderCacheTest.php

Please review, Moving to NR.

I have observed the test failures, may be we can use:

"commentAccepted/commentEligible/commentAllowed" in place of "Commentable"

As "commentAccepted/commentEligible/commentAllowed" word would be more closer/relative to the "Commentable" word to fix cspell test failures.

Left suggestion not updating MR, as most of the work done already.

facing issue while rebasing on local it seems source branch somehow corrupted

So I have create new branch + MR 8569, rebased the branch along with addressed mentioned changes

I have not idea how to move comments added on existiing MR

Please review , move NR

Addressed the mentioned feedbacks & rebased the MR

Please review, move NR.

Concluded that once reference issue fixed(i.e part of another issue), then we can resume on it for "Remove extra line in MediaTest" and can be sure after fixing issue whether it is safe to "Remove extra line in MediaTest" for now

Removed the @todo Views should expect and store a leading /. comments

PLease review, moving to NR

@bbrala can you please confirm , as per description mentioned need to remove the "access content" content permission, however still test fails, that means need to work in the code level to fix this issue , is there any followup that can be check what changes occurs that code level that not lead to pass the pass yet ?

I have observed in core files (for eg. core/includes/errors.inc) instead of using translation t() can be use FormattableMarkup() (even same mentioned in MR where this issue trace out) , So replaced the t() with FormattableMarkup().

Please review , moving NR.

Production build 0.69.0 2024