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

fix preferred translation #1244

Open
wants to merge 3 commits into
base: master
from
Open

Conversation

@m0ark
Copy link
Contributor

m0ark commented Nov 25, 2019

Preferred language selection is broken in discovery service. Currently it uses the last valid translation instead of the first valid.

Copy link
Member

tvdijen left a comment

To be a bit more specific in a nested loop, this should preferably be break 1;

BTW, this screams to be unit tested.. If we can extract the foreach and usort into a separate method, we could perfectly test the alphabetical order and language preference.

@codecov

This comment was marked as off-topic.

Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #1244 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1244   +/-   ##
=========================================
  Coverage     36.84%   36.84%           
  Complexity     3782     3782           
=========================================
  Files           138      138           
  Lines         11548    11548           
=========================================
  Hits           4255     4255           
  Misses         7293     7293
Impacted Files Coverage Δ Complexity Δ
lib/SimpleSAML/XHTML/IdPDisco.php 0% <0%> (ø) 69 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9c52dc...8f641a9. Read the comment docs.

@tvdijen tvdijen force-pushed the m0ark:fix-disco-language branch 2 times, most recently from 7c597df to 8b733bc Nov 25, 2019
@tvdijen

This comment has been minimized.

Copy link
Member

tvdijen commented Nov 25, 2019

I think I've made a good start here, but I'm not really good with unit tests, so it would be great if someone can pick up from here

@tvdijen tvdijen force-pushed the m0ark:fix-disco-language branch from 8b733bc to 4fe7630 Nov 25, 2019
@tvdijen tvdijen force-pushed the m0ark:fix-disco-language branch from 4fe7630 to 8f641a9 Nov 25, 2019
* @param array $idpList
* @return array
*/
private function getPreferredTranslations(array $idpList, array $tryLanguages): array

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Nov 26, 2019

Member

I think this is quite misleading. What this method is doing is returning a list of IdPs, sorted by name, where their names are translated based on the preferred translation for the given request. I don't think getPreferredTranslations() correctly describes that, but instead it sounds like it's just returning the preferred translations for some given IdPs.

I would actually suggest refactoring this code. There's a lot of repetition in there, and testing is quite hard, so we could benefit from routing to feed requests and get responses out of it.

I'm going to merge @m0ark's commit into 1.18, and we can keep working on this meanwhile.

@jaimeperez

This comment has been minimized.

Copy link
Member

jaimeperez commented Nov 26, 2019

I agree with Tim here. We should do this properly and add unit tests. However, it's quite difficult to get some tests working, so I've prioritized getting the fix into a stable release (1.18.1, out just now), and then I'm leaving this PR open as a reminder that we need to get back to this.

@tvdijen tvdijen added this to In progress in 2.0 release via automation Dec 31, 2019
@tvdijen tvdijen added this to the 2.0 milestone Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.0 release
  
In progress
3 participants
You can’t perform that action at this time.