Issue 29347257: Issue 3997 - Wrong information about employees on Eyeo.com (Closed)
|
Created:
July 2, 2016, 6:24 p.m. by juliandoucette Modified:
Jan. 4, 2017, 8:16 p.m. CC:
Thomas Greiner, Vasily Kuznetsov, Jon Sonesen Visibility:
Public. |
DescriptionIssue 3997 - Wrong information about employees on Eyeo.com
Repository: hg.adblockplus.org/web.eyeo.com/
Patch Set 1 #Patch Set 2 : Separated country and flag global variables #Patch Set 3 : Implemented EU exception and removed additional global field #Patch Set 4 : Changed approach to check against non_countries list instead of hard coded non_countries inside cou… #
MessagesTotal messages: 21
No LGTM After discussion with HR department employees should decide on the flags individually, so we can not generate the number of the nationalities according to the information on team page, this number need to be updated manually for now, with the update to the team page, no code review actually needed for that
NOT LGTM
Yes, it's up to our team members which country they want to identify with (on the team page). But that doesn't necessarily is a reason to show an incomsistent number here. In fact, the text says "our # people represent # countries". So if I choose to present myself as German on the team page I "represent" Germany and should be counted as such.
On 2016/07/07 18:42:58, Sebastian Noack wrote: > Yes, it's up to our team members which country they want to identify with (on > the team page). But that doesn't necessarily is a reason to show an incomsistent > number here. In fact, the text says "our # people represent # countries". So if > I choose to present myself as German on the team page I "represent" Germany and > should be counted as such. In this case we are talking about flags that are not representing one exact country (ex. European Union flag). It's a new update request, so we can't rely on the current data to generate countries I guess.
On 2016/07/08 07:53:27, saroyanm wrote: > On 2016/07/07 18:42:58, Sebastian Noack wrote: > > Yes, it's up to our team members which country they want to identify with (on > > the team page). But that doesn't necessarily is a reason to show an > incomsistent > > number here. In fact, the text says "our # people represent # countries". So > if > > I choose to present myself as German on the team page I "represent" Germany > and > > should be counted as such. > > In this case we are talking about flags that are not representing one exact > country (ex. European Union flag). It's a new update request, so we can't rely > on the current data to generate countries I guess. I added an extra field to separate flag and country. Note, I didn't find "eu" in the list. Are there existing team members who have a flag different than the country they are from?
On 2016/07/08 16:45:00, juliandoucette wrote: > On 2016/07/08 07:53:27, saroyanm wrote: > > On 2016/07/07 18:42:58, Sebastian Noack wrote: > > > Yes, it's up to our team members which country they want to identify with > (on > > > the team page). But that doesn't necessarily is a reason to show an > > incomsistent > > > number here. In fact, the text says "our # people represent # countries". So > > if > > > I choose to present myself as German on the team page I "represent" Germany > > and > > > should be counted as such. > > > > In this case we are talking about flags that are not representing one exact > > country (ex. European Union flag). It's a new update request, so we can't rely > > on the current data to generate countries I guess. > > I added an extra field to separate flag and country. > > Note, I didn't find "eu" in the list. Are there existing team members who have a > flag different than the country they are from? Yes I think I've pushed that change after your comment ( https://hg.adblockplus.org/web.eyeo.com/file/tip/includes/globals.tmpl#l33 ), I do not have strong opinion whether it's better to update the number manually or introduce a new field, but if you feel having a new field is better solution, please go for it. Will review the code soon.
On 2016/07/11 09:35:55, saroyanm wrote: > On 2016/07/08 16:45:00, juliandoucette wrote: > > On 2016/07/08 07:53:27, saroyanm wrote: > > > On 2016/07/07 18:42:58, Sebastian Noack wrote: > > > > Yes, it's up to our team members which country they want to identify with > > (on > > > > the team page). But that doesn't necessarily is a reason to show an > > > incomsistent > > > > number here. In fact, the text says "our # people represent # countries". > So > > > if > > > > I choose to present myself as German on the team page I "represent" > Germany > > > and > > > > should be counted as such. > > > > > > In this case we are talking about flags that are not representing one exact > > > country (ex. European Union flag). It's a new update request, so we can't > rely > > > on the current data to generate countries I guess. > > > > I added an extra field to separate flag and country. > > > > Note, I didn't find "eu" in the list. Are there existing team members who have > a > > flag different than the country they are from? > > Yes I think I've pushed that change after your comment ( > https://hg.adblockplus.org/web.eyeo.com/file/tip/includes/globals.tmpl#l33 ), I > do not have strong opinion whether it's better to update the number manually or > introduce a new field, but if you feel having a new field is better solution, > please go for it. Will review the code soon. So if it's only about the EU case, how about simply not counting the value "eu" rather than making the data structure more complex, duplicating the value in all other cases? len({code for _, code, _, _, _, _ in people if code != "eu"})
On 2016/07/11 15:22:57, Sebastian Noack wrote: > On 2016/07/11 09:35:55, saroyanm wrote: > > On 2016/07/08 16:45:00, juliandoucette wrote: > > > On 2016/07/08 07:53:27, saroyanm wrote: > > > > On 2016/07/07 18:42:58, Sebastian Noack wrote: > > > > > Yes, it's up to our team members which country they want to identify > with > > > (on > > > > > the team page). But that doesn't necessarily is a reason to show an > > > > incomsistent > > > > > number here. In fact, the text says "our # people represent # > countries". > > So > > > > if > > > > > I choose to present myself as German on the team page I "represent" > > Germany > > > > and > > > > > should be counted as such. > > > > > > > > In this case we are talking about flags that are not representing one > exact > > > > country (ex. European Union flag). It's a new update request, so we can't > > rely > > > > on the current data to generate countries I guess. > > > > > > I added an extra field to separate flag and country. > > > > > > Note, I didn't find "eu" in the list. Are there existing team members who > have > > a > > > flag different than the country they are from? > > > > Yes I think I've pushed that change after your comment ( > > https://hg.adblockplus.org/web.eyeo.com/file/tip/includes/globals.tmpl#l33 ), > I > > do not have strong opinion whether it's better to update the number manually > or > > introduce a new field, but if you feel having a new field is better solution, > > please go for it. Will review the code soon. > > So if it's only about the EU case, how about simply not counting the value "eu" > rather than making the data structure more complex, duplicating the value in all > other cases? > > > len({code for _, code, _, _, _, _ in people if code != "eu"}) In that case we will have wrong information in the eyeo.com, the issue is about making that number up to date and correct. So we either need to make duplication for other cases or update that number manually I think.
On 2016/07/11 15:32:01, saroyanm wrote: > On 2016/07/11 15:22:57, Sebastian Noack wrote: > > On 2016/07/11 09:35:55, saroyanm wrote: > > > On 2016/07/08 16:45:00, juliandoucette wrote: > > > > On 2016/07/08 07:53:27, saroyanm wrote: > > > > > On 2016/07/07 18:42:58, Sebastian Noack wrote: > > > > > > Yes, it's up to our team members which country they want to identify > > with > > > > (on > > > > > > the team page). But that doesn't necessarily is a reason to show an > > > > > incomsistent > > > > > > number here. In fact, the text says "our # people represent # > > countries". > > > So > > > > > if > > > > > > I choose to present myself as German on the team page I "represent" > > > Germany > > > > > and > > > > > > should be counted as such. > > > > > > > > > > In this case we are talking about flags that are not representing one > > exact > > > > > country (ex. European Union flag). It's a new update request, so we > can't > > > rely > > > > > on the current data to generate countries I guess. > > > > > > > > I added an extra field to separate flag and country. > > > > > > > > Note, I didn't find "eu" in the list. Are there existing team members who > > have > > > a > > > > flag different than the country they are from? > > > > > > Yes I think I've pushed that change after your comment ( > > > https://hg.adblockplus.org/web.eyeo.com/file/tip/includes/globals.tmpl#l33 > ), > > I > > > do not have strong opinion whether it's better to update the number manually > > or > > > introduce a new field, but if you feel having a new field is better > solution, > > > please go for it. Will review the code soon. > > > > So if it's only about the EU case, how about simply not counting the value > "eu" > > rather than making the data structure more complex, duplicating the value in > all > > other cases? > > > > > > len({code for _, code, _, _, _, _ in people if code != "eu"}) > > In that case we will have wrong information in the http://eyeo.com, the issue is about > making that number up to date and correct. So we either need to make duplication > for other cases or update that number manually I think. True, if somebody chooses to have the Europe flag, but is representing a country that nobody else is representing by their flag, the number will be off by one. However, currently this would be just a theoretical scenario, and I suppose this number isn't too important after all. And at least we get consistent behavior without maintaining additional information.
On 2016/07/11 15:48:18, Sebastian Noack wrote: > On 2016/07/11 15:32:01, saroyanm wrote: > > On 2016/07/11 15:22:57, Sebastian Noack wrote: > > > On 2016/07/11 09:35:55, saroyanm wrote: > > > > On 2016/07/08 16:45:00, juliandoucette wrote: > > > > > On 2016/07/08 07:53:27, saroyanm wrote: > > > > > > On 2016/07/07 18:42:58, Sebastian Noack wrote: > > > > > > > Yes, it's up to our team members which country they want to identify > > > with > > > > > (on > > > > > > > the team page). But that doesn't necessarily is a reason to show an > > > > > > incomsistent > > > > > > > number here. In fact, the text says "our # people represent # > > > countries". > > > > So > > > > > > if > > > > > > > I choose to present myself as German on the team page I "represent" > > > > Germany > > > > > > and > > > > > > > should be counted as such. > > > > > > > > > > > > In this case we are talking about flags that are not representing one > > > exact > > > > > > country (ex. European Union flag). It's a new update request, so we > > can't > > > > rely > > > > > > on the current data to generate countries I guess. > > > > > > > > > > I added an extra field to separate flag and country. > > > > > > > > > > Note, I didn't find "eu" in the list. Are there existing team members > who > > > have > > > > a > > > > > flag different than the country they are from? > > > > > > > > Yes I think I've pushed that change after your comment ( > > > > https://hg.adblockplus.org/web.eyeo.com/file/tip/includes/globals.tmpl#l33 > > ), > > > I > > > > do not have strong opinion whether it's better to update the number > manually > > > or > > > > introduce a new field, but if you feel having a new field is better > > solution, > > > > please go for it. Will review the code soon. > > > > > > So if it's only about the EU case, how about simply not counting the value > > "eu" > > > rather than making the data structure more complex, duplicating the value in > > all > > > other cases? > > > > > > > > > len({code for _, code, _, _, _, _ in people if code != "eu"}) > > > > In that case we will have wrong information in the http://eyeo.com, the issue > is about > > making that number up to date and correct. So we either need to make > duplication > > for other cases or update that number manually I think. > > True, if somebody chooses to have the Europe flag, but is representing a country > that nobody else is representing by their flag, the number will be off by one. > However, currently this would be just a theoretical scenario, and I suppose this > number isn't too important after all. And at least we get consistent behavior > without maintaining additional information. Agree, While we are showing correct number I'm fine, I think this information is important for HR. In future if the workaround will become more complex, make sense to change to the Julian proposal, or update manually. But if it's just about this case, we should be fine "for now".
Done deal.
On 2016/07/12 14:49:27, juliandoucette wrote: > Done deal. Shit... That's not right. If we exclude "eu" than we don't account for the employee's actual country. I think a second field is the way to go.
On 2016/07/12 14:51:37, juliandoucette wrote: > On 2016/07/12 14:49:27, juliandoucette wrote: > > Done deal. > > Shit... > > That's not right. If we exclude "eu" than we don't account for the employee's > actual country. > > I think a second field is the way to go. Scratch that.
On 2016/07/12 18:37:45, juliandoucette wrote: > On 2016/07/12 14:51:37, juliandoucette wrote: > > On 2016/07/12 14:49:27, juliandoucette wrote: > > > Done deal. > > > > Shit... > > > > That's not right. If we exclude "eu" than we don't account for the employee's > > actual country. > > > > I think a second field is the way to go. Scratch that. My mistake. "our N people represent N countries" The current solution (exclude EU) works for this grammar.
This is ready for review.
> This is ready for review. This is no longer valid as "eu" was changed to "earth". Also, we shouldn't use includes/globals.tmpl. That's what globals/ are for.
On 2016/09/22 14:02:29, juliandoucette wrote: > Also, we shouldn't use includes/globals.tmpl. That's what globals/ are for. For reference, include/globals.tmpl existed before we added support for custom template globals (i.e. the globals directory) to the CMS. I agree that using the latter would be preferable now.
On 2016/09/22 14:09:52, Sebastian Noack wrote: > On 2016/09/22 14:02:29, juliandoucette wrote: > > Also, we shouldn't use includes/globals.tmpl. That's what globals/ are for. > > For reference, include/globals.tmpl existed before we added support for custom > template globals (i.e. the globals directory) to the CMS. I agree that using the > latter would be preferable now. Thanks for the info Sebastian. I consulted kvas on the matter and we came up with a better solution that I will submit later.
Updated. I encountered errors on the team page when converting includes/globals to globals/people so I decided to leave that change out of this review. This should be a quick one.
On 2016/10/10 20:27:49, juliandoucette wrote: > Updated. > > I encountered errors on the team page when converting includes/globals to > globals/people so I decided to leave that change out of this review. > > This should be a quick one. I am going to implement this differently, based on a discussion with Maren. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
