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

Refactoring #24

Merged
merged 4 commits into from Aug 3, 2023
Merged

Refactoring #24

merged 4 commits into from Aug 3, 2023

Conversation

lartist
Copy link

@lartist lartist commented Aug 3, 2023

Questions Answers
Description? Mutualise CommitterRepository, add skipped test to implement, Inject appVersion in controller instead of getting from the global container, remove useless todo's
Type? refactor
BC breaks? no
Deprecations? no
Fixed ticket?
Sponsor company
How to test?

boherm
boherm approved these changes Aug 3, 2023
View reviewed changes
Copy link
Member

@boherm boherm left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@matks
Copy link
Contributor

matks commented Aug 3, 2023

@boherm @lartist Do we agree CI is enough to confirm this is working?

@lartist
Copy link
Author

lartist commented Aug 3, 2023

@boherm @lartist Do we agree CI is enough to confirm this is working?

Hi @matks
Currently there is no CI. I've just made a PR to process some check in CI. See #15

But I confirm we can merge it safely because I've run every tests in local and everything is ok !

@boherm
Copy link
Member

boherm commented Aug 3, 2023

Your work on the CI is merged.
If you want, you can rebase to have the CI running on this PR.

@boherm boherm merged commit 4cfe5b3 into PrestaShop:main Aug 3, 2023
3 checks passed
@boherm
Copy link
Member

boherm commented Aug 3, 2023

Thanks @lartist!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boherm boherm boherm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
PR Dashboard
Archived in project
Milestone
No milestone
3 participants
@lartist @matks @boherm