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

Combine add delete translations views #243

Merged
merged 16 commits into from Oct 26, 2016

Conversation

@agitator
Copy link
Member

commented Jul 28, 2016

No description provided.

@agitator agitator force-pushed the combine-add-delete-translations-views branch from 34e0c0e to f7d0d13 Oct 19, 2016

agitator added 3 commits Oct 20, 2016
setup.py Outdated
@@ -41,6 +41,7 @@
'plone.app.z3cform',
'plone.behavior',
'plone.dexterity',
'plone.api',

This comment has been minimized.

Copy link
@gforcada

gforcada Oct 21, 2016

Contributor

plone.api can not be used on core packages, as sad as it is that's the decision so far... maybe the @plone/framework-team wants to revisit this at some point :-)

agitator and others added 7 commits Oct 22, 2016
@jensens

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

Since this is a recommended shipped addon for core I would say it's ok. The
difficulty is to identify which packages in core are consumers of plone.api
and which are producers. What do others think?

Am 21. Oktober 2016 7:37:52 vorm. schrieb Gil Forcada Codinachs
notifications@github.com:

gforcada commented on this pull request.

@@ -41,6 +41,7 @@
'plone.app.z3cform',
'plone.behavior',
'plone.dexterity',

  •    'plone.api',
    

plone.api can not be used on core packages, as sad as it is that's the
decision so far... maybe the @plone/framework-team wants to revisit this at
some point :-)

You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub:
#243 (review)

@thet

This comment has been minimized.

Copy link
Member

commented Oct 26, 2016

LGTM

@thet thet merged commit 30d5270 into master Oct 26, 2016

6 checks passed

Changelog verifier Entry found
Details
Plone Contributors Agreement verifier All users have signed it
Details
Plone Jenkins CI - pull-request-5.0 Job finished with success status
Details
Plone Jenkins CI - pull-request-5.1 Job finished with success status
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@thet thet deleted the combine-add-delete-translations-views branch Oct 26, 2016

@mauritsvanrees

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

Since this has been merged, Jenkins on Plone 5.1 throws a test failure off and on:

http://jenkins.plone.org/job/plone-5.1-python-2.7-robot/1017/robot/Robot/Robot/Test%20Add%20Translation/Scenario%3A%20As%20an%20editor%20I%20can%20add%20new%20translation/

WebDriverException: Message: arguments[0] is undefined

Can someone have a look?
Possibly the solution is to use a newer (or older) Selenium or Firefox or whatever.

@jensens

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

Ok, just looked at the robot changes, there is nothing which uses a newer Selenium of FF. Its just about a bit different selectors used according to the new view.

This more looks like a problem upstream in the overall testing. Also, IIRC, the test was green right before merge.

And, no, I have no clue what's going on.

@mauritsvanrees

This comment has been minimized.

Copy link
Member

commented Nov 1, 2016

I see Gil is trying to fix it on master. I have now also tried it with a fix, and have good hope that it helps.

The theory is: if you are using 'Wait until element is visible' to wait for an element that was not on the page at initial load, then you must first use 'Wait until page contains element', otherwise you get:

Message: Element not found in the cache - perhaps the page has changed since it was looked up

Now I look at it, that is actually a different error than I saw earlier, mentioned above... Well, let's see.

@mauritsvanrees

This comment has been minimized.

Copy link
Member

commented Nov 1, 2016

Okay, that helped on Jenkins. Well, for 5.1 anyway. 5.0 has the same error... I am rebuilding now just to see if that helps.

@tmassman

This comment has been minimized.

Copy link
Member

commented on f7d0d13 Dec 14, 2016

@agitator, can we revert this since plone/Products.CMFPlone#1694 has been closed and is not going to be merged? Right now the translate menu is missing the icon.

This comment has been minimized.

Copy link
Member Author

replied Dec 14, 2016

yes, that's not necessary anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.