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.

Sign up for GitHub

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

Jump to bottom

Add translation endpoint #345

Merged
merged 7 commits into from
Oct 28, 2017
Merged

Add translation endpoint #345

merged 7 commits into from
Oct 28, 2017

Conversation

erral
Copy link
Member

@erral erral commented May 2, 2017

See #225 (comment) for rational behind this.

@coveralls
Copy link

coveralls commented May 2, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 96.458% when pulling f74886c on issue-225-only-pam-plone5 into 9a11165 on master.

@coveralls
Copy link

coveralls commented May 2, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 96.458% when pulling f74886c on issue-225-only-pam-plone5 into 9a11165 on master.

@coveralls
Copy link

coveralls commented May 2, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 96.458% when pulling a7755f2 on issue-225-only-pam-plone5 into a4d49b2 on master.

@tisto
Copy link
Member

tisto commented May 8, 2017

@erral is this pr ready for review? If so please remove the "in progress" tag.

@erral
Copy link
Member Author

erral commented May 9, 2017

Yes, sorry, I was expecting some comments on #225 that's why I didn't remove the in-progress tag, but it's ready for review anyway.

@erral erral force-pushed the issue-225-only-pam-plone5 branch from a7755f2 to 88e1bdd Compare May 9, 2017 05:49
gforcada
gforcada reviewed May 9, 2017
View reviewed changes
CHANGES.rst Outdated
@@ -18,7 +21,7 @@ New Features:

New Features:

- Add @history endpoint.
7- Add @history endpoint.
Copy link
Member

Choose a reason for hiding this comment

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

Seems a typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. Fixed.

@erral erral force-pushed the issue-225-only-pam-plone5 branch from 88e1bdd to 29862ac Compare May 9, 2017 07:30
@coveralls
Copy link

coveralls commented May 10, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 96.463% when pulling 29862ac on issue-225-only-pam-plone5 into 8f66f72 on master.

@tisto tisto requested a review from sneridagh May 19, 2017 04:42
tisto
tisto reviewed Jun 10, 2017
View reviewed changes
docs/source/_json/translations_delete.req
Authorization: Basic YWRtaW46c2VjcmV0
Content-Type: application/json

{"language": "es"}
Copy link
Member

Choose a reason for hiding this comment

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

@erral cmon, you could at least use Basque or Catalan. ;)

@tisto
Copy link
Member

tisto commented Jun 10, 2017

@erral sorry to keep you waiting with this pr! @sneridagh would you mind having a look? You have far more experience with this.

@erral erral force-pushed the issue-225-only-pam-plone5 branch from 29862ac to 6b377bd Compare October 9, 2017 08:16
@coveralls
Copy link

coveralls commented Oct 9, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.07%) to 96.637% when pulling 6b377bd on issue-225-only-pam-plone5 into 88a6779 on master.

@tisto tisto changed the title Addresses #225 Add translation endpoint Oct 20, 2017
@erral
Copy link
Member Author

erral commented Oct 22, 2017 •
edited
Loading

Some weeks ago I worked on this again.

I limited the scope of @translations endpoint to be just Plone 5 compatible. The reason behind this is that Plone 4 doesn't have out of the box multilingual-content support: one needs to install LinguaPlone or p.a.m to achieve that.

Moreover, the existing ecosystem of different versions of p.a.m for Plone 4 (1.x and 2.x), require different test setups which I was unable to configure properly.

I think that, in an addon like plone.restapi, it's better to provide only support for something that it's directly supported on plain Plone.

Moreover I needed some multilingual content support for one of our clients in Plone 4 and LinguaPlone, and extracted this endpoint to 2 dedicated add-ons that just support Plone 4 with LinguaPlone and Plone 4 with p.a.m:

https://github.com/collective/collective.restapi.linguaplone
https://github.com/collective/collective.restapi.pam

With this 2 addons and the support for Plone 5 in this pull request, I think that the multilingual-content support should be complete.

tisto
tisto reviewed Oct 25, 2017
View reviewed changes
docs/source/translations.rst Outdated
.. note::
This is only available on Plone 5.


Copy link
Member

Choose a reason for hiding this comment

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

@erral a short introduction about Plone's multilingual capabilities would be nice. Keep in mind that the docs should work for JS devs who do not know the internals of Plone. One or two sentences are sufficient.

@tisto
Copy link
Member

tisto commented Oct 25, 2017

@sneridagh would you mind having a quick look at this?

@tisto tisto merged commit 8ad85be into master Oct 28, 2017
@jensens jensens deleted the issue-225-only-pam-plone5 branch April 7, 2020 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tisto tisto tisto left review comments

@gforcada gforcada gforcada left review comments

@sneridagh sneridagh Awaiting requested review from sneridagh

Assignees
No one assigned
Labels
04 type: enhancement 24 status: ready 32 needs: review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erral @coveralls @tisto @gforcada