Issue 5846 - Add support for greenhouse for jobs on eyeo.com

Issue 5846 - Add support for greenhouse for jobs on eyeo.com

[juliandoucette, 2017-10-09 11:53:46]

Ready for review.

CC wspee because this affects our HR/recruiter content editing process.

Instead of adding another ID to our jobs list I added 3 constants [GREENHOUSE,
TALENTTRACKER, EMAIL] and placed them before the ID in the list. I acknowledge
that we do not currently (and may never) have EMAIL listings. I just wanted to
be explicit. It still falls back to EMAIL if no ID is provided.

(Also, NOTE: I removed the <button> inside the apply link because <button>
inside <a> is invalid AFAIK)

[juliandoucette, 2017-10-09 11:59:14]

On 2017/10/09 11:53:46, juliandoucette wrote:
> It still falls back to EMAIL if no ID is provided.

Correction: All openings list values must be given. There is no error handling
for missing or invalid values. I think we should address missing or invalid
values separately, if at all. EMAIL can be accomplished via ("Title", "slug",
EMAIL, False).

[ire, 2017-10-10 08:13:45]

On 2017/10/09 11:53:46, juliandoucette wrote:
> Ready for review.
> 
> CC wspee because this affects our HR/recruiter content editing process.
> 
> Instead of adding another ID to our jobs list I added 3 constants [GREENHOUSE,
> TALENTTRACKER, EMAIL] and placed them before the ID in the list. I acknowledge
> that we do not currently (and may never) have EMAIL listings. I just wanted to
> be explicit. It still falls back to EMAIL if no ID is provided.
> 
> (Also, NOTE: I removed the <button> inside the apply link because <button>
> inside <a> is invalid AFAIK)

Ack. Yes I think you're right that it is invalid

[ire, 2017-10-10 08:17:51]

On 2017/10/09 11:59:14, juliandoucette wrote:
> On 2017/10/09 11:53:46, juliandoucette wrote:
> > It still falls back to EMAIL if no ID is provided.
> 
> Correction: All openings list values must be given. There is no error handling
> for missing or invalid values. I think we should address missing or invalid
> values separately, if at all. EMAIL can be accomplished via ("Title", "slug",
> EMAIL, False).

The issue here seems to be with the for loop that lists all the openings in the
sidebar.

I think we can fix this by accessing the array, instead of every value in the
array in that loop. e.g:

{% for opening in openings %}
    {{ opening[0] }} // for the title, etc.
{% endfor %}

This way, we can have fields be missing without breaking anything. (since the
only fields needed are the first two for that loop anyway)

[ire, 2017-10-10 08:18:33]

Thanks Julian! One NIT, and I responded to your other comments as well.

https://codereview.adblockplus.org/29570571/diff/29570572/includes/jobs/heade...
File includes/jobs/header.tmpl (right):

https://codereview.adblockplus.org/29570571/diff/29570572/includes/jobs/heade...
includes/jobs/header.tmpl:17: {% set EMAIL = 3 %}
NIT: I found this a bit confusing to understand at first. What is the benefit of
setting these variables up here, instead of just placing them as strings where
they are used? 

e.g. :

("IT System Administrator", "it-system-administrator", "talenttracker", 189)

Then just checking for the string in the footer:

{%- if opening[2] == "talenttracker" -%}

etc.

On initial look, it made me think you were doing something with the numbers
themselves (1, 2, 3).

[juliandoucette, 2017-10-10 10:46:22]

> I think we can fix this by accessing the array, instead of every value in the
array in that loop. e.g:

Didn't think of that. I'll upload a new PatchSet.

https://codereview.adblockplus.org/29570571/diff/29570572/includes/jobs/heade...
File includes/jobs/header.tmpl (right):

https://codereview.adblockplus.org/29570571/diff/29570572/includes/jobs/heade...
includes/jobs/header.tmpl:17: {% set EMAIL = 3 %}
On 2017/10/10 08:18:33, ire wrote:
> NIT: I found this a bit confusing to understand at first. What is the benefit
of
> setting these variables up here, instead of just placing them as strings where
> they are used? 

- variable / constant names are more commonly autocompleted (some editors
autocomplete common strings too)
- variable / constant name typos are more commonly identified by intenseness
(some editors spot typos in strings too)
- a single value takes up less memory than multiple 
- a small positive int takes up less memory than a string 

(I'm lazy, mildly dyslexic (brain typo), and I type fast (finger typo) -
autocomplete helps :D)

(I like these optimizations. But they are not necessary.)

