-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
refactor(ChartDataCommand): remove create queryContext command's responsibly #17461
refactor(ChartDataCommand): remove create queryContext command's responsibly #17461
Conversation
091b379
to
ebb96c3
Compare
… context from command
ebb96c3
to
e56fa3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov Report
@@ Coverage Diff @@
## master #17461 +/- ##
==========================================
- Coverage 76.94% 76.72% -0.23%
==========================================
Files 1042 1042
Lines 56299 56312 +13
Branches 7793 7793
==========================================
- Hits 43322 43208 -114
- Misses 12721 12848 +127
Partials 256 256
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
… context from command (#17461)
Background
When we have worked on #16991 we wanted to test the new functionalities in concrete and accurate unittest.
All chartData flows and its components are too couple to superset so it is impossible to create unittests.
The flows are not testable and so many components do not meet the very important principle SRP and the code became so dirty
So I've started to refactor it (#17344 ) but many changes were added and it was hard to review so I decided to split those changes into small PRs so will be easier to follow
This is the sixth PR in a sequence of PRs to meet these
The next PR is #17465
PR description
move create queryContext command's responsibility to the command clients (ChartDataRestApi and load_chart_data_into_cache celery task) to achieve meeting the SRP
Note - moving it to the view layer is an anti-pattern but remember, the refactor is combination of many PRs, so it'll be changed in one of the next PRs
Note 2 - the more optimal design is to encapsulate it under an ad-hoc object or keep refactoring the CommandaData into three separated commands
Test plans
There are no logic changes, only calling ordering so new tests are not required
Previous PRs