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

Feature/common error conditions order already exists #183

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
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
6 changes: 3 additions & 3 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
"version": "0.2.0",
"configurations": [
{
"name": "integration-tests - cancellation/customer-requested-cancellation/unknown-order-test",
"name": "integration-tests - core/common-error-conditions/order-already-exists",
"request": "launch",
"runtimeArgs": [
"run-script",
"start-tests",
"--",
"--runInBand",
"test/features/cancellation/customer-requested-cancellation/implemented/unknown-order-test.js"
"test/features/core/common-error-conditions/implemented/order-already-exists-test.js"
Copy link
Collaborator

@nickevansuk nickevansuk Sep 15, 2020

Choose a reason for hiding this comment

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

We should fix this #184 next time we change this, so we don't need to update this in each PR

],
"runtimeExecutable": "npm",
"runtimeVersion": "12.18.2",
Expand All @@ -21,7 +21,7 @@
],
"type": "node",
"env": {
"NODE_ENV": "dev"
"NODE_ENV": "remote"
}
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ The tests for these features provide partial coverage but do not include all kno
| Category | Feature | Specification | Description | Prerequisites per Opportunity Type |
|----------|---------|---------------|-------------|-------------------|
| core | Amending the OrderQuote before B ([amending-order-quote](./core/amending-order-quote/README.md)) | [Required](https://www.openactive.io/open-booking-api/EditorsDraft/#amending-the-orderquote-before-b) | Allows the basket to be updated for a particular order | [TestOpportunityBookable](https://openactive.io/test-interface#TestOpportunityBookable) x32 |
| core | Common error conditions ([common-error-conditions](./core/common-error-conditions/README.md)) | [Required](https://openactive.io/open-booking-api/EditorsDraft/#error-model) | Tests C1, C2 and B for common error conditions applicable to all implementations | [TestOpportunityBookable](https://openactive.io/test-interface#TestOpportunityBookable) x21, [TestOpportunityBookablePaid](https://openactive.io/test-interface#TestOpportunityBookablePaid) x3 |
| core | Common error conditions ([common-error-conditions](./core/common-error-conditions/README.md)) | [Required](https://openactive.io/open-booking-api/EditorsDraft/#error-model) | Tests C1, C2 and B for common error conditions applicable to all implementations | [TestOpportunityBookable](https://openactive.io/test-interface#TestOpportunityBookable) x21, [TestOpportunityBookablePaid](https://openactive.io/test-interface#TestOpportunityBookablePaid) x7 |
| core | Simple Booking of free opportunities ([simple-book-free-opportunities](./core/simple-book-free-opportunities/README.md)) | [Required](https://www.openactive.io/open-booking-api/EditorsDraft/#free-opportunities) | The most simple form of booking, for free opportunities. Does not check for leases. | [TestOpportunityBookableFree](https://openactive.io/test-interface#TestOpportunityBookableFree) x8 |
| cancellation | Customer Requested Cancellation ([customer-requested-cancellation](./cancellation/customer-requested-cancellation/README.md)) | [Optional](https://www.openactive.io/open-booking-api/EditorsDraft/#customer-requested-cancellation) | Cancellation triggered by the Customer through the Broker | [TestOpportunityBookableCancellable](https://openactive.io/test-interface#TestOpportunityBookableCancellable) x1 |
| core | Multiple Sellers ([multiple-sellers](./core/multiple-sellers/README.md)) | [Optional](https://openactive.io/open-booking-api/EditorsDraft/#booking-pre-conditions) | The booking system is multi-tenanted and provides services to multiple sellers. | [TestOpportunityBookable](https://openactive.io/test-interface#TestOpportunityBookable) x2 |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Coverage Status: **partial**
### Test prerequisites
Opportunities that match the following criteria must exist in the booking system (for each configured `bookableOpportunityTypesInScope`) for the configured primary Seller in order to use `useRandomOpportunities: true`. Alternatively the following `testOpportunityCriteria` values must be supported by the [test interface](https://openactive.io/test-interface/) of the booking system for `useRandomOpportunities: false`.

[TestOpportunityBookable](https://openactive.io/test-interface#TestOpportunityBookable) x21, [TestOpportunityBookablePaid](https://openactive.io/test-interface#TestOpportunityBookablePaid) x3
[TestOpportunityBookable](https://openactive.io/test-interface#TestOpportunityBookable) x21, [TestOpportunityBookablePaid](https://openactive.io/test-interface#TestOpportunityBookablePaid) x7


### Running tests for only this feature
Expand All @@ -37,6 +37,7 @@ This feature is **required** by the Open Booking API specification, and so must
|------------|------|-------------|---------------|
| [incomplete-broker-details](./implemented/incomplete-broker-details-test.js) | Expect an IncompleteBrokerDetailsError when broker details are missing name | Run each of C1, C2 and B for a valid opportunity, with broker details incomplete (missing name), expecting an IncompleteBrokerDetailsError to be returned | [TestOpportunityBookable](https://openactive.io/test-interface#TestOpportunityBookable) x12 |
| [incomplete-customer-details](./implemented/incomplete-customer-details-test.js) | Expect an IncompleteCustomerDetailsError when customer details are incomplete | Run each of C2 and B for a valid opportunity, with customer details incomplete, expecting an IncompleteCustomerDetailsError to be returned (C1 is ignored because customer details are not accepted for C1) | [TestOpportunityBookable](https://openactive.io/test-interface#TestOpportunityBookable) x8 |
| [order-already-exists](./implemented/order-already-exists-test.js) | Expect an OrderAlreadyExistsError if an Order UUID exists but with different OrderItems | Do a successful C1, C2, B run. Then, run B again for the same Order UUID, but with different OrderItems. Expect an OrderAlreadyExistsError. | [TestOpportunityBookablePaid](https://openactive.io/test-interface#TestOpportunityBookablePaid) x4 |
| [payment-mismatch](./implemented/payment-mismatch-test.js) | Expect a TotalPaymentDueMismatchError when the totalPaymentDue property does not match | Run B for a valid opportunity, with totalPaymentDue not matching the value returned by C2, expecting a TotalPaymentDueMismatchError to be returned (C1 and C2 ignored as they do not have totalPaymentDue) | [TestOpportunityBookablePaid](https://openactive.io/test-interface#TestOpportunityBookablePaid) x3, [TestOpportunityBookable](https://openactive.io/test-interface#TestOpportunityBookable) x1 |


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
const { FeatureHelper } = require('../../../../helpers/feature-helper');
const { GetMatch, C1, C2, B } = require('../../../../shared-behaviours');
const { itShouldReturnAnOpenBookingError } = require('../../../../shared-behaviours/errors');
const { RequestState } = require('../../../../helpers/request-state');
const { FlowHelper } = require('../../../../helpers/flow-helper');
const { generateUuid } = require('../../../../helpers/generate-uuid');

FeatureHelper.describeFeature(module, {
testCategory: 'core',
testFeature: 'common-error-conditions',
testFeatureImplemented: true,
testIdentifier: 'order-already-exists',
testName: 'Expect an OrderAlreadyExistsError if an Order UUID exists but with different OrderItems',
testDescription: 'Do a successful C1, C2, B run. Then, run B again for the same Order UUID, but with different OrderItems. Expect an OrderAlreadyExistsError.',
// The primary opportunity criteria to use for the primary OrderItem under test
testOpportunityCriteria: 'TestOpportunityBookablePaid',
// The secondary opportunity criteria to use for multiple OrderItem tests
controlOpportunityCriteria: 'TestOpportunityBookablePaid',
numOpportunitiesUsedPerCriteria: 1,
},
(configuration, orderItemCriteria, featureIsImplemented, logger) => {
describe('OrderAlreadyExistsError at B', () => {
const uuid = generateUuid();

/**
* @param {Set<import('../../../../helpers/flow-helper').StageIdentifier>} [stagesToSkip]
*/
function getMatchWithNewState(stagesToSkip) {
const state = new RequestState(logger, { uuid });
const flow = new FlowHelper(state, { stagesToSkip });

// Get All Opportunities
beforeAll(async () => {
await state.fetchOpportunities(orderItemCriteria);
});

describe('Get Opportunity Feed Items', () => {
(new GetMatch({
state, flow, logger, orderItemCriteria,
}))
.beforeSetup()
.successChecks()
.validationTests();
});

return { state, flow };
}

describe('First Run', () => {
const { state, flow } = getMatchWithNewState();

describe('C1', () => {
(new C1({
state, flow, logger,
}))
.beforeSetup()
.successChecks()
.validationTests();
});

describe('C2', () => {
(new C2({
state, flow, logger,
}))
.beforeSetup()
.successChecks()
.validationTests();
});

describe('B first time', function () {
(new B({
state, flow, logger,
}))
.beforeSetup()
.successChecks()
.validationTests();
});
});


describe('Second Run', async () => {
const { state, flow } = getMatchWithNewState(new Set(['C1', 'C2']));

// Try B again with same UUID specified in state
describe('B second time', function () {
(new B({
state, flow, logger,
}))
.beforeSetup()
.validationTests();

itShouldReturnAnOpenBookingError('OrderAlreadyExistsError', 500, () => state.bResponse);
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -207,14 +207,33 @@ class RequestState {
return isResponse(this.c1Response);
}

/**
* As leasing is optional, getting the totalPaymentDue should not be dependent on C1 or C2 being completed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although Leasing is optional C1 and C2 are not (ref: https://openactive.io/open-booking-api/EditorsDraft/#broker-contract-with-booking-system)

So totalPaymentDue from the response must always be used. See also openactive/open-booking-api#152

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, in that case the RefImpl needs to be changed as it returns a PaymentMismatchError when it shouldn't

* However if they have been, they should be used as they will be more accurate.
*
* @readonly
* @memberof RequestState
* @returns {number | null}
*/
get totalPaymentDue() {
// Check if C1 or C2 have successfully happened
const response = this.c2Response || this.c1Response;
if (response) {
if (!response.body.totalPaymentDue) return 0;

if (!response) return;
return response.body.totalPaymentDue.price;
}

// If C1 or C2 have not been successfully completed, work out the totalPriceDue from the orderItems
if (this.orderItems) {
const totalPriceDue = this.orderItems
.map(orderItem => orderItem.acceptedOffer.price)
.reduce((accumulator, currentValue) => accumulator + currentValue);

if (!response.body.totalPaymentDue) return;
return totalPriceDue;
}

return response.body.totalPaymentDue.price;
return null;
}

async putOrderQuote() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ const { expect } = require('chakram');
const sharedValidationTests = require('./validation');

class B {
/**
* @param {object} args
* @param {InstanceType<import('../helpers/request-state')['RequestState']>} args.state
* @param {import('../helpers/flow-helper').FlowHelperType} args.flow
* @param {import('../helpers/logger').BaseLoggerType} args.logger
*/
Comment on lines +5 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

yasss

constructor({ state, flow, logger }) {
this.state = state;
this.flow = flow;
Expand Down