(This is a ~classical programming habit and these are ~classical programming
reasons.)

[juliandoucette, 2017-10-10 10:53:13]

PatchSet up.

[juliandoucette, 2017-10-10 10:57:12]

@wspee question below.

https://codereview.adblockplus.org/29570571/diff/29570572/includes/jobs/heade...
File includes/jobs/header.tmpl (right):

https://codereview.adblockplus.org/29570571/diff/29570572/includes/jobs/heade...
includes/jobs/header.tmpl:17: {% set EMAIL = 3 %}
On 2017/10/10 10:46:22, juliandoucette wrote:
> On 2017/10/10 08:18:33, ire wrote:
> > NIT: I found this a bit confusing to understand at first. What is the
benefit
> of
> > setting these variables up here, instead of just placing them as strings
where
> > they are used? 
> 
> - variable / constant names are more commonly autocompleted (some editors
> autocomplete common strings too)
> - variable / constant name typos are more commonly identified by intenseness
> (some editors spot typos in strings too)
> - a single value takes up less memory than multiple 
> - a small positive int takes up less memory than a string 
> 
> (I'm lazy, mildly dyslexic (brain typo), and I type fast (finger typo) -
> autocomplete helps :D)
> 
> (I like these optimizations. But they are not necessary.)
> 
> (This is a ~classical programming habit and these are ~classical programming
> reasons.)

@wspee Do you think that this makes a difference to HR/Recruitment?

[ire, 2017-10-10 11:43:53]

It appears that is issue has been unset. 

A couple of comments below, but otherwise this LGTM.

https://codereview.adblockplus.org/29570571/diff/29570572/includes/jobs/heade...
File includes/jobs/header.tmpl (right):

https://codereview.adblockplus.org/29570571/diff/29570572/includes/jobs/heade...
includes/jobs/header.tmpl:17: {% set EMAIL = 3 %}
On 2017/10/10 10:46:22, juliandoucette wrote:
> On 2017/10/10 08:18:33, ire wrote:
> > NIT: I found this a bit confusing to understand at first. What is the
benefit
> of
> > setting these variables up here, instead of just placing them as strings
where
> > they are used? 
> 
> - variable / constant names are more commonly autocompleted (some editors
> autocomplete common strings too)
> - variable / constant name typos are more commonly identified by intenseness
> (some editors spot typos in strings too)
> - a single value takes up less memory than multiple 
> - a small positive int takes up less memory than a string 
> 
> (I'm lazy, mildly dyslexic (brain typo), and I type fast (finger typo) -
> autocomplete helps :D)
> 
> (I like these optimizations. But they are not necessary.)
> 
> (This is a ~classical programming habit and these are ~classical programming
> reasons.)

Okay, I agree with your decision

https://codereview.adblockplus.org/29570571/diff/29570572/includes/jobs/heade...
includes/jobs/header.tmpl:17: {% set EMAIL = 3 %}
We probably don't need the EMAIL constant anymore do we? I don't mind if you
want to keep it for the sake of being explicit

https://codereview.adblockplus.org/29570571/diff/29572607/includes/jobs/heade...
File includes/jobs/header.tmpl (right):

https://codereview.adblockplus.org/29570571/diff/29572607/includes/jobs/heade...
includes/jobs/header.tmpl:29: ("test", "test"),
I think you forgot to delete this :)

[juliandoucette, 2017-10-10 11:59:20]

https://codereview.adblockplus.org/29570571/diff/29572607/includes/jobs/heade...
File includes/jobs/header.tmpl (right):

https://codereview.adblockplus.org/29570571/diff/29572607/includes/jobs/heade...
includes/jobs/header.tmpl:29: ("test", "test"),
On 2017/10/10 11:43:53, ire wrote:
> I think you forgot to delete this :)

Thank you! :D

[juliandoucette, 2017-10-11 11:32:16]

@wspee should we delay this change for some reason?

[wspee, 2017-10-11 18:10:51]

On 2017/10/11 11:32:16, juliandoucette wrote:
> @wspee should we delay this change for some reason?

No, but depending on the outcome of [0] it might not be necessary to have
support for both talentstracker and greenhouse and this change could have been
simply replace the talentstracker url with the greenhouse url and update all ids
in header.tmpl.

So if you don't mind you can go ahead.

[0] https://issues.adblockplus.org/ticket/5839#comment:7

[juliandoucette, 2017-10-12 11:49:18]

https://hg.adblockplus.org/web.eyeo.com/rev/a39469c50875