Fixes #2453 - Changes regex to fix progressive label filtering #2496

laghee · 2018-06-15T18:58:22Z

This PR fixes issue #2453

Proposed PR background

Progressive label filtering was broken, seemingly because of something wonky in the regex. I simplified the regex used in editor.js to escape special characters in label titles. I only included currently used special characters, so if future label titles involve anything other than /, ., (, ), /s, -, &, and :, this will need to be updated.

[✔︎] I have read the Pull Request + Code Style Guidelines thoroughly.

miketaylr

Looks good!

I think in the future we will want to restrict possible chars in labels further, but this seems good.

Can we add a functional test for this so it doesn't regress in the future? Something like:

go to issue 100
open the label filter widget
type test-label
assert we get expected number of results

https://github.com/webcompat/webcompat.com/blob/master/tests/functional/labels-auth.js is a good place for that test.

we may need to update a fixture file or 2, that will be something to investigate if things are failing for you unexpectedly:

https://github.com/webcompat/webcompat.com/tree/master/tests/fixtures/api/issues

laghee · 2018-06-18T17:27:15Z

OK, will do. 👍

laghee · 2018-06-21T13:51:00Z

@miketaylr I'm not totally clear on fixtures, so I can't figure out if I'm doing something goofy or if there's something amiss in the way the fixtures are set up:

running the server in test mode serves the labels fixture (where there is no test-label value, so I substituted status-tacos ... obviously)
all of the labels are hidden (have style="display: none;") when status-tacos is entered into the search field
entering s matches only status-sitewait
entering tacos matches only status-tacos
entering i matches 4 -- closed-invalid, browser-ie, status-imported, internet-explorer 🤔

Does this sound like the fix isn't working after all, or does something need to be tweaked in the testing environment?

miketaylr · 2018-06-21T17:31:30Z

OK, let's see.

When looking at a normal issue (non-fixture):

If i type s, I get 2 labels.

When looking at fixture data, I get one:

It seems like we're matching on the first letter after status-... this seems expected, I think?

So if you search for status-tacos, it won't match because it's looking for status-status-tacos. Does that make sense? If I were writing a test, I would search for tacos instead, then do assertions, etc.

webcompat.com/webcompat/static/js/lib/models/label-list.js

Line 25 in d8d18c6

