1) Fix fetchSummary addVaryHeader call

2) Remove unneeded check of shouldConvert 3) Test if accept-language is present in vary header
wikimedia · Dec 11, 2019 · 1ea8819 · 1ea8819
1 parent 8c7cddd
commit 1ea8819
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 17 deletions.
diff --git a/lib/language_variants_filter.js b/lib/language_variants_filter.js
@@ -89,19 +89,16 @@ module.exports = (hyper, req, next, options, specInfo) => {
                 // other content pure-fetching, but right now let's use a magic
                 // header.
                 mReq.headers['cache-control'] = 'no-cache,no-store';
-                
+
                 //
                 return next(hyper, mReq)
-                .then((res) => mwUtil.canConvertLangVariant(hyper, mReq, revision.page_language)
-                .then((canConvert) => {
+                .then((res) => {
                     // TODO: For now we hacky check by domain as the vary header
                     // is not stored for everything.
-                    if (canConvert) {
-                        mwUtil.addVaryHeader(res, 'accept-language');
-                        res.headers['content-language'] = revision.page_language;
-                    }
-                })
-                .thenReturn(res));
+                    mwUtil.addVaryHeader(res, 'accept-language');
+                    res.headers['content-language'] = acceptLanguage;
+                    return res;
+                });
             }
         });
     });

diff --git a/lib/mwUtil.js b/lib/mwUtil.js
@@ -414,16 +414,18 @@ mwUtil.isSelfRedirect = (req, location) => {
  * Fetches the summary content from the provided URI
  * @param  {HyperSwitch}  hyper     HyperSwitch context
  * @param  {URI}          uri       summary URI
- * @param  {headers}      headers    req header
+ * @param  {headers}      headers   req header
+ * @param  {headers}      pRes      response of the parent request
  * @return {string}                 summary body content
  */
-mwUtil.fetchSummary = (mRes, hyper, uri, headers = undefined) => {
+mwUtil.fetchSummary = (hyper, uri, headers, pRes) => {
     return hyper.get({ uri, headers })
     .then((res) => {
-        // Assign content-language and vary header based on one of summary responses
-        if (mRes.headers && !mRes.headers['content-language']) {
-            mRes.headers['content-language'] = res.headers && res.headers['content-language']
-            mwUtil.addVaryHeader(res, 'accept-language');
+        // Assign content-language and vary header to parent response
+        // based on one of summary responses
+        if (pRes && pRes.headers && !pRes.headers['content-language']) {
+            pRes.headers['content-language'] = res.headers && res.headers['content-language'];
+            mwUtil.addVaryHeader(pRes, 'accept-language');
         }
         res.body.normalizedtitle = res.body.title;
         res.body.title = res.body.title.replace(/ /g, '_');

diff --git a/test/features/related.js b/test/features/related.js
@@ -21,7 +21,8 @@ describe('Page Related', () => {
             assert.deepEqual(res.status, 200);
             assert.ok(Array.isArray(res.body.pages));
             assert.deepEqual(res.body.pages[0].displaytitle, '首頁');
-            assert.deepEqual(res.headers['content-language'], 'zh');
+            assert.deepEqual(res.headers['content-language'], 'zh-hant');
+            assert.ok(res.headers['vary'].includes('accept-language'));
         });
     })
 

diff --git a/v1/related.js b/v1/related.js
@@ -50,7 +50,7 @@ class Related {
             delete res.body.items;
 
             // Step 2: Hydrate response as always.
-            return mwUtil.hydrateResponse(res, (uri) => mwUtil.fetchSummary(res, hyper, uri, rh));
+            return mwUtil.hydrateResponse(res, (uri) => mwUtil.fetchSummary(hyper, uri, rh, res));
         });
     }
 }