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 duplicate pages in on this day results. #937

Merged
merged 4 commits into from
Jan 5, 2018
Merged

Fix duplicate pages in on this day results. #937

merged 4 commits into from
Jan 5, 2018

Conversation

montehurd
Copy link
Contributor

The events for the following days are examples having dupe pages:

onthisday/events/09/14
"Telecommunications companies MCI Communications and WorldCom complete their $37 billion merger to form MCI WorldCom."

onthisday/events/09/01
"St. Petersburg, Russia, changes its name to Petrograd."

Bug: T175974

The `events` for the following days are examples having dupe pages:

`onthisday/events/09/14`
"Telecommunications companies MCI Communications and WorldCom complete their $37 billion merger to form MCI WorldCom."

`onthisday/events/09/01`
"St. Petersburg, Russia, changes its name to Petrograd."

Bug: T175974
v1/onthisday.js Outdated
@@ -26,7 +26,23 @@ class Feed extends BaseFeed {
[req.params.type]: res.body[req.params.type]
};
}
return super._hydrateResponse(hyper, req, res);
const hydratedResponse = super._hydrateResponse(hyper, req, res);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually faster to use Objects to determine whether a value should be filtered out:

return super._hydrateResponse(hyper, req, res).then((response) => {
  Object.keys(response.body).forEach((key) => {
    if (!Array.isArray(response.body[key])) {
      return;
    }
    response.body[key].forEach((elem) => {
      if (!elem || !Array.isArray(elem.pages)) {
        return;
      }
      const titles = {};
      elem.pages = elem.pages.filter((item) => {
        if (titles[item.title]) {
          return false;
        }
        titles[item.title] = true;
        return true;
      });
    });
  });
  return response;
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@d00rman
Updated! I kept the declarative naming, but I don't feel super strongly about it if you prefer it more concise.

@montehurd
Copy link
Contributor Author

@d00rman

Quick question - the REQUEST_TEMPLATE is set to...
{{options.host}}/{{domain}}/v1/feed/onthisday/all/{{mm}}/{{dd}}
...but shouldn't it be something like...
{{options.host}}/{{domain}}/v1/feed/onthisday/{type}/{{mm}}/{{dd}}?

(type being either births, deaths, events, holidays, selected or all)

@d00rman
Copy link
Contributor

d00rman commented Jan 5, 2018

@montehurd Nope, because we need all for storage purposes. When another type is requested, it's filtered out here so the client gets only what they asked for.

@d00rman d00rman merged commit c13582c into wikimedia:master Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants