Fix accept-language header for /page/related #1229
Conversation
|
One more thing - you probably would want to add |
thesocialdev
changed the title
|
I believe tests flaked with 502 HTTP errors. Am I missing something or rechecking CI is needed? |
Restarted a build. Let's seee... |
1) Forward headers to summary endpoint to fetch proper displaytitle for language variant 2) Add test using zh beta 3) TODO: Identify why language_variants_filter.js#12 is deleting accept-language header and not forwarding it. Bug: T231609
2) Pass vary and content-language to response
2) Remove unneeded check of shouldConvert 3) Test if accept-language is present in vary header
thesocialdev
force-pushed
the
fix_related_lang_header
branch
from
1ea8819
to
9f69478
Compare
|
This PR needs another review after rebasing to make sure it complies with recent parsoid cleanup. |
|
recheck? |
lib/mwUtil.js
Outdated
| if (pRes && pRes.headers && !pRes.headers['content-language']) { | ||
| pRes.headers['content-language'] = res.headers && res.headers['content-language']; | ||
| mwUtil.addVaryHeader(pRes, 'accept-language'); | ||
| } |
There was a problem hiding this comment.
Maybe we can return an object [summary, content-language] instead of assigning the header here. I think it would be less magical.
|
RE-reviewd. LGTM, just some little suggestions. |
2) move related tests to language_variants 3) return req instead of undefined
2) Fix some linting issues 3) apply fetchSummary calls to check for summary key when hydrating responses 4) Deep clone only needed params at language_variant_filter
|
Ping! Is this good to merge? |
|
Cool, thanks. Would you mind pinging me next time you deploy restbase so we can test and close out the ticket? |
|
sure |
language variant
Bug: T231609