feat: B/P Detailed Errors, race condition fixes, and preliminary support for the Customer Account API by nickevansuk · Pull Request #177 · openactive/OpenActive.Server.NET

nickevansuk · 2022-05-27T18:02:27Z

This PR includes:

Very early work to add preliminary CustomerAccount support based on the ongoing conversations around the Customer Accounts proposal, to inform the OpenActive specification work for Customer Accounts with implementation feedback.
B/P detailed errors (Return detailed errors from B, same as C2 open-booking-api#218)
Prevention of race conditions from feat: Fixing issues with race conditions #164

Migration required for users of the library as follows:

Find-and-replace (case sensitive) all instances of SellerIdComponents with SimpleIdComponents
Find-and-replace all instances of SellerIdLong with IdLong
Find-and-replace all instances of SellerIdGuid with IdGuid
Find-and-replace all instances of SellerIdString with IdString
Find-and-replace all instances of (string clientId, Uri sellerId) = User.GetAccessTokenOpenBookingClaims(); with (string clientId, Uri sellerId, _) = User.GetAccessTokenOpenBookingClaims();

…mple, due to any discount relevant to the Customer Account)

…Components`, etc

nickevansuk

A few notes

nickevansuk · 2022-06-01T13:38:10Z

      "applicationUrl": "https://localhost:5001",
      "environmentVariables": {
-        "ASPNETCORE_ENVIRONMENT": "Development",
+        "ASPNETCORE_ENVIRONMENT": "no-auth",


Suspect this shouldn't have been left in?

Have removed that in the next commit

nickevansuk · 2022-06-01T13:39:34Z

+                                orderItem.Status == BookingStatus.Absent ? OrderItemStatus.AttendeeAbsent : (OrderItemStatus?)null,
+                    Attendee = hasAttendeeDetails ? new Person
+                    {
+                        GivenName = orderItem.AttendeeGivenName ?? null,


Not sure ?? null has any affect here? If it's already null, then null will be assigned

nickevansuk · 2022-06-01T13:40:29Z

            long numberOfSpaces,
-            bool proposal
+            bool proposal,
+            List<string> attendeeEmail,


Why is this a set of List<string> and not a List<> of a structured object?

Was following the pattern already set for speed

For example, addLease and addOrder have exactly the same "string instead of structured object" pattern. I suspect this is because there's isn't a API<->DB translation layer (unless I've missed something)

nickevansuk

@civsiv a few more minor fixes here

nickevansuk · 2022-06-06T10:44:36Z

+                        .Where(x => x.RequestOrderItem.Attendee != null)
+                        .Select(x => new FakeDbPerson
+                        {
+                            GivenName = x.RequestOrderItem.Attendee.GivenName,
+                            FamilyName = x.RequestOrderItem.Attendee.FamilyName,
+                            Email = x.RequestOrderItem.Attendee.Email,
+                            Telephone = x.RequestOrderItem.Attendee.Telephone,
+                        })
+                        .ToList(),


@civsiv will this not cause issues with the attendees not being stored against the correct OrderItems if only some attendees are specified.

e.g. at P

1: Bob
2: Null
3: Alex

then B would be

1: Bob
2: Alex
3: Null

So we'd actually want something more like the below:?

Suggested change

.Where(x => x.RequestOrderItem.Attendee != null)

.Select(x => new FakeDbPerson

{

GivenName = x.RequestOrderItem.Attendee.GivenName,

FamilyName = x.RequestOrderItem.Attendee.FamilyName,

Email = x.RequestOrderItem.Attendee.Email,

Telephone = x.RequestOrderItem.Attendee.Telephone,

})

.ToList(),

.Select(x => x.RequestOrderItem.Attendee == null ? null : new FakeDbPerson

{

GivenName = x.RequestOrderItem.Attendee.GivenName,

FamilyName = x.RequestOrderItem.Attendee.FamilyName,

Email = x.RequestOrderItem.Attendee.Email,

Telephone = x.RequestOrderItem.Attendee.Telephone,

})

.ToList(),

Yep that's a good catch. I'll make two additional fixes here too:

Make this change for OrderItemIntakeFormResponse as that has the same logic

Turn this into the Serialize/Deserialize logic used for OrderItemIntakeFormResponse so that FakeDbPerson can be removed.

nickevansuk · 2022-06-06T10:46:02Z

+                        .Where(x => x.RequestOrderItem.Attendee != null)
+                        .Select(x => new FakeDbPerson
+                        {
+                            GivenName = x.RequestOrderItem.Attendee.GivenName,
+                            FamilyName = x.RequestOrderItem.Attendee.FamilyName,
+                            Email = x.RequestOrderItem.Attendee.Email,
+                            Telephone = x.RequestOrderItem.Attendee.Telephone,
+                        })
+                        .ToList(),


Same issue as above?

nickevansuk · 2022-06-06T10:46:41Z

+                        .Where(x => x.RequestOrderItem.Attendee != null)
+                        .Select(x => new FakeDbPerson
+                        {
+                            GivenName = x.RequestOrderItem.Attendee.GivenName,
+                            FamilyName = x.RequestOrderItem.Attendee.FamilyName,
+                            Email = x.RequestOrderItem.Attendee.Email,
+                            Telephone = x.RequestOrderItem.Attendee.Telephone,
+                        })
+                        .ToList(),


Same issue as above?

nickevansuk · 2022-06-06T10:47:20Z

+                        .Where(x => x.RequestOrderItem.Attendee != null)
+                        .Select(x => new FakeDbPerson
+                        {
+                            GivenName = x.RequestOrderItem.Attendee.GivenName,
+                            FamilyName = x.RequestOrderItem.Attendee.FamilyName,
+                            Email = x.RequestOrderItem.Attendee.Email,
+                            Telephone = x.RequestOrderItem.Attendee.Telephone,
+                        })
+                        .ToList(),


Same issue as below?

nickevansuk · 2022-06-06T10:51:41Z

                    MeetingId = thisClass.AttendanceMode != AttendanceMode.Offline ? Faker.Random.String(length: 10, minChar: '0', maxChar: '9') : null,
-                    MeetingPassword = thisClass.AttendanceMode != AttendanceMode.Offline ? Faker.Random.String(length: 10, minChar: '0', maxChar: '9') : null
+                    MeetingPassword = thisClass.AttendanceMode != AttendanceMode.Offline ? Faker.Random.String(length: 10, minChar: '0', maxChar: '9') : null,
+                    AttendeeEmail = attendees.Count > 0 ? attendees[i].Email : null,


Does this check make sense? Could we be out-of-bounds of the array if i was 2 and the attendee count was 1 (due to the Where clause flagged earlier).

Perhaps something more defensive such as:

Suggested change

AttendeeEmail = attendees.Count > 0 ? attendees[i].Email : null,

AttendeeEmail = attendees.Count > i ? attendees[i].Email : null,

Also note any amends to this pattern need to be replicated below also

I'm going to change this to be a serialised string which will remove all of these issues

…ctive/OpenActive.Server.NET into coverage/b-detailed-errors

nickevansuk added 20 commits August 31, 2021 10:01

temp: Debug race conditions

22bd534

Further fixes

10c11e9

Merge branch 'master' into temp/debug-race-conditions

211ab7f

Merge branch 'master' into temp/debug-race-conditions

0ce4876

Update to latest OpenActive.NET

a22ba75

Update OpenActive.NET

2a5f07c

Remove AuthenticatedCustomer

6bbf963

Initial additions to support Customer Accounts

dbe129f

Fix CustomerAccountIdBaseUrl

6962003

Further hacks

cd58220

Refactor to allow price to change during the lease/book flow (for exa…

ca22c7c

…mple, due to any discount relevant to the Customer Account)

Find-and-replace all instances of SellerIdComponents with `SimpleId…

b70a526

…Components`, etc

Revert simpleIdComponents to sellerIdComponents

c2a0194

Fix for missing settings

40a30b1

Fix for AugmentOrderWithTotals

ae46336

Second fix attempt

e1655e5

Fix for IncompleteOrderItemError

e6b2093

Further fix for IncompleteOrderItemError

d8e0170

Fix for OrderRequiresApproval

d6c689a

Remove critical section console logs

9badbea

nickevansuk changed the title ~~feat: P/B Detailed Errors and preliminary support for the Customer Account API~~ feat: B/P Detailed Errors and preliminary support for the Customer Account API May 27, 2022

nickevansuk changed the title ~~feat: B/P Detailed Errors and preliminary support for the Customer Account API~~ feat: B/P Detailed Errors, race condition fixes, and preliminary support for the Customer Account API May 27, 2022

nickevansuk mentioned this pull request May 27, 2022

feat: Fixing issues with race conditions #164

Closed

nickevansuk and others added 5 commits May 27, 2022 19:12

Merge branch 'master' into coverage/b-detailed-errors

787b13c

Return a 409 status code if any OrderItem level errors exist at P

caa5816

Fix orderRequiresApproval

59739c3

feat: fix attendee details being lost between P and B

d52e834

update framework from core

a76f696

nickevansuk commented Jun 1, 2022

View reviewed changes

fix 500 errors and pass object to fake db

b65b3e7

civsiv and others added 4 commits June 1, 2022 18:07

feat: add additional details support

c24ea69

undo launchSettings

38a5a5f

update framework from core

b9e5dc6

Fix framework build to windows-2019

29a241f

nickevansuk commented Jun 6, 2022

View reviewed changes

Comment thread Examples/BookingSystem.AspNetFramework/Web.config Outdated

Revert Examples/BookingSystem.AspNetFramework/Web.config

fec47ac

nickevansuk commented Jun 6, 2022

View reviewed changes

Fix comment

ed99636

nickevansuk commented Jun 6, 2022

View reviewed changes

Comment thread Examples/BookingSystem.AspNetFramework/Stores/SessionStore.cs Outdated

Fix comment

ec66ae1

nickevansuk commented Jun 6, 2022

View reviewed changes

Comment thread Examples/BookingSystem.AspNetCore/Stores/SessionStore.cs Outdated

nickevansuk and others added 4 commits June 6, 2022 12:02

Fix comments

3904b12

review changes

ca9e7f0

Merge branch 'coverage/b-detailed-errors' of https://github.com/opena…

22b8e1d

…ctive/OpenActive.Server.NET into coverage/b-detailed-errors

add core -> framework changes

f4b15b6

nickevansuk merged commit d7bed9d into master Jun 6, 2022

nickevansuk deleted the coverage/b-detailed-errors branch June 6, 2022 13:00

	AttendeeEmail = attendees.Count > 0 ? attendees[i].Email : null,
	AttendeeEmail = attendees.Count > i ? attendees[i].Email : null,

Conversation

nickevansuk commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickevansuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickevansuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nickevansuk commented May 27, 2022 •

edited

Loading