[discussion] Config-level list of accepted labels?

[miketaylr]

Previous discussions:

#436 (comment)

#773 (comment)

Pros:

• Simplicity.
• We know the list in advance and don't need to make requests to GitHub.
• Would solve some caching/empty response bugs (and probably introduce new ones :p)

Cons:

• Requires process for adding new labels (might be a pro...).
• We have to build it.

Discuss!

[karlcow]

The controlled list would make it a lot simpler indeed.
And would also remove a useless HTTP request and make it faster.

A proposal would be to have a separate json file to deploy that would be read when the server is started (and/or any n hours). It could be also in the config file. Once the data structure is in memory it becomes a lot easier to use it. A possible transition if we are afraid to break things is that the route https://webcompat.com/api/issues/labels is in fact just sending the same json file but from webcompat server in the python code without requesting to GitHub. We still get an HTTP request, but it makes the transition smoother.

Step 2. Would be to remove the HTTP request entirely little by little having both in parallel if necessary, they would be fed from the same file and/or data structure.

[hallvors]

Implementing this is easy ;) Adding one JSON file and changing one URL in the JS source, done. Here's what the backend sends right now:
labels.json.txt

But we could also tweak the JSON a bit and simplify the code handling it even more. For example, the current JSON is an array of these objects:

 {
    "url": "https://api.github.com/repos/webcompat/web-bugs/labels/status-duplicate",
    "name": "status-duplicate",
    "color": "cccccc"
  }

and we could make that

 {
    "category": "status",
    "name": "duplicate",
    "color": "cccccc"
  }

and use "category" for grouping in the UI and prefixing when saving to GitHub.

[karlcow]

I have partly suggested this in the code for @magsout in https://github.com/karlcow/webcompat.com/blob/486ecaf01fa08edfa65116a17586a6e1894ebdb1/config.py.example#L104

The data model can be improved.

[hallvors]

So 344b277 is a possible (basic) implementation of this. It simply adds the JSON file GitHub is currently sending to static/config_data/labels.json and changes endpoints.py to read this file instead of getting data from GitHub.

[hallvors]

r?@miketaylr

[hallvors]

We might want to augment this fix with some code that makes sure existing labels on an issue get listed in the labels editor even if they are not included in the hardcoded "all labels" list - just to guard against data loss. I'll look into that..

[hallvors]

A suggestion for how to make sure we don't drop labels that are not in the labels.json list on the floor is here: c04e67a

[hallvors]

(and now I've grabbed somebody else's issue without reassigning again..)

[miketaylr]

(Hm, I don't even think I meant to assign myself this issue.)

[hallvors]

In the pull request / code review we're discussing a little how the static json file should get updated - like periodic downloads from Github, or every time the webcompat app i started, or manually / through pull requests. This decision depends on what exactly we want to achieve here.

• if we want more control of the labels, giving a certain set of labels some "official" blessing or status, fetching all labels automagically from GitHub is a bad idea. In this case we want to maintain the file more manually, with PRs and such.
• If we're doing this just to save one request to GH's backend and speed up the site a little bit, we're basically implementing a cache and checking periodically if it is stale is a good idea.

From some of the above discussion, I thought we were doing the former. From the PR discussion it sounds like we're doing the latter. Thoughts?

(Personally I think we should just go with the current code and iterate if we change our minds - it's not hard to change - because the site is semi-broken and labels are being dropped and lost without this fix or something equivalent.)

[miketaylr]

My understanding of "Config-level list of accepted labels" is we manually enumerate the labels we want to display in the webcompat UI (and then from that we generate a JSON file to serve to the client).

What we have in #812 is just serving today's version of the repo's labels. I'm less interested in saving a single (asynchronous) request to GitHub, but that's a bonus.

(and #781 (comment) led me to believe we wanted to build the JSON response ourselves, not just re-hash what GitHub gives us).

I think we should just go with the current code and iterate if we change our minds - it's not hard to change - because the site is semi-broken and labels are being dropped and lost without this fix or something equivalent

If we want a quick fix, we can just land #803.

[hallvors]

No problem, let's land #803 and take the time to agree on what we want to do here.

[miketaylr]

This shouldn't have been closed, I think. Let's discuss how to solve this in Orlando and move forward. ⏩

[hallvors]

Summarizing our discussion: we agree that we want this for the following reasons:

• better control of the labels we show and apply (main reason)
• less requests to GitHub backend
• better performance

Having a static list on the backend will fulfil all those reasons. However, we don't need to maintain a GitHub-style JSON file with URLs and colours per label - this just adds overhead. We can do just a plain text file or simple array - for example in static/config/labels.txt

[hallvors]

My proposal: I'd like to improve performance not only by not hitting GitHub, but also by including the list of labels in our built JS file so that we don't need an extra XHR request to get them. Nearly every page we show needs to know something about labels. They should simply be included.
The drawback is that changing our set of "official" labels requires re-building the site. I think it's ok to do this. If nobody screams, I'll suggest the coding changes soon :)

[miketaylr]

Hold off on sending a PR, just yet.

I know @karlcow has some ideas around why we might need to have these labels in memory from the Python side as well. If that's the case (I forget the specifics right now), then having the list of labels in a labels.py dict (or in config.py, or literally anywhere that Python can read them) would be good. From that we can build out a labels.js (serving plain .txt seems weird?) and include that in relevant files via the build system.

@karlcow, remind us?

It would mean adding new labels would require a new deploy/build, but that's how we ship all new features -- seems OK.

[karlcow]

Our current system is far to be DRY. So let's see where we have redundancies. The labels are used in 3 different areas.

• JS client side
• Python server side
• CSS
• templates
• https://github.com/webcompat/webcompat.com/blob/master/webcompat/issues.py#L39
• https://github.com/webcompat/webcompat.com/blob/master/webcompat/api/endpoints.py#L115
• https://github.com/webcompat/webcompat.com/blob/master/config.py.example#L106 (used for the CSS now, but made to be more generic)
• https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/css/development/components/issue-item.css#L37
• https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/css/development/base/variable.css#L19
• https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/css/development/components/filter.css#L34
• https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/diagnose.js#L51
• https://github.com/webcompat/webcompat.com/blob/master/tests/test_urls.py#L89

etc. Just a simple search on needsdiagnosis for example.

Templates have been cleaned up by @magsout using the structure defined in config.py

• https://github.com/webcompat/webcompat.com/blob/master/templates/web_modules/filter.html#L1
• https://github.com/webcompat/webcompat.com/blob/master/templates/issue-list/issue-filter.html#L7
• https://github.com/webcompat/webcompat.com/blob/master/templates/layout.html#L66

[miketaylr]

I think we can split this up into a number of tasks:

1. Do what @hallvors is suggesting. Have a list of labels we care about, at build time, generate a JS object from that, and likely concat into the minified JS files for where we want them (issue-list, issue page, homepage).

2. Move the list of labels into Python config which defines a set of LABEL_CONSTANTS, e.g., LABEL_NEEDS_DIAGNOSIS = 'needsdiagnosis' or whatever, and use those constants in the code rather than duplicate strings.

3. Use that same list to generate JS from As a user, I can report a compatibility issue for a specific browser so I can encourage them to fix it. #1.

I think we should ignore the CSS. We just use the same names as labels because it's convenient for us to know where to stick the right colors, etc. And I don't want to start sticking Python templates into our CSS if we can avoid it. We already have a JS build process for them... let's not add more if we can avoid it.

[miketaylr]

(and yes, we should open new bugs for 1 - 3, if we're going to work on them).

[hallvors]

(Minor point I just want to mention: we should not add more non-sensitive stuff into config.py - or if we do choose to put them in config.py we should fix #786 first so we avoid all those painful updates where you first debug and then finally remember to copy and paste new stuff from config.py.example to your own config.py.)

[miketaylr]

Yep, agreed @hallvors. I might pick up #786 after I get addon screenshot stuff working. Or we can create a top-level /config/ dir and have config.py and labels.py or something. And then eventually secrets.py in there too.

[miketaylr]

Let's reconsider after #886

[karlcow]

This is fixed by #1765 Where I defined a config file for statuses. So we don't need to fetch anymore anything. And this maybe answers my question in #1773