Right now the HTML markup needed for the password JavaScript matching, strength meter, and strength suggestions is hardcoded in user.js Drupal.behaviors.password behavior function. In keeping with Drupal's JavaScript best practices, and to make the markup accessible for themer's to add additional CSS classes to the markup, the markup should be moved to Drupal JavaScript theme functions instead.

Comments

PCate created an issue. See original summary.

PCate’s picture

droplet’s picture

Status: Active » Needs work
+++ b/core/modules/user/user.js
@@ -214,4 +214,50 @@
+  $.extend(Drupal.theme, /** @lends Drupal.theme */ {

Let's get rid of $.extend

PCate’s picture

Status: Needs work » Needs review
droplet’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs review » Reviewed & tested by the community

Thanks. I counted it as API addition. So 8.3.x

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Is there any possibility of a regression test?

PCate’s picture

Is there any possibility of a regression test?

Unfortunately I don't have experience writing regression tests.

cilefen’s picture

@droplet I am going to defer to you on the testability prospects. What do you think? All of the likely user tests are WebTestBase in 8.4.x HEAD right now. These are the kinds of functions I would love to see unit tests for.

droplet’s picture

Personally, I don't expect write tests for simple code refactoring. The code style should be catch by testbot, not UnitTests. Let's open a
separate issue for password features tests.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @cilefen and @droplet.

So if this were a PHP patch, we would do both these things in this issue:

  1. Add unit tests for the three added functions.
  2. Ensure the password indicator feature already had functional test coverage, and if it did not, add that as well.

That's how we apply the testing gate. For JS tests, now that we have integration testing, we should try to apply the same policy. I checked and the committers agreed that for now we would require that either the tests be included on the issue or a followup be filed for them.

So the next step here is to either file that followup explicitly now, or (ideally) to add the test coverage on the issue. Marking NW to do one of those two things here.

Do we have the ability yet to run JS unit tests?

droplet’s picture

Issue tags: +Needs JS testing

Unfortunately then, Rule is Rule.

nod_’s picture

No JS unit tests yet, part of the ES6 stuff #2815199: Add tools and scripts for writing and running javascript unit tests

We might need some help on how we should test this. If we call Drupal.theme('passwordMeter', translate) and check for the HTML that's supposed to be returned, I don't see the benefit. There is no theme function override in core.

Testing Drupal.theme() itself, sure definitely needed. Testing a specific theme function, that feels like testing a twig file for the markup it outputs. If we're doing that, can someone point to a test we can see how it's done?

droplet’s picture

Status: Needs work » Reviewed & tested by the community

Needs clarify from committers when the test is required. This is inefficiency to forced to write a full-set of feature testing for little code improvement.

Remark: this is a code style improve more than bug fixing.

xjm’s picture

Title: Use Drupal JS theme functions for outputting password validation markup. » Use Drupal JS theme functions for outputting password validation markup
Assigned: PCate » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup, +Needs manual testing

In the interim where we do not have JS test coverage for the majority of things, a followup issue for testing the password validation interaction is also acceptable, so we would need to file that issue and reference it here if it's too onerous now to add the test with this issue. If we file a followup, though, we also need complete before-and-after manual testing with this patch instead, ideally with screenshots etc.

I will say that regardless of the language, having test coverage when doing a code improvement/refactoring is just as important as adding a regression test for a bugfix. But it's definitely still an uphill challenge for JS functionality since the existing JS test suite is so incomplete.

Thanks @nod_ and @droplet for looking into this!

PCate’s picture

FileSize
2.29 MB
1.92 MB

I've attached 2 screen recordings of using the password fields before and after the latest patch. As they show password strength JS continues to work as usual after applying the patch.

PCate’s picture

Status: Needs work » Needs review
PCate’s picture

Anyone have a chance to check out my manual testing of this?