Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #0
Problem/Motivation
The test makes sure that there's no name and mail fields for authenticated user but no check for this fields for anonymous user
Proposed resolution
Extend this test to check that this fields present for anonymous
Remaining tasks
create patch ;)
User interface changes
no
API changes
no
Comment | File | Size | Author |
---|---|---|---|
#37 | interdiff-2212869-35-37.txt | 3.18 KB | naveenvalecha |
#37 | 2212869-37.patch | 2.76 KB | naveenvalecha |
Comments
Comment #1
premgangwar CreditAttribution: premgangwar commentedComment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedI added the test to check the presence of name and email field for anonymous user in the same test
Comment #3
andypostComment #4
GrimreaperHello,
I just test the patch and it is OK.
I saw in the debug message, the case authenticated and the case authenticated and the results are the same as the results browsing the sites.
Is there any other thing that need testing ?
Comment #5
andypostShould be "One-line summary, ending in a period (.)"
See https://drupal.org/node/1354#order
and https://drupal.org/node/325974
trailing white-space, and unneeded line
"Leave an empty line between end of method and end of class definition" see https://drupal.org/node/608152
Comment #6
GrimreaperI remade the patch of comment #2. Is it good now ?
@andypost : on the documentation pages you mentionned, is there a problem ? Because I read "leave empty line ...." and in the exemples I don't see empty lines.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedI generated a new patch taking care of the coding style
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedChanged the summary to one line summary
Removed empty space
One line between end of method and end of class definition
Comment #9
andypostAwesome!
@Grimreaper hm, that should be fixed, please edit this page
Comment #10
Grimreaper@andypost : ok, I see, when I edit the documentation page, there are blank lines which are removed when seeing the page : https://drupal.org/node/325974
What should be fixed this issue or the documentation page?
Comment #11
andypost@Grimreaper Suppose you mean https://drupal.org/node/608152#indenting that's a really bug!
Comment #12
andypostFiled documentation bug #2218915: Code examples removes blank lines
Comment #13
karan.poddar CreditAttribution: karan.poddar commentedApplied the patch and it works perfect.
Comment #15
visabhishek CreditAttribution: visabhishek commented7: added_check_for_field_name_and_email_for_anonymous_user-2212869-7.patch queued for re-testing.
Comment #16
andypostComment #17
alexpottNot 100% sure that adding this to a test method called
testContactSiteWideTextfieldsLoggedInTestCase
in a file calledContactAuthenticatedUserTest
makes sense. Plus the name and email fields are tested every timeContactSitewideTest::submitContact()
is called when using the anon user. If we want to add the assertion for the fields let's add it toContactSitewideTest::testSiteWideContact()
when it's is using the anon user.Comment #18
leevingo CreditAttribution: leevingo commentedMoved the test to ContactSitewideTest::testSiteWideContact() and removed ContactAuthenticatedUserTest.php.
Comment #20
leevingo CreditAttribution: leevingo commentedUpdated the patch that failed testing.
Comment #22
subhojit777Comment #24
subhojit777Comment #25
subhojit777Comment #27
Mile23Generally looks good but needs a re-roll. We can leave this at 8.1.x since it only changes tests.
Comment #28
kostyashupenkoReroll of #25.
ContactAuthenticatedUserTest.php was removed in 8.1.x
Comment #29
andypostPlease remove "Now" or s/Ensure/ensure
Comment #30
poojasharmaece CreditAttribution: poojasharmaece as a volunteer and commentedPatch updated as per #29
Comment #32
kellyimagined CreditAttribution: kellyimagined commentedUpdated patch for case in #29
Comment #35
ZeiP CreditAttribution: ZeiP as a volunteer commentedRe-roll against 8.3.x. IMO the comment changes in patch 30 were better, so I re-rolled that one.
Comment #36
jibranTest looks good but do we need
ContactAuthenticatedUserTest
after this?Comment #37
naveenvalecha#36,
Nice question Jibran. We don't need that as we are testing the same stuff in testSiteWideContact. We could easily remove that class as that's not a base class and there wouldn't be any BC problem of removing it.
//Naveen
Comment #38
jibranThanks!
Comment #40
catchCommitted/pushed to 8.4.x, thanks!