issues.LabelList = Backbone.Model.extend({

handles all the matching around "namespaces"... I think. It's been a while. :)

laghee · 2018-06-22T08:55:02Z

Wait, so status isn't part of the actual label text? Ahhhhhhh, OK. And I suppose that's true for closed and browser, too? Now I get it.

Sorry, @miketaylr , my bad -- even setting the tests to run in non-headless, I couldn't see the labels themselves or get the inspector to run, so I just assumed everything after label was displayed text (along the lines of status: add to changelog). 🤦‍♀️ (Now that I go back and look at the live labels with the inspector, I see it's the same there, which totally slipped past me.)

New commit incoming ...

Edit: OK, I'm not re-running these in case you wanted to check out the error, but the tests were passing locally. Let me know if I messed up and it's not the usual problem, though.

TravisBuddy · 2018-06-22T09:18:27Z

Travis tests have failed

Hey @laghee,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

npm run test:js -- --reporters="runner" --firefoxBinary=`which firefox`

> webcompat@ test:js /home/travis/build/webcompat/webcompat.com
> node ./tests/functional/_intern.js "--reporters=runner" "--firefoxBinary=/home/travis/firefox-58.0/firefox/firefox"

Error: 
        ======================================================
        Oops, something went wrong. Try restarting the server.
        Open another terminal and window type:
        npm run start:test
        or
        python run.py -t
        ======================================================
        
  at ClientRequest.<anonymous>  <tests/functional/lib/setup.js:55:9>
  at emitOne  <events.js:116:13>
  at ClientRequest.emit  <events.js:211:7>
  at Socket.socketErrorListener  <_http_client.js:387:9>
  at emitOne  <events.js:116:13>
  at Socket.emit  <events.js:211:7>
  at emitErrorNT  <internal/streams/destroy.js:64:8>
something bad happened inside intern.run()
{ Error: 
        ======================================================
        Oops, something went wrong. Try restarting the server.
        Open another terminal and window type:
        npm run start:test
        or
        python run.py -t
        ======================================================
        
    at ClientRequest.<anonymous> (/home/travis/build/webcompat/webcompat.com/tests/functional/lib/setup.js:55:9)
    at emitOne (events.js:116:13)
    at ClientRequest.emit (events.js:211:7)
    at Socket.socketErrorListener (_http_client.js:387:9)
    at emitOne (events.js:116:13)
    at Socket.emit (events.js:211:7)
    at emitErrorNT (internal/streams/destroy.js:64:8)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9) reported: true }
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! webcompat@ test:js: `node ./tests/functional/_intern.js "--reporters=runner" "--firefoxBinary=/home/travis/firefox-58.0/firefox/firefox"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the webcompat@ test:js script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/travis/.npm/_logs/2018-06-22T09_18_06_134Z-debug.log

miketaylr · 2018-06-22T14:35:19Z

OK, travis is super busted, let's do this manually:

npm run test:js -- --functionalSuites=tests/functional/labels-auth.js

✓ firefox on darwin 17.6.0 - Labels (auth) - Label editor opens then closes (clicks)
✓ firefox on darwin 17.6.0 - Labels (auth) - Label editor opens then closes (key events)
✓ firefox on darwin 17.6.0 - Labels (auth) - Clicking outside label editor closes it
✓ firefox on darwin 17.6.0 - Labels (auth) - Clicking close button actually closes it
✓ firefox on darwin 17.6.0 - Labels (auth) - Label editor filters label list based on entered text
✓ chrome 67.0.3396.87 on Mac OS X - Labels (auth) - Label editor opens then closes (clicks)
✓ chrome 67.0.3396.87 on Mac OS X - Labels (auth) - Label editor opens then closes (key events)
✓ chrome 67.0.3396.87 on Mac OS X - Labels (auth) - Clicking outside label editor closes it
✓ chrome 67.0.3396.87 on Mac OS X - Labels (auth) - Clicking close button actually closes it
✓ chrome 67.0.3396.87 on Mac OS X - Labels (auth) - Label editor filters label list based on entered text

node
Tunnel: Ready

Total: [✓✓✓✓✓✓✓✓✓✓] 10/10
Passed: 10  Failed: 0   Skipped: 0

Fx:         [✓✓✓✓✓] 5/5
Chr 67 Mac: [✓✓✓✓✓] 5/5
it started, yay!

miketaylr

thanks!

tests/functional/labels-auth.js

+        .type("tacos")
+        .end()
+        .sleep(1000)
+        .findAllByXpath(


laghee requested review from miketaylr and karlcow June 15, 2018 18:58

miketaylr requested changes Jun 18, 2018

View reviewed changes

laghee added 2 commits June 22, 2018 11:13

Issue webcompat#2453 - Changes regex to fix progressive label filtering

f1a7812

Issue webcompat#2453 - Adds functional test for label filtering

20dfd26

laghee force-pushed the issues/2453/1 branch from 0d4427c to 20dfd26 Compare June 22, 2018 09:15

miketaylr approved these changes Jun 22, 2018

View reviewed changes

tests/functional/labels-auth.js

.type("tacos")

.end()

.sleep(1000)

.findAllByXpath(

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

miketaylr merged commit 2bbcf85 into webcompat:master Jun 22, 2018

miketaylr added the status: add to changelog label Jun 22, 2018

miketaylr removed the status: add to changelog label Jun 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2453 - Changes regex to fix progressive label filtering #2496

Fixes #2453 - Changes regex to fix progressive label filtering #2496

laghee commented Jun 15, 2018

miketaylr left a comment

laghee commented Jun 18, 2018

laghee commented Jun 21, 2018 •

edited

Loading

miketaylr commented Jun 21, 2018

laghee commented Jun 22, 2018 •

edited

Loading

TravisBuddy commented Jun 22, 2018

miketaylr commented Jun 22, 2018

miketaylr left a comment

This comment was marked as abuse.

This comment was marked as abuse.

Fixes #2453 - Changes regex to fix progressive label filtering #2496

Fixes #2453 - Changes regex to fix progressive label filtering #2496

Conversation

laghee commented Jun 15, 2018

Proposed PR background

miketaylr left a comment

Choose a reason for hiding this comment

laghee commented Jun 18, 2018

laghee commented Jun 21, 2018 • edited Loading

miketaylr commented Jun 21, 2018

laghee commented Jun 22, 2018 • edited Loading

TravisBuddy commented Jun 22, 2018

Travis tests have failed

1st Build

miketaylr commented Jun 22, 2018

miketaylr left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

laghee commented Jun 21, 2018 •

edited

Loading

laghee commented Jun 22, 2018 •

edited

Loading