feat: merge Lorem Fitsum features into Reference Implementation feeds #215
Conversation
| var facility = new FacilityUseTable | ||
| { | ||
| Id = seed.Id, | ||
| Deleted = false, | ||
| Name = facilityUseName, | ||
| SellerId = Faker.Random.Bool(0.8f) ? Faker.Random.Long(1, 2) : Faker.Random.Long(3, 5), // distribution: 80% 1-2, 20% 3-5 | ||
| SellerId = Faker.Random.Bool(0.8f) ? Faker.Random.Long(1, 2) : Faker.Random.Long(3, 4), // distribution: 80% 1-2, 20% 3-5 |
There was a problem hiding this comment.
Seller 5 is an individual seller which doesn't make sense for a facility
There was a problem hiding this comment.
Have just spotted a typo, so will fix if this current CI run passes
| @@ -1497,13 +1385,13 @@ private static async Task CreateFakeFacilitiesAndSlots(IDbConnection db, bool fa | |||
| var slotId = 0; | |||
| List<(FacilityUseTable facility, List<SlotTable> slots)> facilitiesAndSlots = opportunitySeeds.Select((seed) => | |||
| { | |||
| var facilityUseName = $"{Faker.Commerce.ProductMaterial()} {Faker.PickRandomParam("Sports Hall", "Swimming Pool Hall", "Running Hall", "Jumping Hall")}"; | |||
| var facilityUseName = $"{Faker.Commerce.ProductMaterial()} {Faker.PickRandomParam("Sports Hall", "Squash Court", "Badminton Court", "Cricket Net")}"; | |||
There was a problem hiding this comment.
These have matching facility types in the Facility Types JSONLD
There was a problem hiding this comment.
Should we use an enum for these, rather than string matching?
There was a problem hiding this comment.
This is facility type name, not the Facility Type
| @@ -644,118 +644,6 @@ public async Task<(bool, ClassTable, OccurrenceTable, BookedOrderItemInfo)> GetO | |||
| } | |||
| } | |||
|
|
|||
| public OpenActive.NET.Place GetPlaceById(long placeId) | |||
There was a problem hiding this comment.
Moved this to FeedGenerationHelper as it doesn't really belong in FakeBookingSystem
| Modified = result.Item1.Modified, | ||
| State = result.Item1.Deleted ? RpdeState.Deleted : RpdeState.Updated, | ||
| Data = result.Item1.Deleted ? null : new FacilityUse | ||
| var faker = new Faker() { Random = new Randomizer(((int)result.Item1.Modified + (int)result.Item1.Id)) }; |
There was a problem hiding this comment.
Apologies I don't have enough context to fully understand this. Two potential issues that I see with the random seed value provided here, though I likely misunderstand things are:
- modified + id. There will be clashes with items. Maybe that's okay or maybe there won't due to how modified and ID are set.
- modified + id. Due to the inclusion of the
modifiedvalue, it looks like all of an item's data will change when modified changes
There was a problem hiding this comment.
Sorry Luke I don't understand the first point. How would modified+id clash? The ids are unique within the type (SessionSeries/FacilityUse etc) as the id is the primary key in the table they are stored in. Clashing across types is not an issue as they don't have shared fields.
Completely agree however the modified can change. I have changed the seed to just be the id which will be unique
There was a problem hiding this comment.
I have changed the seed to just be the id which will be unique
Great, that fixes both points!
| @@ -166,6 +166,9 @@ public static StoreBookingEngine CreateStoreBookingEngine(AppSettings appSetting | |||
| ), | |||
| HasSingleSeller = appSettings.FeatureFlags.SingleSeller, | |||
|
|
|||
| // IdempotencyStore used for storing the response to Order Creation B/P requests | |||
There was a problem hiding this comment.
This is a hangover from a previous PR (#207) where the Core code wasn't copied over to Framework
| @@ -0,0 +1,25 @@ | |||
| using OpenActive.Server.NET.OpenBookingHelper; | |||
There was a problem hiding this comment.
This is a hangover from a previous PR (#207) where the Core code wasn't copied over to Framework
| @@ -6,8 +6,7 @@ | |||
| "profiles": { | |||
| "BookingSystem.AspNetCore": { | |||
| "commandName": "Project", | |||
| "launchBrowser": true, | |||
| "launchUrl": "https://localhost:5001/openactive", | |||
| "launchUrl": "https://localhost:5002/openactive", | |||
There was a problem hiding this comment.
Have just spotted a typo, so will fix if this current CI run passes
| 1. In Visual Studio, run the BookingSystem.AspNetCore project | ||
|
|
||
| When it's finished building, it will open a page in your browser with a randomly assigned port e.g. http://localhost:55603/. Make note of this port. | ||
|
|
||
| Head to `http://localhost:{PORT}/openactive` to check that the project is running correctly. You should see an Open Data landing page. | ||
| 2. Head to BookingSystem.AspNetCore project options and add an env var using the port you made note of earlier: | ||
|
|
||
| `ApplicationHostBaseUrl: http://localhost:{PORT}` | ||
| 3. Now, re-run the project. You're good to go 👍 | ||
|
|
There was a problem hiding this comment.
Is this still true? I think I removed this at some point in a PR because this no longer seems to be true. For example the port assignment. I at some point removed and re-installed the project, run it with dotnet run and it always uses 5001.
The section in CONTRIBUTING.md shows how I run it. I think recommending using dotnet CLI is useful because the IDE in use will always rely on that CLI, and some users will use VSCode, or some users may not use an IDE and just git clone the project and run it.
But I do agree with you that that section focuses on how to run it with Test Suite and thus requires running the IdentityServer. But to run it just as a standalone reference implementation (w/o reference to Test Suite), the IdentityServer part should be ignored
There was a problem hiding this comment.
Yep since launchSettings.json fixed the port, this has no longer been the case. Have fixed
|
|
||
| ## Reference Implementation Data Generation | ||
|
|
||
| Reference Implementation has three main uses that make it very important in the OpenActive ecosystem: |
There was a problem hiding this comment.
I think we need to do either:
- Make it clear that "BookingSystem.AspNetCore" is synonymous with "Reference Implementation", which is not exactly explicit in the top section of this README
- Or just always call the project BookingSystem.AspNetCore in the README
My personal opinion is # 2 because OpenActive may need to broaden the meaning of "Reference Implementation" at a later date (e.g. if there are future reference implementations in other languages)
There was a problem hiding this comment.
Yep makes sense, I've added a comment about that.
| - For data publishers / booking systems: It is used to demonstrate the properties and shape of data and APIs, according to the OpenActive specifications | ||
| - For data users / brokers: It is used as a trial integration where testing can be done with no ramifications | ||
| - For contributors: It is used to ensure the Test Suite tests are correct and passing, for different combinations of Open Booking API features. |
There was a problem hiding this comment.
This is unbelievably useful to finally spell this out — good shout!
| Due to this split of functionality, the sample data in the feeds are created/transformed in both files, depending on whether they are important to booking | ||
| or not. For example, `Price` is important to booking and there is generated in FakeBookingSystem at startup and stored in the in-memory database. However `Terms Of Service` is not | ||
| needed for booking, and therefore is generated at request time. |
| or not. For example, `Price` is important to booking and there is generated in FakeBookingSystem at startup and stored in the in-memory database. However `Terms Of Service` is not | ||
| needed for booking, and therefore is generated at request time. | ||
|
|
||
| ### Lorem Fitsum mode |
There was a problem hiding this comment.
Maybe this section should refer to how to set Lorem Fitsum mode?
| State = result.Item1.Deleted ? RpdeState.Deleted : RpdeState.Updated, | ||
| Data = result.Item1.Deleted ? null : new FacilityUse | ||
| var faker = new Faker() { Random = new Randomizer((int)result.Item1.Id) }; | ||
| var isGoldenRecord = faker.Random.Bool(); |
There was a problem hiding this comment.
So does this mean that half of records (in Lorem Fitsum mode) will be golden? I assumed we'd want a lower rate
There was a problem hiding this comment.
Yeh fair, changed to 75%
| var faker = new Faker() { Random = new Randomizer((int)result.Item1.Id) }; | ||
| // here we randomly decide whether the item is going to be a golden record or not by using Faker | ||
| // See the README for more detail on golden records. | ||
| var isGoldenRecord = faker.Random.Bool(); |
There was a problem hiding this comment.
same comment about isGoldenRecord
This closes #212
It is a port of https://github.com/openactive/lorem-fitsum with a few minor changes outlined below: