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

Consolidate the channels + channel-labels #625

Merged
merged 12 commits into from
Oct 12, 2018
Merged

Conversation

lukebjerring
Copy link
Contributor

@lukebjerring lukebjerring commented Oct 5, 2018

Description

Further work on #535

Changes the TestRunQuery to expand channel-labels onto the products during the building phase, and collapse them off of the products when producing the query-string.

Review Information

  • Visit /flags, enable queryBuilder
  • Load /results, open the query builder with the pencil icon
  • Observe that the Channel is Stable for all products
  • Submit the query
  • Observe that the query is ?aligned&label=stable, not ?product=chrome[stable]&product=...

@wpt-pr-bot
Copy link

Staging instance deployed by Travis CI!
Running at https://shared-channel-dot-wptdashboard-staging.appspot.com

@foolip
Copy link
Member

foolip commented Oct 5, 2018

This is great! I could quite easily use it get a view of Chrome+Edge+Firefox stable and Safari experimental, which was a URL I was trying to make manually just yesterday.

I also think I've found a bug, so before I review here are steps to repro:

  1. Enable /flags and start at https://shared-channel-dot-wptdashboard-staging.appspot.com/results/
  2. Click the pen
  3. Change all the browser channels to "Experimental", uncheck "Align runs" and submit
  4. You are at https://shared-channel-dot-wptdashboard-staging.appspot.com/results/?label=experimental
  5. Change all the browser channels back to "Stable" and submit
  6. You are still at ?label=experimental. Even toggling "Align runs", changing back and forth, as long as you pick "Stable" for all, you'll be back at ?label=experimental.

@foolip
Copy link
Member

foolip commented Oct 5, 2018

Oh, it also looks like "Add product" is broken. It also strikes me that it won't be too obvious to people how to close the query builder. In addition to being able to toggle it with the pen, maybe ad an × to dismiss it? Or a "Done" button?

@lukebjerring
Copy link
Contributor Author

I'll add the collapse behaviour in a separate PR.

@foolip
Copy link
Member

foolip commented Oct 10, 2018

Trying the steps in #625 (comment) I think there's still a bug, at the end I'm still left at ?label=experimental and showing experimental runs, not stable. Can you try to repro?

@lukebjerring
Copy link
Contributor Author

lukebjerring commented Oct 10, 2018

Confirmed there's a bug; looks like the collapsed label isn't being updated correctly.

EDIT: That took a while to diagnose; turned out to be a mutation of array of labels on the global DEFAULT_PRODUCTS helper; wrote a test that was failing for the same reason and fixed.

@lukebjerring
Copy link
Contributor Author

PTAL

@@ -132,7 +132,7 @@
}

clearQuery() {
this.products = Array.from(DEFAULT_PRODUCTS);
this.products = DEFAULT_PRODUCTS.map(p => Object.assign({}, p));
Copy link
Member

Choose a reason for hiding this comment

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

Aha. Subtle bug there.

if ('product' in params) {
this.products = params.product.map(p => this.parseProductSpec(p));
}
// Expand a global channel label into the separate products
let sharedChannel = (params.label || []).find(l => CHANNELS.has(l));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: if you just let sharedChannel here you can move params.label.find(l => CHANNELS.has(l)) into the if body. Minimally clearer, take your pick :)

@lukebjerring lukebjerring merged commit 81b4d32 into master Oct 12, 2018
@lukebjerring lukebjerring deleted the shared-channel branch October 12, 2018 13:54
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.

3 participants