From 3aac5abd17c348f68b24dc8a45e368e75821dc3a Mon Sep 17 00:00:00 2001 From: Philipp Metzner Date: Tue, 1 Oct 2024 16:55:36 +0200 Subject: [PATCH 1/5] Use batched bulk creation for shipment details --- .../box_transfer/shipment/crud.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/back/boxtribute_server/business_logic/box_transfer/shipment/crud.py b/back/boxtribute_server/business_logic/box_transfer/shipment/crud.py index f98f13b5c..816dd42ec 100644 --- a/back/boxtribute_server/business_logic/box_transfer/shipment/crud.py +++ b/back/boxtribute_server/business_logic/box_transfer/shipment/crud.py @@ -264,22 +264,22 @@ def _update_shipment_with_prepared_boxes( ) ) details = [ - { - "shipment": shipment.id, - "box": box.id, - "source_product": box.product_id, - "source_location": box.location_id, - "source_size": box.size_id, - "source_quantity": box.number_of_items, - "created_by": user_id, - } + ShipmentDetail( + shipment=shipment.id, + box=box.id, + source_product=box.product_id, + source_location=box.location_id, + source_size=box.size_id, + source_quantity=box.number_of_items, + created_by=user_id, + ) for box in boxes ] _bulk_update_box_state( boxes=boxes, state=BoxState.MarkedForShipment, user_id=user_id, now=now ) - ShipmentDetail.insert_many(details).execute() + ShipmentDetail.bulk_create(details, batch_size=BATCH_SIZE) def _remove_boxes_from_shipment( From fa0b3f633afc1d5d360bed4cf30d5e08052ebc6f Mon Sep 17 00:00:00 2001 From: Philipp Metzner Date: Tue, 1 Oct 2024 17:00:17 +0200 Subject: [PATCH 2/5] Account for alphabetically sorted label identifiers The test used to be flaky, e.g. https://app.circleci.com/pipelines/github/boxwise/boxtribute/5511/workflows/bba1bc05-7b0c-4c85-a5b8-6d373a4106b3/jobs/30695 --- back/test/endpoint_tests/test_box.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/back/test/endpoint_tests/test_box.py b/back/test/endpoint_tests/test_box.py index a56cd4e5a..ae78c33a2 100644 --- a/back/test/endpoint_tests/test_box.py +++ b/back/test/endpoint_tests/test_box.py @@ -584,7 +584,10 @@ def test_box_mutations( "updatedBoxes": [ {"id": str(another_box["id"]), "location": {"id": another_location_id}} ], - "invalidBoxLabelIdentifiers": [created_box["labelIdentifier"], "99119911"], + # Identifiers will be alphabetically sorted in the response + "invalidBoxLabelIdentifiers": sorted( + [created_box["labelIdentifier"], "99119911"] + ), } # Test cases 8.2.1, 8.2.2., 8.2.11, 8.2.25 From fba7b298d4960b7607fe1d8377579736d5555f89 Mon Sep 17 00:00:00 2001 From: Philipp Metzner Date: Tue, 1 Oct 2024 17:07:09 +0200 Subject: [PATCH 3/5] Extend docs about Box.location and Box.product --- .../graph_ql/definitions/protected/types.graphql | 2 ++ front/src/types/generated/graphql.ts | 2 ++ 2 files changed, 4 insertions(+) diff --git a/back/boxtribute_server/graph_ql/definitions/protected/types.graphql b/back/boxtribute_server/graph_ql/definitions/protected/types.graphql index 1fc146371..98579289b 100644 --- a/back/boxtribute_server/graph_ql/definitions/protected/types.graphql +++ b/back/boxtribute_server/graph_ql/definitions/protected/types.graphql @@ -26,9 +26,11 @@ type Box implements ItemsCollection { id: ID! " Sequence of numbers for identifying the box, usually written on box label " labelIdentifier: String! + " If the client is not authorized to access the location's base, return `null` instead of raising an authorization error. This enables the target side of a shipment to scan boxes that are not yet reconciliated into their stock (but still registered at the source side) " location: Location distributionEvent: DistributionEvent numberOfItems: Int + " If the client is not authorized to access the product's base, return `null` instead of raising an authorization error. This enables the target side of a shipment to scan boxes that are not yet reconciliated into their stock (but still registered at the source side) " product: Product size: Size! state: BoxState! diff --git a/front/src/types/generated/graphql.ts b/front/src/types/generated/graphql.ts index 349848b0e..168ccbf7d 100755 --- a/front/src/types/generated/graphql.ts +++ b/front/src/types/generated/graphql.ts @@ -208,8 +208,10 @@ export type Box = ItemsCollection & { labelIdentifier: Scalars['String']; lastModifiedBy?: Maybe; lastModifiedOn?: Maybe; + /** If the client is not authorized to access the location's base, return `null` instead of raising an authorization error. This enables the target side of a shipment to scan boxes that are not yet reconciliated into their stock (but still registered at the source side) */ location?: Maybe; numberOfItems?: Maybe; + /** If the client is not authorized to access the product's base, return `null` instead of raising an authorization error. This enables the target side of a shipment to scan boxes that are not yet reconciliated into their stock (but still registered at the source side) */ product?: Maybe; qrCode?: Maybe; /** Returns null if box is not part of an active shipment */ From eb2d1703537506013728a8e222bd08701724ab05 Mon Sep 17 00:00:00 2001 From: Philipp Metzner Date: Tue, 1 Oct 2024 17:14:34 +0200 Subject: [PATCH 4/5] Use batch-loading for createdBy and lastModifiedBy fields --- .../business_logic/beneficiary/fields.py | 10 ++++++++++ .../business_logic/warehouse/box/fields.py | 10 ++++++++++ .../business_logic/warehouse/location/fields.py | 10 ++++++++++ .../business_logic/warehouse/product/fields.py | 10 ++++++++++ .../models/definitions/beneficiary.py | 1 + back/boxtribute_server/models/definitions/box.py | 1 + back/boxtribute_server/models/definitions/location.py | 1 + back/boxtribute_server/models/definitions/product.py | 1 + back/test/endpoint_tests/test_box.py | 2 ++ back/test/endpoint_tests/test_classic_location.py | 4 ++++ back/test/endpoint_tests/test_product.py | 4 ++++ 11 files changed, 54 insertions(+) diff --git a/back/boxtribute_server/business_logic/beneficiary/fields.py b/back/boxtribute_server/business_logic/beneficiary/fields.py index 6061e630e..1a98e81a4 100644 --- a/back/boxtribute_server/business_logic/beneficiary/fields.py +++ b/back/boxtribute_server/business_logic/beneficiary/fields.py @@ -80,3 +80,13 @@ def resolve_beneficiary_active(beneficiary_obj, _): def resolve_beneficiary_base(beneficiary_obj, info): authorize(permission="base:read", base_id=beneficiary_obj.base_id) return info.context["base_loader"].load(beneficiary_obj.base_id) + + +@beneficiary.field("createdBy") +def resolve_beneficiary_created_by(beneficiary_obj, info): + return info.context["user_loader"].load(beneficiary_obj.created_by_id) + + +@beneficiary.field("lastModifiedBy") +def resolve_beneficiary_last_modified_by(beneficiary_obj, info): + return info.context["user_loader"].load(beneficiary_obj.last_modified_by_id) diff --git a/back/boxtribute_server/business_logic/warehouse/box/fields.py b/back/boxtribute_server/business_logic/warehouse/box/fields.py index 1ff03ee6a..f9d3dc897 100644 --- a/back/boxtribute_server/business_logic/warehouse/box/fields.py +++ b/back/boxtribute_server/business_logic/warehouse/box/fields.py @@ -67,6 +67,16 @@ def resolve_box_shipment_detail(box_obj, info): return info.context["shipment_detail_for_box_loader"].load(box_obj.id) +@box.field("createdBy") +def resolve_box_created_by(box_obj, info): + return info.context["user_loader"].load(box_obj.created_by_id) + + +@box.field("lastModifiedBy") +def resolve_box_last_modified_by(box_obj, info): + return info.context["user_loader"].load(box_obj.last_modified_by_id) + + @history_entry.field("user") def resolve_history_entry_user(history_entry_obj, info): return info.context["user_loader"].load(history_entry_obj.user_id) diff --git a/back/boxtribute_server/business_logic/warehouse/location/fields.py b/back/boxtribute_server/business_logic/warehouse/location/fields.py index 4aee96b4a..d772ef3f1 100644 --- a/back/boxtribute_server/business_logic/warehouse/location/fields.py +++ b/back/boxtribute_server/business_logic/warehouse/location/fields.py @@ -31,3 +31,13 @@ def resolve_location_boxes(location_obj, _, pagination_input=None, filter_input= def resolve_location_base(location_obj, info): authorize(permission="base:read", base_id=location_obj.base_id) return info.context["base_loader"].load(location_obj.base_id) + + +@classic_location.field("createdBy") +def resolve_location_created_by(location_obj, info): + return info.context["user_loader"].load(location_obj.created_by_id) + + +@classic_location.field("lastModifiedBy") +def resolve_location_last_modified_by(location_obj, info): + return info.context["user_loader"].load(location_obj.last_modified_by_id) diff --git a/back/boxtribute_server/business_logic/warehouse/product/fields.py b/back/boxtribute_server/business_logic/warehouse/product/fields.py index d1f69af8b..630f97752 100644 --- a/back/boxtribute_server/business_logic/warehouse/product/fields.py +++ b/back/boxtribute_server/business_logic/warehouse/product/fields.py @@ -33,3 +33,13 @@ def resolve_product_type(product_obj, _): if product_obj.standard_product_id is None: return ProductType.Custom return ProductType.StandardInstantiation + + +@product.field("createdBy") +def resolve_product_created_by(product_obj, info): + return info.context["user_loader"].load(product_obj.created_by_id) + + +@product.field("lastModifiedBy") +def resolve_product_last_modified_by(product_obj, info): + return info.context["user_loader"].load(product_obj.last_modified_by_id) diff --git a/back/boxtribute_server/models/definitions/beneficiary.py b/back/boxtribute_server/models/definitions/beneficiary.py index c9b810778..16c17dac1 100644 --- a/back/boxtribute_server/models/definitions/beneficiary.py +++ b/back/boxtribute_server/models/definitions/beneficiary.py @@ -61,6 +61,7 @@ class Beneficiary(db.Model): # type: ignore null=True, on_delete="SET NULL", on_update="CASCADE", + object_id_name="last_modified_by_id", ) not_registered = IntegerField( column_name="notregistered", constraints=[SQL("DEFAULT 0")], default=False diff --git a/back/boxtribute_server/models/definitions/box.py b/back/boxtribute_server/models/definitions/box.py index 2bbbf8ef5..b1ebc46b8 100644 --- a/back/boxtribute_server/models/definitions/box.py +++ b/back/boxtribute_server/models/definitions/box.py @@ -68,6 +68,7 @@ class Box(db.Model): # type: ignore null=True, on_delete="SET NULL", on_update="CASCADE", + object_id_name="last_modified_by_id", ) product = UIntForeignKeyField( column_name="product_id", diff --git a/back/boxtribute_server/models/definitions/location.py b/back/boxtribute_server/models/definitions/location.py index 5411078e6..d163a3bd2 100644 --- a/back/boxtribute_server/models/definitions/location.py +++ b/back/boxtribute_server/models/definitions/location.py @@ -53,6 +53,7 @@ class Location(db.Model): # type: ignore null=True, on_delete="SET NULL", on_update="CASCADE", + object_id_name="last_modified_by_id", ) seq = IntegerField(null=True) visible = IntegerField(constraints=[SQL("DEFAULT 1")]) diff --git a/back/boxtribute_server/models/definitions/product.py b/back/boxtribute_server/models/definitions/product.py index bb2f19c79..d9698d129 100644 --- a/back/boxtribute_server/models/definitions/product.py +++ b/back/boxtribute_server/models/definitions/product.py @@ -55,6 +55,7 @@ class Product(db.Model): # type: ignore null=True, on_delete="SET NULL", on_update="CASCADE", + object_id_name="last_modified_by_id", ) name = CharField() size_range = UIntForeignKeyField( diff --git a/back/test/endpoint_tests/test_box.py b/back/test/endpoint_tests/test_box.py index ae78c33a2..d5fe73946 100644 --- a/back/test/endpoint_tests/test_box.py +++ b/back/test/endpoint_tests/test_box.py @@ -34,6 +34,7 @@ def test_box_query_by_label_identifier( state qrCode {{ id }} createdBy {{ id }} + lastModifiedBy {{ id }} deletedOn comment tags {{ @@ -56,6 +57,7 @@ def test_box_query_by_label_identifier( "state": BoxState.InStock.name, "qrCode": {"id": str(default_box["qr_code"])}, "createdBy": {"id": str(default_box["created_by"])}, + "lastModifiedBy": {"id": str(default_box["last_modified_by"])}, "deletedOn": None, "comment": None, "tags": [ diff --git a/back/test/endpoint_tests/test_classic_location.py b/back/test/endpoint_tests/test_classic_location.py index ee8a4e996..3b452b74c 100644 --- a/back/test/endpoint_tests/test_classic_location.py +++ b/back/test/endpoint_tests/test_classic_location.py @@ -30,6 +30,8 @@ def test_location_query( defaultBoxState createdOn createdBy {{ id }} + lastModifiedOn + lastModifiedBy {{ id }} }} }}""" queried_location = assert_successful_request(read_only_client, query) @@ -44,6 +46,8 @@ def test_location_query( "defaultBoxState": BoxState(default_location["box_state"]).name, "createdOn": None, "createdBy": {"id": str(default_location["created_by"])}, + "lastModifiedOn": None, + "lastModifiedBy": None, } query = f"""query {{ location(id: "{distribution_spot['id']}") {{ id }} }}""" diff --git a/back/test/endpoint_tests/test_product.py b/back/test/endpoint_tests/test_product.py index d36e398cf..2793e26e3 100644 --- a/back/test/endpoint_tests/test_product.py +++ b/back/test/endpoint_tests/test_product.py @@ -25,6 +25,8 @@ def test_product_query(read_only_client, default_product, default_size, another_ gender comment createdBy {{ id }} + lastModifiedOn + lastModifiedBy {{ id }} deletedOn }} }}""" @@ -43,6 +45,8 @@ def test_product_query(read_only_client, default_product, default_size, another_ "comment": default_product["comment"], "gender": "Women", "createdBy": {"id": str(default_product["created_by"])}, + "lastModifiedOn": None, + "lastModifiedBy": None, "deletedOn": default_product["deleted_on"], } From 43d1954b3f618067d98889662d9c3f3775662400 Mon Sep 17 00:00:00 2001 From: Philipp Metzner Date: Tue, 1 Oct 2024 17:26:03 +0200 Subject: [PATCH 5/5] Extend test case --- back/test/endpoint_tests/test_permissions.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/back/test/endpoint_tests/test_permissions.py b/back/test/endpoint_tests/test_permissions.py index 92a0baaf4..a22cb45a6 100644 --- a/back/test/endpoint_tests/test_permissions.py +++ b/back/test/endpoint_tests/test_permissions.py @@ -396,6 +396,15 @@ def test_invalid_permission_for_box_field(read_only_client, mocker, default_box, assert_forbidden_request(read_only_client, query, value={field: None}) +def test_invalid_permission_for_box_size(read_only_client, mocker, default_box): + # Test case 8.1.9 + # verify missing size:read permission + mock_user_for_request(mocker, permissions=["stock:read"]) + query = f"""query {{ box(labelIdentifier: "{default_box["label_identifier"]}") + {{ size {{ id }} }} }}""" + assert_forbidden_request(read_only_client, query) + + @pytest.mark.parametrize( "field", ["sourceLocation", "targetLocation", "sourceProduct", "targetProduct"] )