Skip to content

Provide page id and revision timestamp in revision data end point#231

Merged
gwicke merged 3 commits intowikimedia:masterfrom
JonCook:master
May 7, 2015
Merged

Provide page id and revision timestamp in revision data end point#231
gwicke merged 3 commits intowikimedia:masterfrom
JonCook:master

Conversation

@JonCook
Copy link
Copy Markdown
Contributor

@JonCook JonCook commented Apr 22, 2015

Relating to changes described in:
https://phabricator.wikimedia.org/T95364

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 86.85% when pulling 967fa02 on JonCook:master into 607b6bc on wikimedia:master.

1 similar comment
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 86.85% when pulling 967fa02 on JonCook:master into 607b6bc on wikimedia:master.

@gwicke
Copy link
Copy Markdown
Member

gwicke commented Apr 22, 2015

@JonCook, thank you for your patch! This looks good to go to me. The only blocker is that we'll need schema migration support for existing content before we can merge / deploy this. @eevans is currently working on https://phabricator.wikimedia.org/T75808, and is looking into support for adding columns in particular.

@gwicke
Copy link
Copy Markdown
Member

gwicke commented Apr 22, 2015

/cc @d00rman

Comment thread mods/page_revisions.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can be simply:

var redirect = dataResp.redirect !== undefined;

@d00rman
Copy link
Copy Markdown
Contributor

d00rman commented Apr 23, 2015

Looks really good @JonCook, thank you for contributing! As @gwicke mentioned, we need to implement and deploy schema migrations before we can deploy this change, so we are leaving it open until then. Minor nitpicks in-lined.

@JonCook
Copy link
Copy Markdown
Contributor Author

JonCook commented Apr 23, 2015

Thanks for the comments. I'll fix the minor nitpicks later. The indentation is a bit strange as well as it looks ok in my text editor.

@d00rman
Copy link
Copy Markdown
Contributor

d00rman commented Apr 23, 2015

The indentation is a bit strange as well as it looks ok in my text editor.

Might be a tabs vs spaces issue

Correct indentation.
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 86.85% when pulling 3df3fe1 on JonCook:master into 607b6bc on wikimedia:master.

@gwicke
Copy link
Copy Markdown
Member

gwicke commented Apr 23, 2015

Might be a tabs vs spaces issue

Yeah, there are still some stray tabs in some of the test files. Lets clean that up in a separate patch.

Simplify redirect property boolean retrieval logic.
@JonCook
Copy link
Copy Markdown
Contributor Author

JonCook commented Apr 23, 2015

I updated the minor nitpicks. Hope it is ok. Think it is ready to go now when you've finished the schema migration changes.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 86.85% when pulling f5a1e0b on JonCook:master into 607b6bc on wikimedia:master.

@gwicke
Copy link
Copy Markdown
Member

gwicke commented May 2, 2015

@JonCook, the schema support has landed now, but we'll hold of with merging this until after the Monday deploy to reduce the amount of change we deploy at once.

@JonCook
Copy link
Copy Markdown
Contributor Author

JonCook commented May 5, 2015

@gwicke Did the Monday deploy go ok? Is it safe for you guys to go ahead with this merge?

@gwicke
Copy link
Copy Markdown
Member

gwicke commented May 5, 2015

@JonCook, the Monday deploy went well, and I just merged the capability to actually add columns. We'll be doing one more deploy tomorrow before likely pushing this out on Thursday.

gwicke added a commit that referenced this pull request May 7, 2015
Provide page id and revision timestamp in revision data end point
@gwicke gwicke merged commit 6d439ae into wikimedia:master May 7, 2015
@gwicke
Copy link
Copy Markdown
Member

gwicke commented May 7, 2015

@JonCook, after some testing your code went live 15 minutes ago. It's all looking great, so thanks a lot for your contribution! The next step will now be to leverage this information in HTML dumps (https://phabricator.wikimedia.org/T93396 and https://github.com/wikimedia/htmldumper).

@JonCook
Copy link
Copy Markdown
Contributor Author

JonCook commented May 8, 2015

Great, thanks. Glad I could help/contribute!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants