Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto print login cards #32312

Merged
merged 1 commit into from Dec 6, 2019
Merged

Auto print login cards #32312

merged 1 commit into from Dec 6, 2019

Conversation

dmcavoy
Copy link
Contributor

@dmcavoy dmcavoy commented Dec 5, 2019

Description

Call print on the login cards page but it would print before the secret pictures loaded so we removed the print command thinking users could just print it themselves.

We found out that wasn't working great for users so we decided to bring back calling print for the user but after waiting for the page to load fully. So this calls print once all parts of the login card page is loaded.

Screen Shot 2019-12-05 at 2 04 17 PM

Links

Testing story

What is the best way to test this? A UI test?

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@dmcavoy dmcavoy requested a review from a team as a code owner December 5, 2019 19:10
@dmcavoy dmcavoy changed the base branch from staging to staging-next December 5, 2019 19:11
Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I am legit not sure how to fully unit test (or even UI test) print behavior in the browser. A quick search isn't bringing up any ideas either.

Instead, the first thing that came to mind is the Skin and Wrap the API approach from Working Effectively With Legacy Code, (Feathers) Chapter 15 "My Application Is All API Calls:"

We end up with applications that look like they are nothing but repeated calls to someone else's library. How do we make changes in code like that?

The immediate temptation is to say that we don't really need tests. After all, we aren't really doing anything significant; we're just calling a method here and there, and our code is simple. It's really simple. What can go wrong?

[...]

When we Skin and Wrap the API we make interfaces that mirror the API as close as possible and then create wrappers around that API. [...] One advantage to skinning and wrapping an API is that we can end up having no dependencies on the underlying API code. Our wrappers can delegate to the real API in production code and we can use fakes during test.

[...]

Skin and Wrap the API is good in these circumstances:

  • The API is relatively small.
  • You want to completely separate out dependencies on a third-party library.
  • You don't have tests, and you can't write them because you can't test through the API.
    When we skin and wrap an API, we have the chance to get all of our code under test except for a thin layer of delegation from the wrapper to the real API classes.

I use this approach a lot when testing UI code, trying to treat the native browser APIs just like any other third-party API. In this case I'd consider a setup that wraps window.open in a method we can stub in our tests. That stub method would return a Mock object that fakes the behavior of a Window that we need, and can tell us at the end of the test what calls were made against it. I'd also consider extracting a method encapsulating those interactions with the mock Window for testing in isolation.

  printLoginCards = () => {
    const printArea = document.getElementById('printArea').outerHTML;

    // Adding a unique ID to the window name allows for multiple instances of this window
    // to be open at once without affecting each other.
    const windowName = `printWindow-${_.uniqueId()}`;
    
    const {section} = this.props;
    const documentTitle = i18n.printLoginCards_windowTitle({
      sectionName: section.name
    });

    // Browser API calls (other than the getElementById call above) isolated here:
    const printWindow = this.openWindow(windowName);
    this.printHtml(printWindow, documentTitle, printArea);
  };

  // In tests you could stub this method to return a mock window object,
  // and verify that the appropriate methods were called on it.
  openWindow(name) {
    return window.open('', name, '');
  }

  // You could write a number of small unit tests against this method, passing in a
  // mock printWindow and checking, for example, that something was bound to the
  // load event.  You could even test triggering that load event by having test locate
  // and call the second argument to a spy `printWindow.addEventListener`
  printHtml(printWindow, documentTitle, htmlContent) {
    printWindow.document.open();
    printWindow.addEventListener('load', event => {
      printWindow.print();
    });

    printWindow.document.write(
      `<html><head><title>${documentTitle}</title></head>`
    );
    printWindow.document.write('<body onafterprint="self.close()">');
    printWindow.document.write(htmlContent);
    printWindow.document.write('</body></html>');
    printWindow.document.close();
  };

In this way, the only line of code that remains untested is the one with the window.open call - we can check the rest of our logic, including everything we do with that window object, at whatever level of detail seems appropriate.

@dmcavoy dmcavoy merged commit 834dc72 into staging-next Dec 6, 2019
@dmcavoy dmcavoy deleted the auto-print-login-cards branch December 6, 2019 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants