Skip to content

[FIX] app/accounting: Fix outdated Colombian test server url #1125

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
Closed
wants to merge 1 commit into from
Closed

[FIX] app/accounting: Fix outdated Colombian test server url #1125

wants to merge 1 commit into from

Conversation

@raf-odoo
Copy link
Contributor

@raf-odoo raf-odoo commented Sep 9, 2021

This is the V14 followup of #1107 . V14 has two different services which can be used, because Carvajal split into 2 entities.

@robodoo
Copy link
Collaborator

robodoo commented Sep 9, 2021

Pull request status dashboard.

@raf-odoo
Copy link
Contributor Author

raf-odoo commented Sep 9, 2021

@vbe-odoo I have a couple of questions about this one:

  • I noticed that the text mentions "Carvajal T&S". Is that actually the same as the "CTS" I am referring to and which is mentioned in task 2508208? Should I use a different naming than the one I used?
  • I see there's a colombia_ES.rst in addition to colombia.rst. Should I update that one as well? My Spanish would be Google Translate quality :-)

@vbe-odoo
Copy link
Contributor

vbe-odoo commented Sep 10, 2021 •
edited
Loading

Hi @raf-odoo
Thanks for asking. About your questions:

  • I noticed that the text mentions "Carvajal T&S". Is that actually the same as the "CTS" I am referring to and which is mentioned in task 2508208? Should I use a different naming than the one I used?

Yes, "Carvajal T&S" is same as "CTS" but "CSC" is "Carvajal Servicios de Comunicación".
I think we can just leave Carvajal (Group) where is add it in general as that's how is most common for customer to call it.
But I will add in parenthesis - next to the titles added by you (see comment added by me there) the full name of Carvajal depending on the Web Service (they have 2 names depending on the Web Service)
See my comments on the file changes to understand better this point.

  • I see there's a colombia_ES.rst in addition to colombia.rst. Should I update that one as well? My Spanish would be Google Translate quality :-)

Yes, we should update it, you can add it in ENG or use google translate and I can help you with the corrections hehe, just ping me.

Many thanks!

vbe-odoo
vbe-odoo reviewed Sep 10, 2021
View reviewed changes
content/applications/finance/accounting/fiscal_localizations/localizations/colombia.rst Outdated
Carvajal CTS
https://cenflab.cen.biz/site/

Carvajal CSC
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it could be:
Carvajal CSC (Carvajal Servicios de Comunicación)

@raf-odoo raf-odoo force-pushed the 14.0-correct-colombian-test-servers-raf branch from 4cd5411 to 15dd456 Compare September 13, 2021 07:46
@raf-odoo
Copy link
Contributor Author

I noticed that in the Spanish document it also made reference to the production server URL, but that the URL was also wrong (it was the test one twice). I decided to keep it consistent with the rest of the documentation and only mention the test server URL's.

@vbe-odoo
Copy link
Contributor

I noticed that in the Spanish document it also made reference to the production server URL, but that the URL was also wrong (it was the test one twice). I decided to keep it consistent with the rest of the documentation and only mention the test server URL's.

Hi @raf-odoo awesome, I saw your changes in the colombia_ES.rst and LGTM, thanks for keeping it consistent in both DOCs.
From my side this is ready to go.
Thanks!

@raf-odoo
Copy link
Contributor Author

@vbe-odoo Ok, thanks for the feedback :-)

@raf-odoo
Copy link
Contributor Author

@odoo/doc-review This is the V14 follow-up of #1107 .

@raf-odoo
Copy link
Contributor Author

I also double-checked at which point colombia_ES.rst was introduced, and it was already there in V12. So I'll do an additional V12-V13 PR to apply the changes from #1107 over there as well.

@raf-odoo raf-odoo mentioned this pull request Sep 14, 2021
Closed
Feyensv
Feyensv reviewed Sep 14, 2021
View reviewed changes
Copy link
Collaborator

@Feyensv Feyensv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Commit message could be more detailed for people not knowing colombian localization:

  • include related R&D task ?
  • explain the T&S removal: It's not clear how it is linked to the commit title about the test url. I saw the explanation on the PR, but it could be explained in commit as well :)

But content change seems safe, only made a little remark on title headers :)

@raf-odoo
[FIX] app/accounting: Fix outdated Colombian test server url
cc96ea0 
The test server url for Carvajal T&S was out of date and has been
updated with the correct one. Carvajal has also split into 2
different companies, with new customers using the new company (CSC)
by default but the older company (CTS) still being available for
existing customers. See task 2508208. So the documentation was
updated to include both of them.

CTS (Carvajal T&S) was being used to refer to Carvajal in general,
so to avoid confusion those mentions have been changed to just
"Carvajal" since it can refer to either CTS or CSC.
@raf-odoo raf-odoo force-pushed the 14.0-correct-colombian-test-servers-raf branch from 15dd456 to cc96ea0 Compare September 14, 2021 13:16
@raf-odoo
Copy link
Contributor Author

Commit message could be more detailed for people not knowing colombian localization:

@Feyensv Good point, I've updated the commit message to include your suggestions.

@Feyensv
Copy link
Collaborator

Feyensv commented Sep 14, 2021

@robodoo r+

@robodoo robodoo closed this in 396df2a Sep 14, 2021
@fw-bot fw-bot mentioned this pull request Sep 14, 2021
Closed
@robodoo robodoo temporarily deployed to merge September 14, 2021 14:27 Inactive
@fw-bot fw-bot mentioned this pull request Sep 14, 2021
Closed
@fw-bot
Copy link
Collaborator

fw-bot commented Sep 18, 2021

This pull request has forward-port PRs awaiting action (not merged or closed): #1139

2 similar comments
@fw-bot
Copy link
Collaborator

fw-bot commented Sep 19, 2021

This pull request has forward-port PRs awaiting action (not merged or closed): #1139

@fw-bot
Copy link
Collaborator

fw-bot commented Sep 20, 2021

This pull request has forward-port PRs awaiting action (not merged or closed): #1139

@fw-bot fw-bot deleted the 14.0-correct-colombian-test-servers-raf branch September 28, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Feyensv Feyensv Feyensv left review comments

@vbe-odoo vbe-odoo vbe-odoo left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@raf-odoo @robodoo @vbe-odoo @Feyensv @fw-bot