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 falining AT Collection creation when using api.content.create. #414

Merged
merged 6 commits into from
Sep 14, 2018

Conversation

gbastien
Copy link
Member

@gbastien gbastien commented Sep 6, 2018

I also added the AT/DX event creation related tests from PR #407.

With this, the Collection creation is correct.

Thank you,

Gauthier

@gbastien
Copy link
Member Author

gbastien commented Sep 6, 2018

Hi @idgserpro FYI

gbastien added a commit to collective/collective.eeafaceted.collectionwidget that referenced this pull request Sep 6, 2018

@unittest.skipUnless(HAS_PACONTENTYPES, 'Dexterity only')
def test_create_dx_event(self):
"""https://github.com/plone/plone.app.contenttypes/issues/465"""

Choose a reason for hiding this comment

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

@hvelarde what's the relationship between plone/plone.app.contenttypes#465 and plone.api? It seems to me more a problem of plone.app.contenttypes.

I think this test can continue but without reference to plone.app.contenttypes.

Copy link
Member

@hvelarde hvelarde Sep 6, 2018

Choose a reason for hiding this comment

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

IIRC, I was unable to reproduce it on a clean Plone installation; so maybe it's not related at all; we may remove the reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed the typos and run test for AT and DX and Travis is green.

I kept the test regarding the event creation, do not know if it is necessary now that we have a test with Collection creation for AT and DX?

Choose a reason for hiding this comment

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

@gbastien this event testing is necessary to avoid regression #364. I think it is necessary now only to remove the reference:

plone/plone.app.contenttypes#465

Choose a reason for hiding this comment

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

In fact what avoids #364 is the AT test. But it's good to have an event creation test with DX

Copy link
Member Author

Choose a reason for hiding this comment

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

OK so I would remove the test_create_at_event, unnecessary now because of test_create_collection?

Choose a reason for hiding this comment

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

@gbastien no. test_create_at_event avoid regression #364. test_create_collection not avoid that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, so let's merge? :-)

Choose a reason for hiding this comment

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

@gbastien I think it is necessary now only to remove the reference:

plone/plone.app.contenttypes#465

in docstring.

@@ -376,6 +376,64 @@ def test_create_at_with_title_in_request(self):

self.assertEqual(page.title, 'Test document')

@unittest.skipIf(HAS_PACONTENTYPES, 'Archetypes only')

Choose a reason for hiding this comment

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

@gbastien thinking minor, I think this test should run with dx too. Collection creation should work with dx. You can remove this condition and change the name of the test.

CHANGES.rst Outdated
@@ -21,6 +21,9 @@ Bug fixes:
- fix typos in doc strings
[tkimnguyen]

- Fix falining AT Collection creation when using api.content.create.
Copy link
Member

Choose a reason for hiding this comment

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

typo

@@ -376,6 +376,64 @@ def test_create_at_with_title_in_request(self):

self.assertEqual(page.title, 'Test document')

@unittest.skipIf(HAS_PACONTENTYPES, 'Archetypes only')
def test_create_at_collection(self):
"""Test create at Collecition."""
Copy link
Member

Choose a reason for hiding this comment

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

typo

@hvelarde
Copy link
Member

hvelarde commented Sep 6, 2018

@gbastien you need to run the Jenkins jobs also.


@unittest.skipUnless(HAS_PACONTENTYPES, 'Dexterity only')
def test_create_dx_event(self):
""" """
Copy link
Member

Choose a reason for hiding this comment

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

it doesn't make sense to have an empty docstring.

@gbastien
Copy link
Member Author

gbastien commented Sep 7, 2018

@hvelarde ready to merge ? ;-)

@hvelarde
Copy link
Member

I prefer to leave it to one @plone/framework-team member.

hvelarde added a commit to collective/collective.cover that referenced this pull request Sep 10, 2018
Copy link
Member

@hvelarde hvelarde left a comment

Choose a reason for hiding this comment

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

I can confirm this fixes indeed all previous issues.

@gforcada gforcada merged commit 3e38c4f into master Sep 14, 2018
@gforcada gforcada deleted the create_at_collection_fix branch September 14, 2018 07:15
@hvelarde
Copy link
Member

@plone/release-team can we have a new release, please?

@gforcada
Copy link
Member

@hvelarde done: https://pypi.org/project/plone.api/ 1.8.6

@hvelarde
Copy link
Member

@gforcada in fact, it's 1.8.5.

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.

4 participants