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

Enable history page for OFS.Image.File #396

Conversation

perrinjerome
Copy link
Contributor

Use the standard history page, enabled using the same condition than the one
that enables the text area on edit (ie. the file must be text or javascript and
reasonably sized)

@perrinjerome
Copy link
Contributor Author

I remembered I also had this old patch. I understand history support is removed from master ( #346 ), but maybe this sill makes sense for 2.13 branch. If not, then don't hesitate to close this pull request.

There's the same problem that tests are broken for 2.13 because of zc.recipe.egg version conflict. and there's no fix in this branch. Tests for this branch plus an extra commit to fix version conflicts are OK
https://travis-ci.org/perrinjerome/Zope/jobs/450638496

cc @icemac @leorochael

@icemac
Copy link
Member

icemac commented Nov 7, 2018

@perrinjerome Thank you for your pull request. Please rebase it to the current version of the 2.13 branch as I fixed TravisCI there in #393.

Additionally please sign a Zope Committer Agreement, see https://github.com/zopefoundation/developer_docs/blob/master/source/Zope_Foundation_Committer_Agreement-20181008.pdf

Use the standard history page, enabled using the same condition than the one
that enables the text area on edit (ie. the file must be text or javascript and
reasonably sized)
@perrinjerome perrinjerome force-pushed the feat/history_tab_for_ofs_file-2.13 branch from efb4cd3 to 2bdbfc3 Compare November 7, 2018 07:21
@perrinjerome
Copy link
Contributor Author

Thanks I rebased.

I remember now why I did not submit 4 years ago :) I'm trying to do this contributor agreement this time.

@perrinjerome
Copy link
Contributor Author

Thanks, I just submitted a signed contributor agreement.

@perrinjerome perrinjerome requested review from aclark4life, icemac and leorochael and removed request for aclark4life November 9, 2018 02:32
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

I am okay with this change, I also tested it in the ZMI.

Additionally I'd like to have a more proper test and a change log entry.

self.file.manage_historyCompare(
self.file,
self.app.b,
self.app.REQUEST)
Copy link
Member

Choose a reason for hiding this comment

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

The test should assert something in the return value of the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return value of this method is the full html page. The diff is presented in an HTML table:

<table border=1>
<tr>
<td><pre>
-
+
</pre></td>
<td><pre>
content_of_a
content_of_b
</pre></td>
</tr>

</table>

... which makes it not so easy to test.

I added simple assertions that the two versions of the content are present in the page.

add some assertions on the html returned by manage_historyCompare
@perrinjerome
Copy link
Contributor Author

Thanks for feedback @icemac !

I extended a bit the test and added a ChangeLog entry.

icemac
icemac previously requested changes Nov 19, 2018
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

Thank you for improving the test and adding a change log entry. I only have some little remaining suggestions.

src/OFS/tests/testFileAndImage.py Show resolved Hide resolved
src/OFS/tests/testFileAndImage.py Show resolved Hide resolved
@icemac icemac dismissed their stale review November 19, 2018 08:05

We still to have support Python 2.6.

Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

LGTM feel free to merge the PR.

@perrinjerome
Copy link
Contributor Author

Thanks for feedback,I'm merging this !

@perrinjerome perrinjerome merged commit fb840ce into zopefoundation:2.13 Nov 20, 2018
@perrinjerome
Copy link
Contributor Author

This change was only on 2.13 and not on Zope 5, I pushed a slightly updated version as #1245

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants