Fix Field UI tests that rely on UID1's super user behavior

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

Applied necessary changes to fix these tests.

Left some comments but appears to have test failures.

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

Please review MR 8637, move NR

Left a comment on MR.

Left a comment for suggestion, Please review, move NR

See response

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.

Looks much better thanks.

The FieldUIRouteTest is not accessing content so why does the user need 'access content' permission? And EntityReferenceXSSTest is not editing content so it shouldn't need 'edit own article content'. Setting to needs work for checking the permissions.

Addressed the mentioned changes & updated the MR

Please review , moving NR

Addressed the mentioned changes & updated the MR

Please review , moving NR

Thanks for working on this issue. I think there are even more permissions that can be removed. Left some comments inline.

After conferring with @smustgrave, I filed #3460654 for a small performance optimization as a follow-up issue. For now postponed on this one so that we can get this done first.

Thanks for reviewing it, Applied the mentioned suggestions

Please review , moving NR

Thanks @pooja_sharma. As far as I can see, the login in FieldUIRouteTest is still done in the setup method. Can we please move this to testFieldUIRoutes(), because I don't think it is needed for the other test in that file? Or is there a specific reason why you think it shouldn't be done?

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

There is follow-up already created for it

No, the follow-up is to merge the to test methods, which is definitely out of scope for this issue.

But my understanding is, also from looking at the reviews by others in this issue and other similar issues, who are a part of the same meta, that the intention is to use minimal required permissions only. And the other test in FieldUIRouteTest doesn't need any permissions at all to pass. This is also underlined by an earlier review comment from @smustgrave "The move to setup() is probably fine since this is the only test in this file." In this case we have two tests, so it is different. So I think we should move the login method into the test where it is needed as part of this issue so that the other test runs without any elevated permissions. But if you want, I can ask for a second opinion. Or wait for others to RTBC this.

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.

Thanks @pooja_sharma, the final state is exactly what I asked for.

Alright, I verified yesterday that the permissions now in use are the minimum required permissions. I also verified that previous review comments had been addressed. Then I looked specifically at the places where we assert that something is not on the page to make sure that this doesn't pass now just because we are missing some permissions now. Luckily those very only few places, because we mostly only assert what's there. So the code review looks good now.

There are no super user flags left to be removed within tests in the scope of this issue. The tests pass with the changes. Rest of the pipeline also looks good.

The issue is scoped appropriately. I'm gonna move the files from the remaining tasks section to the proposed resolution and then add the MR to commit, because we have multiple. Then the IS is fine. The issue title could be improved, but is okay as a git commit message and it makes sense to keep this similar to the other issues already committed as part of this meta. Follow up for performance optimization is filed.

Thanks again @pooja_sharma for staying on top of this and for understanding my concerns.

I think the issue is RTBC! „🎉

FeyP → changed the visibility of the branch 3439835-fix-field-ui to hidden.

nod_ → closed merge request !8637

Committed and pushed 139bf72968 to 11.x and 35ee84ae5f to 11.0.x and 554f305ce5 to 10.4.x and 911c200988 to 10.3.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.