-
Notifications
You must be signed in to change notification settings - Fork 77
BREAKING CHANGE: refactor labels with the new architecture #1784
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
Changes from all commits
174afb7
1e258d0
56d1230
458e5b6
c6b4a4f
9bdef33
34d2629
21e3e41
f43be53
ca8561c
92312e9
9a4901c
fd7f307
c695476
fe2d820
b400b71
2c7a0eb
c33514f
fadd877
24f9c12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,16 @@ | ||
| import 'package:bb_mobile/core/exchange/domain/entity/order.dart'; | ||
| import 'package:bb_mobile/core/exchange/domain/usecases/list_all_orders_usecase.dart'; | ||
| import 'package:bb_mobile/core/labels/data/label_datasource.dart'; | ||
| import 'package:bb_mobile/core/labels/domain/batch_labels_usecase.dart'; | ||
| import 'package:bb_mobile/core/labels/domain/label.dart'; | ||
| import 'package:bb_mobile/core/labels/label_system.dart'; | ||
| import 'package:bb_mobile/core/utils/logger.dart'; | ||
| import 'package:bb_mobile/features/labels/labels_facade.dart'; | ||
|
|
||
| class LabelExchangeOrdersUsecase { | ||
| final LabelDatasource _labelDatasource; | ||
| final BatchLabelsUsecase _batchLabelsUsecase; | ||
| final LabelsFacade _labelsFacade; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should not directly import a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This usecase is out of the scope as it belongs to the exchange feature. This PR focus on refactoring the labels folder
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This usecase is out of the scope as it belongs to the exchange feature. This PR focus on refactoring the labels folder
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh ok, just wrote another comment saying the same and only now see you responded haha.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will refactor this usecase in when we attack the exchange feature |
||
| final ListAllOrdersUsecase _listAllOrdersUsecase; | ||
|
|
||
| LabelExchangeOrdersUsecase({ | ||
| required LabelDatasource labelDatasource, | ||
| required BatchLabelsUsecase batchLabelsUsecase, | ||
| required LabelsFacade labelsFacade, | ||
| required ListAllOrdersUsecase listAllOrdersUsecase, | ||
| }) : _labelDatasource = labelDatasource, | ||
| _batchLabelsUsecase = batchLabelsUsecase, | ||
| }) : _labelsFacade = labelsFacade, | ||
| _listAllOrdersUsecase = listAllOrdersUsecase; | ||
|
|
||
| Future<void> execute() async { | ||
|
|
@@ -59,7 +53,7 @@ class LabelExchangeOrdersUsecase { | |
| } | ||
| } | ||
|
|
||
| await _batchLabelsUsecase.execute(labels); | ||
| await _labelsFacade.store(labels); | ||
|
|
||
| log.fine( | ||
| '$LabelExchangeOrdersUsecase labeled ${orders.length} exchange orders', | ||
|
|
@@ -71,11 +65,11 @@ class LabelExchangeOrdersUsecase { | |
|
|
||
| Future<bool> _hasExistingExchangeSystemLabels() async { | ||
| try { | ||
| final allLabels = await _labelDatasource.fetchAll(); | ||
| final allLabels = await _labelsFacade.fetch(); | ||
| return allLabels.any( | ||
| (label) => | ||
| label.label == LabelSystem.exchangeBuy.label || | ||
| label.label == LabelSystem.exchangeSell.label, | ||
| LabelSystem.isSystemLabel(label) && | ||
| LabelSystem.fromLabel(label).isExchangeRelated(), | ||
| ); | ||
| } catch (e) { | ||
| log.warning('$LabelExchangeOrdersUsecase: $e'); | ||
|
|
||
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
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.
I think this is not a good usecase, this labeling should be part of the usecase that creates the orders.
But I don't know if we should refactor that when we apply the new architecture guidelines to the exchange features and we should focus on the labels feature here.
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.
The labeling is already part of the usecase that creates orders. This usecase is executed once the first time the user connect to the exchange to ensure it is labeling the orders that might have been created before the app was supporting the exchange.
The day we attack the exchange folder, let's rename it better!
LabelExistingExchangeOrdersUsecase ?
Uh oh!
There was an error while loading. Please reload this page.
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.
I was thinking some more about this and I actually don't even think this should be done from the exchange feature. In the end it is the labels feature that needs a migration of the labels, or a seeding in some sense of missed values. So I was thinking that we should actually remove this usecase and let the labels feature use the core BBX api client directly to fetch the orders and seed the labels in a real db migration/seeding in the labels feature. It is a pure migration (framework) thing from the labels, no usecase needed and certainly not in exchange features.