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

[T-6490][17.0][ADD] crm_lead_attach_related_saleorder_report #12

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

Tisho99
Copy link

@Tisho99 Tisho99 commented Aug 9, 2024

No description provided.

Copy link

@luis-ron luis-ron left a comment

Choose a reason for hiding this comment

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

Functional Review: LGTM 👍🏻

Ask for a tecnical review if needed.

Ty

Copy link
Author

@Tisho99 Tisho99 left a comment

Choose a reason for hiding this comment

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

Changes applied

@Tisho99
Copy link
Author

Tisho99 commented Sep 9, 2024

@manuelregidor

I resolved all requested changes.

Do i need to add tests?

@manuelregidor
Copy link

@Tisho99 As far as I know, all the modules in Sygel repositories must have tests from now on, so please, add them (unless @ValentinVinagre says the opposite).

@Tisho99 Tisho99 force-pushed the 17.0-T-6490 branch 3 times, most recently from e82cf4a to b045ddf Compare September 10, 2024 10:32
@ValentinVinagre
Copy link

@Tisho99 do test's please 💘 . correct @manuelregidor 😄

Copy link

@manuelregidor manuelregidor left a comment

Choose a reason for hiding this comment

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

LGTM However, as some importantes core changes have been applied, should you ask for a second functional review?

@ValentinVinagre
Copy link

LGTM However, as some importantes core changes have been applied, should you ask for a second functional review?

I think that to be on the safe side it wouldn't be a bad idea to review it again.

@ValentinVinagre
Copy link

@luis-ron u could review again please?

Copy link

@luis-ron luis-ron left a comment

Choose a reason for hiding this comment

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

Functional review: LGTM 👍🏻

Copy link

@ValentinVinagre ValentinVinagre left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@ValentinVinagre ValentinVinagre merged commit e63cc68 into 17.0 Sep 25, 2024
4 checks passed
@ValentinVinagre ValentinVinagre deleted the 17.0-T-6490 branch September 25, 2024 07:27
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.

4 participants