Need to test:
Book html exports.
Attempting to export a book page in an unknown format.
Attempting to view exported book pages as an unprivileged user.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

paulmckibben’s picture

Component: tests » book.module
Assigned: Unassigned » paulmckibben

As part of the DrupalCon DC code sprint, I'm writing my very first test. Please be lenient on me! :)

paulmckibben’s picture

Component: book.module » theme system
Status: Active » Needs review
FileSize
3.42 KB

The attached patch adds a new test method, testBookExport(), to the existing BookTestCase class. This method:
- creates a book with a hierarchy of pages
- verifies the book can be exported to html
- verifies that the book cannot be exported to another format (tested using path book/export/foobar/$nid)
- verifies that a user without the 'access printer-friendly version' privilege cannot export the book to html

mr.baileys’s picture

Thanks for this patch!

Reviewing the patch, here are my comments:

  1. Comments need to be re-formatted a bit, as the coding guidelines state:

    Non-documentation comments should use capitalized sentences with punctuation. Sentences should be separated by single spaces. All caps are used in comments only when referencing constants, for example TRUE. Comments should be on a separate line immediately before the code line or block they reference.

  2. When it comes to concatenating strings:

    Always use a space between the dot and the concatenated parts to improve readability.

  3. Probably no need to create an extra user for the following test:
    +    //make sure a user without the right privilege cannot view printer-friendly version
    +    $this->drupalCreateUser();
    +    $this->drupalGet('book/export/html/'.$this->book->nid);
    +    $this->assertResponse('403',t('Unprivileged user properly forbidden.'));
    

    You can just logout and use the anonymous user (saves some cycles when running tests). I made the same mistake recently when working on one of my first tests.

  4. You'll need to pay attention to the spacing between arguments of a function call (one space after an argument), see the coding standards.
  5. Your test goes straight to the export page for a book. May be we should load the book, and then use $this->clickLink(...) to get to the printer-friendly page?
  6. Not sure about the wording of this assertion:
    $this->assertText($node->title, t('Printer friendly title found.'));
    I think something like "Node title found in printer friendly version" is better (as there is no such thing as a printer friendlly title for nodes). Same goes for the "printer friendly body".
mr.baileys’s picture

Status: Needs review » Needs work

Back to CNW for the coding standards items (#1, #2 and #4).

paulmckibben’s picture

Component: theme system » book.module
FileSize
3.71 KB

Thanks for your review and explanation, and I apologize for the newbie mistake of not reading the coding standards.

I have attached a new patch with your suggested changes.

Cheers,
Paul

paulmckibben’s picture

Status: Needs work » Needs review

Changing to "code needs review" - hopefully for the last time. :)

Status: Needs review » Needs work

The last submitted patch failed testing.

paulmckibben’s picture

Issue tags: +drupalconsprint

I know it's been awhile since DrupalCon DC. I'm here in the Paris code sprint and am working on it again.

paulmckibben’s picture

Status: Needs work » Needs review
FileSize
4.51 KB

Fields in core, and access to the node body, changed, which broke the previous patch. Here's a new patch.

Francewhoa’s picture

Confirming that patch in #9 can be apply without error. But could someone give me direction on how to test book html exports. I searched Drupal 7 admin & settings but found nothing. Do I need to install a contributed module?

Patch applied to Drupal 7 (September 9, 2009 - 05:11) fresh install.

paulmckibben’s picture

Thanks, Onopoc,

An example of an export is the "Printer-friendly version" link. The "Printer-friendly version" link exports the book as a single HTML page. You will see that it points to the path: book/export/html/[nid]

As I understand the issue, the new test in this patch needs to make sure:
1. book/export/html/[nid] works for permitted roles
2. book/export/html/[nid] is blocked for roles that do not have permission
3. book/export/something-else/[nid] does not work

This new patch only adds test code, so there will be no change in the GUI. The reviewer will need to verify that the new test code actually achieves the above requirements.

Cheers,
Paul

Francewhoa’s picture

Thanks for clarifying Paul.

Francewhoa’s picture

FileSize
50.24 KB
114.21 KB

Confirming that patch in #9 works. Thanks pmckibben

The good news. I tested all that is describe in #11. And it works fine.

As for the bad news. On the Printer-friendly version The link Add new comment should be hidden. Steps to reproduce the usability issue:
1. Create a book.
2. Add a comment to any page of the book.
3. View the book. Then click on Printer-friendly version button.
4. The issue is on this page. The HTML link Add new comment is display on this page. Expected result is to hide this link. Usually a Printer-friendly version is strip of all navigation link/button. I don't see any point for end user to print Add new comment links. Find attached screenshots to clarify. What do you think?

Tested with.
-Drupal 7 (September 9, 2009 - 05:11) fresh install.
-Garland theme.
-Three different roles :User #1 (Admin), Content Author, Anonymous. With various permissions.

Francewhoa’s picture

Status: Needs review » Needs work

Forgot to change status to 'needs work'.

paulmckibben’s picture

Status: Needs work » Needs review

Onopoc,

Thanks for testing this patch, and I'm glad that you have found that it meets the requested functionality.

Since this issue is for adding a test to the test suite, and has nothing to do with the user interface, I think this should be considered "RTBC", and a new usability issue should be written for the "Add new comment" link. That way, this test can be committed to core, and a new issue will be in place to track the problem you found with the "Add new comment" link.

Would that be ok with you?

Thanks,
Paul

Francewhoa’s picture

@Paul: Thanks for pointing this out. I'll let someone review the test for the test suite. I don't know how to do that yet.

Ok with me about the issue. I have opened a new issue at http://drupal.org/node/583576

Status: Needs review » Needs work

The last submitted patch failed testing.

sun.core’s picture

Priority: Critical » Normal

Tests don't qualify as critical.

paulmckibben’s picture

Status: Needs work » Needs review
FileSize
4.73 KB

In grand DrupalCon code sprint tradition, I'm back on this! Here's a new patch correcting the test failures of the previous patch.

mtift’s picture

FileSize
15.45 KB

I applied the patch, ran it, and everything appears to be working correctly. I simply added a doc block to explain the function ('Tests book export ("printer-friendly version") functionality'). If I formatted my patch incorrectly I apologize.

paulmckibben’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.58 KB

Re-generating mtift's patch (he patched the patch file, not the code).
Per our discussion, we both believe this is RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, this is a great clean-up patch, folks! :D

Committed to HEAD!

Status: Fixed » Closed (fixed)
Issue tags: -drupalconsprint

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