Crowdin API v2, OAuth v2, open/save/sync as Xliff all files with translations #629

berezins · 2020-04-24T12:37:31Z

Added support (from customer's prospective):

Crowdin for enterprise
- Works with Crowdin Enterprise accounts of any organization (on any domains)
Open/save/sync as Xliff any file which can be parsed for translations by Crowdin

Improved

Path to locally stored Crowdin files contains <project ID> <project name>/<lang code>/<branch><path to file in Crowdin project>

Migrated to new:

Also parsed, checked/validated

- So it shows that authenticated and full name of the user at the end of authentication process (when Authorize button pressed on authorization page) - But refresh_token is not implemented yet (so need to be authorized again each time once expired)

vslavik · 2020-04-24T13:01:49Z

Thanks!

Any chance you could break it into topical, one PR per one thing, PRs, though? This is huge, with several unrelated topics and apparently work-in-progress commits like fixing previous ones or "playing" with stuff, "work in progress" etc., which makes it extremely hard to review.

E.g. 6407583 does many unrelated things, none seem at a glance related to what it says it does. Some, like de223d8 or the other version changes, don't make sense to me.

src/http_client_casablanca.cpp

src/crowdin_gui.cpp

berezins · 2020-04-24T20:11:19Z

Any chance you could break it into topical, one PR per one thing, PRs, though?

May be rather as single PR but just with more granular and stepped commits just without "work in progress", "playing with stuff" and mixing with version update and other unrelated topics per commit? Since nevertheless most of things in this PR are highly correlated and dependent each from other so without many of commits some of functionalities or improvements would be just broken and incomplete so it might be even most complicated to review one PR without other. Goog examples:

New API v2 does not work with old OAuth since they just does not accept token from it
New OAuth v2 does not work with old API v1 since they just does not accept token from it.
.po saving/syncing does not work with new API v2 due API v2 does not include file and project ID as x-headers inside .po
Open/save/sync as Xliff to/from Crowdin does no work with old API v1 since they just does not allow to download or upload any translatable file as Xliff

Thought in any case I will try to regroup/rearrange/split/squash commits by real mostly complete functionalities/improvements within next week and will let you know once done. And if you will still prefer those already rearranged commits to be spread as separate PRs nevertheless - I will do this as well just to follow and reach your expectations to simplify review

vslavik · 2020-04-25T11:03:27Z

May be rather as single PR but just with more granular and stepped commits

Yes, you're right. What I'm after is having clearly separated smaller chunks that are self-contained (except having dependencies on prior commits), do exactly one thing, and are easier to review as a whole: I can assess that this part is OK, I understood the whole picture and focus on the rest now. For example, there are some independent bits that the rest depends on, such as http_client additions, new dependency additions (those don't make sense on their own, sure, but touch so many things it's better to just keep them in a single commit).

I'll try to do some initial review as-is, but please keep in mind that it's going to be superficial, i.e. focusing on the things easily spotted in a wall-of-diff, rather than on actually important higher-level review. Apologies for that in advance.

vslavik

Again, please treat this as just superficial review for things I noticed when reading them mostly out of context. Consequently, it's entirely possible I'm wrong about things.

src/crowdin_client.cpp

src/http_client.h

src/http_client_casablanca.cpp

src/crowdin_client.cpp

berezins · 2020-04-26T06:32:33Z

I'll try to do some initial review as-is, but please keep in mind that it's going to be superficial, i.e. focusing on the things easily spotted in a wall-of-diff, rather than on actually important higher-level review. Apologies for that in advance.

May be better for you to hold on with reviewing as is until I rearrange according to your expectations (hopefully within first couple days of this week, that is why I made this PR as draft for a while just to better get your opinion/impression/expectations). So you would just save your time and efforts while doing what you really like to focus on during review and having more consistent picture of what is why and for what.
Anyway of course feel free to continue as much as you feel it could be fully productive+comfortable for you with that as is now but I would just hold on replying on some comments where I see you could get better consistent picture after commits rearrangement to make discussion and making final decision more effectively.
Thanks a lot!

vslavik · 2020-04-26T09:44:54Z

May be better for you to hold on with reviewing

Sure. I figured it was worth getting some sense of the scope and comment on trivia that are ideally dealt with precisely when cleaning up stuff.

Misc improvements

then in deps/wx

i.e. which are not authorized to get file list etc.

from temporary arbitrary URL returned by API

In dropdown list of "Open from Crowdin" dialog

Point to the latest 1.1.1g release. Also changes Xcode includes to use openssl/boringssl to fix compilation. Fixes omission in 7812401.

i.e. `{` from new line unless omitted due of single-line statement

src/crowdin_client.cpp

berezins · 2020-05-24T08:20:17Z

Closing this in favour of #633 as to final clean version of this (as @vslavik mentioned by email it makes sense) so all further work (final review, discussion, refinements etc) moving to there

berezins added 2 commits February 26, 2020 00:10

Auth code received back to app

701eb4c

Also parsed, checked/validated

Works new OAuth and one new API /user

2cf82b4

- So it shows that authenticated and full name of the user at the end of authentication process (when Authorize button pressed on authorization page) - But refresh_token is not implemented yet (so need to be authorized again each time once expired)

vslavik reviewed Apr 24, 2020

View reviewed changes

src/http_client_casablanca.cpp Outdated Show resolved Hide resolved

vslavik reviewed Apr 24, 2020

View reviewed changes

src/crowdin_gui.cpp Show resolved Hide resolved

vslavik reviewed Apr 25, 2020

View reviewed changes

berezins force-pushed the crowdin-new-oauth-api-xliff branch 4 times, most recently from 4a71270 to 5198397 Compare May 6, 2020 12:41

berezins requested a review from vslavik May 6, 2020 12:53

berezins force-pushed the crowdin-new-oauth-api-xliff branch 2 times, most recently from d23e847 to 4a4fe79 Compare May 8, 2020 07:22

berezins added 4 commits May 8, 2020 10:32

API v2 work in progress

136a3bc

Played with Crowdin personal support (partially done)

f03b9b5

Download and open file in Xliff, OAuth improvements

0c9b3dd

Misc improvements

Local path matches path in Crowdin

9ea5c9d

berezins force-pushed the crowdin-new-oauth-api-xliff branch 6 times, most recently from 6b26568 to 5964579 Compare May 14, 2020 12:59

berezins force-pushed the crowdin-new-oauth-api-xliff branch 3 times, most recently from e330ce8 to 54c6b05 Compare May 17, 2020 12:21

berezins and others added 22 commits May 17, 2020 17:45

OAuth to any organization or personal account

7eebef4

Crash on OAuth fixed

578d88f

Fixed empty first and last name in organization user

7ef949c

Hide assets from list

e1d06db

Fix compilation errors on newer wxWidgets versions

dff254f

then in deps/wx

Crashed when to Save file deleted on Crowdin side

29f72f3

Crashed on open cached Crowdin file if logged out

b86635e

Buildable on Windows after changes on Linux

b23ff83

Buildable on MacOS

cbc75a7

Refresh token

3c44157

Hide 'Forbidden' projects

e961f78

i.e. which are not authorized to get file list etc.

Domain agnostic file download

fece230

from temporary arbitrary URL returned by API

Full path in file list including branch

71ce35f

In dropdown list of "Open from Crowdin" dialog

Implicit conversion from json to json_data

0382c41

Buildable on Windows/macOS

75c8d72

hdrs_t=>headers changed

8eebba2

wstring=>string http_client::headers value type

f04c541

std explicitly

db71548

Actually add OpenSSL submodule

dfb0cab

Point to the latest 1.1.1g release. Also changes Xcode includes to use openssl/boringssl to fix compilation. Fixes omission in 7812401.

Logging wrapped and using wxLogTrace()

81432c9

Logging using wxLogTrace() as is (without wrapping)

f64e576

Predominant if/for/while formatting

a7ebaf8

i.e. `{` from new line unless omitted due of single-line statement

berezins force-pushed the crowdin-new-oauth-api-xliff branch from 79d5f05 to a7ebaf8 Compare May 17, 2020 15:06

vslavik reviewed May 17, 2020

View reviewed changes

src/crowdin_client.cpp Outdated Show resolved Hide resolved

vslavik reviewed May 17, 2020

View reviewed changes

src/crowdin_client.cpp Outdated Show resolved Hide resolved

berezins added 2 commits May 18, 2020 01:59

DownloadFile arrangements

199f937

Language is as arg of UploadFile()

3999ec0

berezins mentioned this pull request May 24, 2020

Crowdin API v2, allow editing of any files as XLIFF #633

Closed

berezins closed this May 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crowdin API v2, OAuth v2, open/save/sync as Xliff all files with translations #629

Crowdin API v2, OAuth v2, open/save/sync as Xliff all files with translations #629

berezins commented Apr 24, 2020 •

edited

Loading

vslavik commented Apr 24, 2020

berezins commented Apr 24, 2020 •

edited

Loading

vslavik commented Apr 25, 2020

vslavik left a comment

berezins commented Apr 26, 2020 •

edited

Loading

vslavik commented Apr 26, 2020

berezins commented May 24, 2020

Crowdin API v2, OAuth v2, open/save/sync as Xliff all files with translations #629

Crowdin API v2, OAuth v2, open/save/sync as Xliff all files with translations #629

Conversation

berezins commented Apr 24, 2020 • edited Loading

vslavik commented Apr 24, 2020

berezins commented Apr 24, 2020 • edited Loading

vslavik commented Apr 25, 2020

vslavik left a comment

Choose a reason for hiding this comment

berezins commented Apr 26, 2020 • edited Loading

vslavik commented Apr 26, 2020

berezins commented May 24, 2020

berezins commented Apr 24, 2020 •

edited

Loading

berezins commented Apr 24, 2020 •

edited

Loading

berezins commented Apr 26, 2020 •

edited

Loading