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

Add goods on licences endpoint #2307

Merged
merged 5 commits into from
Nov 29, 2024

Conversation

hnryjmes
Copy link
Contributor

@hnryjmes hnryjmes commented Nov 25, 2024

@hnryjmes hnryjmes force-pushed the LTD-5674_Productionise_goods_on_licences_endpoint branch 5 times, most recently from 1553cc8 to 585ef1d Compare November 28, 2024 17:18
Comment on lines -65 to -77
@pytest.fixture(autouse=True)
def mock_s3():
with mock_aws():
s3 = init_s3_client()
s3.create_bucket(
Bucket=settings.AWS_STORAGE_BUCKET_NAME,
CreateBucketConfiguration={
"LocationConstraint": settings.AWS_REGION,
},
)
yield


Copy link
Contributor Author

@hnryjmes hnryjmes Nov 28, 2024

Choose a reason for hiding this comment

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

this was already moved into conftest.py which is why it doesn't need to be here https://github.com/uktrade/lite-api/blob/dev/api/data_workspace/v2/tests/bdd/conftest.py#L67-L77

Comment on lines +396 to +398
# create final advice for controlled goods; skip NLR goods
if good_on_app.is_good_controlled == False:
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a new thing I added, otherwise it's the same fixture just moved to conftest.py

@hnryjmes hnryjmes force-pushed the LTD-5674_Productionise_goods_on_licences_endpoint branch from 451202d to 807d0a0 Compare November 28, 2024 19:44
class GoodOnLicenceViewSet(BaseViewSet):
serializer_class = GoodOnLicenceSerializer
queryset = GoodOnLicence.objects.exclude(
licence__case__status__status=CaseStatusEnum.DRAFT,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in practice it would be pretty unusual to have a licence with a GoodOnLicence object and a case status DRAFT, so I considered removing this filtering licence__case__status__status=CaseStatusEnum.DRAFT, but ultimately I didn't want to change the queryset from the prototype which has been validated so it stayed in.

@hnryjmes hnryjmes marked this pull request as ready for review November 28, 2024 20:02
Copy link
Contributor

@code-review-doctor code-review-doctor bot left a comment

Choose a reason for hiding this comment

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

Looks good. Worth considering though. View full project report here.

data[f"quantity-{good_on_app.id}"] = str(good_on_app.quantity)
data[f"value-{good_on_app.id}"] = str(good_on_app.value)
# create final advice for controlled goods; skip NLR goods
if good_on_app.is_good_controlled == False:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if good_on_app.is_good_controlled == False:
if good_on_app.is_good_controlled is False:

False is a singleton data type, so should be compared using is. More info.

Comment on lines +43 to +44
@given(parsers.parse("the goods are assessed by TAU as:{assessments}"))
def the_goods_are_assessed_by_tau_as(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is there in test_goods_descriptions.py and can be moved to conftest.py

@hnryjmes hnryjmes merged commit 63c3b5a into dev Nov 29, 2024
26 checks passed
@hnryjmes hnryjmes deleted the LTD-5674_Productionise_goods_on_licences_endpoint branch November 29, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants