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

feat: #132 - new method OpenFoodAPIClient.addProductFields #436

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions lib/openfoodfacts.dart
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import 'package:openfoodfacts/model/TaxonomyLabel.dart';
import 'package:openfoodfacts/model/TaxonomyLanguage.dart';
import 'package:openfoodfacts/model/TaxonomyPackaging.dart';
import 'package:openfoodfacts/utils/AbstractQueryConfiguration.dart';
import 'package:openfoodfacts/utils/AddableProductFields.dart';
import 'package:openfoodfacts/utils/CountryHelper.dart';
import 'package:openfoodfacts/utils/ImageHelper.dart';
import 'package:openfoodfacts/utils/OcrField.dart';
Expand Down Expand Up @@ -131,6 +132,37 @@ class OpenFoodAPIClient {
return Status.fromApiResponse(response.body);
}

/// Add the given product to the database.
/// Returns a Status object as result.
static Future<Status> addProductFields(
final User user,
final String barcode,
final Map<AddableProductField, String> map, {
QueryType? queryType,
}) async {
if (map.isEmpty) {
throw Exception('Please add at least one AddableProductField');
}

final Map<String, String> parameterMap = <String, String>{};
parameterMap.addAll(user.toData());
Copy link
Contributor

Choose a reason for hiding this comment

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

By curiosity, why don't you call Map.from(user.toData)?

parameterMap['code'] = barcode;
for (final MapEntry<AddableProductField, String> entry in map.entries) {
parameterMap[entry.key.addableKey] = entry.value;
}

final Response response = await HttpHelper().doPostRequest(
UriHelper.getPostUri(
path: '/cgi/product_jqm2.pl',
queryType: queryType,
),
parameterMap,
user,
queryType: queryType,
);
return Status.fromApiResponse(response.body);
}

/// Send one image to the server.
/// The image will be added to the product specified in the SendImage
/// Returns a Status object as result.
Expand Down
17 changes: 17 additions & 0 deletions lib/utils/AddableProductFields.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// Fields of a [Product] that can be simply added.
enum AddableProductField {
BRANDS,
STORES,
COUNTRIES,
}

extension AddableProductFieldExtension on AddableProductField {
static const Map<AddableProductField, String> _KEYS = {
AddableProductField.BRANDS: 'add_brands',
AddableProductField.STORES: 'add_stores',
AddableProductField.COUNTRIES: 'add_countries',
};

/// Returns the key of the product field
String get addableKey => _KEYS[this]!;
}
114 changes: 114 additions & 0 deletions test/api_addProductFields_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import 'package:openfoodfacts/openfoodfacts.dart';
import 'package:openfoodfacts/utils/AddableProductFields.dart';
import 'package:openfoodfacts/utils/OpenFoodAPIConfiguration.dart';
import 'package:openfoodfacts/utils/QueryType.dart';
import 'package:test/test.dart';
import 'test_constants.dart';

/// Unit/Integration tests around OpenFoodAPIClient.addProductFields
void main() {
OpenFoodAPIConfiguration.globalQueryType = QueryType.TEST;

group('$OpenFoodAPIClient add product fields', () {
const String barcode = '0048151623426';
const List<String> additionalValues = <String>['djobi', 'djoba'];
const OpenFoodFactsLanguage language = OpenFoodFactsLanguage.ENGLISH;
const User user = TestConstants.TEST_USER;
const Timeout timeout = Timeout(Duration(seconds: 90));

void _checkStatus(final Status status) {
expect(status.status, 1);
expect(status.statusVerbose, 'fields saved');
}

String? _getValue(
final AddableProductField field,
final Product product,
) {
switch (field) {
case AddableProductField.BRANDS:
return product.brands;
case AddableProductField.COUNTRIES:
return product.countries;
case AddableProductField.STORES:
return product.stores;
}
}

Future<void> _checkValue(
final AddableProductField field,
final String expectedValue,
) async {
final ProductQueryConfiguration configuration = ProductQueryConfiguration(
barcode,
language: language,
fields: [ProductField.ALL],
);

final ProductResult productResult = await OpenFoodAPIClient.getProduct(
configuration,
user: user,
);
expect(productResult.product, isNotNull);
final String? actualValue = _getValue(field, productResult.product!);
expect(actualValue, equalsIgnoringCase(expectedValue));
}

Future<void> _checkAddableField(
final AddableProductField field,
final Product initialProduct,
) async {
late Status status;
late Product product;

// from scratch
product = initialProduct;
status = await OpenFoodAPIClient.saveProduct(user, product);
_checkStatus(status);

// cumulative list
String expectedValue = _getValue(field, product)!;
Copy link
Contributor

@g123k g123k Apr 10, 2022

Choose a reason for hiding this comment

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

A StringBuffer would be better, since you concatenate strings multiple times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@g123k StringBuffer is never used in the whole project. Here we're talking about adding a String to a String twice, and checking the resulting value 3 times (that would mean 3 additional toString() calls with StringBuffer). In a test. That makes async calls to the back-end that take longer time than a String allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@g123k I had a hunch about StringBuffer not being significantly faster than String when you only concatenate two Strings and convert each time the result as a String.
I've coded a test with the 2 solutions: with String and with StringBuffer.

test code
      const String initialString = "init";
      const List<String> additionalValues = <String>['djobi', 'djoba'];
      final List<String> result = <String>[];

      late int start;
      late int end;

      result.clear();
      start = DateTime.now().microsecondsSinceEpoch;
      final StringBuffer asStringBuffer = StringBuffer(initialString);
      result.add(asStringBuffer.toString());
      for (final String additionalValue in additionalValues) {
        asStringBuffer.write(', $additionalValue');
        result.add(asStringBuffer.toString());
      }
      end = DateTime.now().microsecondsSinceEpoch;
      print('${end - start}: $result');

      result.clear();
      start = DateTime.now().microsecondsSinceEpoch;
      String asString = initialString;
      result.add(asString);
      for (final String additionalValue in additionalValues) {
        asString += ', $additionalValue';
        result.add(asString);
      }
      end = DateTime.now().microsecondsSinceEpoch;
      print('${end - start}: $result');

After several runs, I've noticed that the first block run is a bit slower than the other - regardless of StringBuffer and String. By approximately 1ms.
Can you afford that 1ms uncertainty for tests? I think so. If that's OK with you, please approve my PR. If you think there is a hidden performance potential, don't hesitate to keep this PR rejected and to run performance tests.


await _checkValue(field, expectedValue);

for (final String additionalValue in additionalValues) {
expectedValue += ', $additionalValue';

status = await OpenFoodAPIClient.addProductFields(
user,
barcode,
<AddableProductField, String>{field: additionalValue},
);
_checkStatus(status);

await _checkValue(field, expectedValue);
}
}

test(
'add brand',
() async => _checkAddableField(
AddableProductField.BRANDS,
Product(
barcode: barcode, lang: language, brands: 'Golden Cookies'),
),
timeout: timeout);

test(
'add country',
() async => _checkAddableField(
AddableProductField.COUNTRIES,
Product(
barcode: barcode, lang: language, countries: 'United States'),
),
timeout: timeout);

test(
'add stores',
() async => _checkAddableField(
AddableProductField.STORES,
Product(barcode: barcode, lang: language, stores: 'Intermarché'),
),
timeout: timeout);
});
}