Skip to content

Conversation

@XIDO-odoo
Copy link
Contributor

@XIDO-odoo XIDO-odoo commented Jun 10, 2025

Following the improvements made in Malaysia e-invoicing module, the following task is to update the improved features and additional flows involved.

@robodoo
Copy link
Collaborator

robodoo commented Jun 10, 2025

Pull request status dashboard

@XIDO-odoo XIDO-odoo marked this pull request as ready for review June 10, 2025 05:50
@C3POdoo C3POdoo requested review from a team June 10, 2025 05:52
Copy link
Contributor

@afma-odoo afma-odoo left a comment

Choose a reason for hiding this comment

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

Hi @XIDO-odoo! Thanks a lot for reviewing the page :)
I added a few comments and suggestions. Just let me know if you have any questions. Thank you! ☺️

Comment on lines 311 to 312
.. note::
The :guilabel:`Debit Notes module` must be installed to issue a debit note.
Copy link
Contributor

Choose a reason for hiding this comment

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

For technical reasons, I can't do it, but I think the module information should be added to the "Modules installation" section, and a reference to that section should be added to make it complete.

Suggested change
.. note::
The :guilabel:`Debit Notes module` must be installed to issue a debit note.
.. note::
The :ref:`Debit Notes module <malaysia/configuration/modules>` must be installed to issue a debit note.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I revised it according to your suggestion; but I'm not sure if I should if this should be added to the "Modules installation" section, because this is not a "localization" module of Malaysia.

Could you please check if this is suitable? Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right. I was going to ask my colleague what we should do in this case, but I tested it on a runbot first. The module is installed by default, so I don't think the note is necessary. What do you think?

@XIDO-odoo XIDO-odoo force-pushed the 17.0-Localization_MY-XIDO branch from 70d06b2 to ca905ea Compare June 27, 2025 08:28
@XIDO-odoo
Copy link
Contributor Author

Hello @afma-odoo , thank you for the suggestion and comments; sorry for the late follow up.
I have revised it based on your suggestion, could you please have a check again?

Thank you!

@XIDO-odoo XIDO-odoo force-pushed the 17.0-Localization_MY-XIDO branch from ca905ea to 39f48e0 Compare June 27, 2025 08:47
Copy link
Contributor

@afma-odoo afma-odoo left a comment

Choose a reason for hiding this comment

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

Thank you @XIDO-odoo for the updates. I just have some minor suggestions.
Please let me know if you have any questions ;)
Thanks

Comment on lines 311 to 312
.. note::
The :guilabel:`Debit Notes module` must be installed to issue a debit note.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right. I was going to ask my colleague what we should do in this case, but I tested it on a runbot first. The module is installed by default, so I don't think the note is necessary. What do you think?

@XIDO-odoo XIDO-odoo force-pushed the 17.0-Localization_MY-XIDO branch from 39f48e0 to 283ed26 Compare June 30, 2025 09:09
@XIDO-odoo
Copy link
Contributor Author

Hello @afma-odoo ,

Thank you for the follow up!

I have revised it based on your suggestion.

If you have any other comments or feedback, please just let me know. Thank you!

Copy link
Contributor

@afma-odoo afma-odoo left a comment

Choose a reason for hiding this comment

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

Thank you @XIDO-odoo for the update!
I approve the review and ping be-doc-review for the final review :)

@afma-odoo afma-odoo requested a review from a team June 30, 2025 11:04
Copy link
Contributor

@auva-odoo auva-odoo left a comment

Choose a reason for hiding this comment

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

Hey @XIDO-odoo thank you for your work on this! I added a few minor comments in my review. Also:

  • I see there's a conflict on this PR, could you please resolve it?
  • I know you didn't change that part but I noticed the Malaysia - E-invoicing Extended Features module (l10n_my_edi_extended) is missing from the list; should we add it?

Thanks again! And thank you @afma-odoo for the thorough review 🙏

@XIDO-odoo
Copy link
Contributor Author

Hello @auva-odoo , thank you for the feedback.

I have revised it based on your suggestions; but for the first point "conflict on this PR", could you please help check if it has been resolved.

If there are any other points necessary to be revised, please just let me know.

Thank you!

cc: @afma-odoo

@auva-odoo auva-odoo force-pushed the 17.0-Localization_MY-XIDO branch from ff9d1c1 to 7513ea2 Compare July 8, 2025 15:27
@auva-odoo auva-odoo changed the title [IMP] Improve Malaysia localization modules documentation [IMP] Localizations/Malaysia: improve doc Jul 8, 2025
@auva-odoo auva-odoo force-pushed the 17.0-Localization_MY-XIDO branch from 7513ea2 to c768c8c Compare July 9, 2025 06:48
Copy link
Contributor

@auva-odoo auva-odoo left a comment

Choose a reason for hiding this comment

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

Hey @XIDO-odoo I squashed your 2 commits, fixed the conflict in the process, and fixed an issue with an anchor. Could you please check everything looks good on your side? If it is, you can r+ the PR :) Thank you for your work again!

@robodoo delegate+

@XIDO-odoo
Copy link
Contributor Author

Hello @auva-odoo , thank you for your help! This is good on my side. Please allow me to r+ it.
cc: @afma-odoo

@robodoo r+

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.

5 participants