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 availabilities to the REST API client #33

Merged
merged 11 commits into from
Apr 5, 2024

Conversation

m-timmermann
Copy link
Contributor

@m-timmermann m-timmermann commented Mar 18, 2024

This PR adds the availabilities to the REST API client.

See https://github.com/meplato/store.v3/issues/4041

Beware:

  • the delete and upsert request can have some delay (they are getting queued up)

* add Availabilities
@m-timmermann m-timmermann changed the title Add availabilities Add availabilities to the REST API client Mar 18, 2024
@m-timmermann m-timmermann marked this pull request as ready for review March 19, 2024 08:30
Copy link
Member

@MCwolek MCwolek left a comment

Choose a reason for hiding this comment

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

The service itself so far looks OK. But the c# project Meplato.Store2 doesn't contain the new service yet.
So the build of the service won't include the new service file. You need to modify the Meplato.Store2.csproj file and add this to the existing ItemGroup element which contains the other files:

<Compile Include="Availabilities\Service.cs" />

Also, no new test cases?

*added the new service
Copy link
Member

@MCwolek MCwolek left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽
Tested with small .Net Console App.
✅ Get
✅ Delete
✅ Upsert (OK after method changed to PUT)


var uriTemplate = _service.BaseURL + "/products/{spn}/availabilities";
var response = await _service.Client.Execute(
HttpMethod.Post,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
HttpMethod.Post,
HttpMethod.Put,

@MCwolek
Copy link
Member

MCwolek commented Mar 25, 2024

Just a general FYI: Project and Solution are using very old .Net version 😉 .Net 4.7.2. Which is multiple years old and is not supported anymore.
The current LTS of .Net is .Net 8

* fix - change post to put
* tests created
* WIP
* fix error in test
Copy link
Member

@MCwolek MCwolek left a comment

Choose a reason for hiding this comment

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

In the older .Net or C# projects, you need to add all the files into the projects file 😉

So you need to update the Meplato.Store2.Tests.csproj file, because right now all your new test data files and the Availabilities tests aren't part of the project. And therefore aren't executed.

* added tests to the Meplato.Store2.Tests.csproj
@m-timmermann
Copy link
Contributor Author

Added the testdata and testfiles to the Meplato.Store2.Tests.csproj

Copy link
Member

@MCwolek MCwolek left a comment

Choose a reason for hiding this comment

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

Please check my comments below. After these changes, I was able to run all tests successfully.

src/Meplato.Store2.Tests/Availabilities/DeleteTests.cs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

If you change in all Availabilities test files the class definition to use : Tests.TestCase instead of : TestCase, then you can delete this file here.
I know, you only copy-paste it from the other test cases. But it's only an additional file that does nothing else. So no need for it, right?

src/Meplato.Store2.Tests/Availabilities/UpsertTests.cs Outdated Show resolved Hide resolved
src/Meplato.Store2.Tests/Availabilities/UpsertTests.cs Outdated Show resolved Hide resolved
@@ -0,0 +1,17 @@
HTTP/1.1 200
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
HTTP/1.1 200
HTTP/1.1 404 Not Found

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 currently returns a 200 with the following json Data

{
    "kind": "store#availabilities/getResponse",
    "items": null,
    "Error": null
}

Copy link
Member

Choose a reason for hiding this comment

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

OK, are you sure you want to leave it this way? 🤔 Every other service (Catalogs, Jobs, Products) in the .Net client throws in the Get endpoint an execution when the requested item doesn't exist.
So the Availabilities service behaves differently, which doesn't feel good TBH, it's not consistent.

Comment on lines +14 to +16
"kind": "store#availabilities/getResponse",
"items": null,
"Error": null
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"kind": "store#availabilities/getResponse",
"items": null,
"Error": null
"error": {
"message": "Availability not found"
}

Comment on lines +52 to +56
var response = await service.Get().Spn("no-such-product").Do();
Assert.NotNull(response);
Assert.NotNull(response.Kind);
Assert.AreEqual("store#availabilities/getResponse", response.Kind);
Assert.Null(response.Items);
Copy link
Member

Choose a reason for hiding this comment

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

Test will fail, because an exception is thrown for an unknown product, so you need to change the assertion:

Suggested change
var response = await service.Get().Spn("no-such-product").Do();
Assert.NotNull(response);
Assert.NotNull(response.Kind);
Assert.AreEqual("store#availabilities/getResponse", response.Kind);
Assert.Null(response.Items);
var ex = Assert.ThrowsAsync<ServiceException>(() => service.Get().Spn("no-such-product").Do());
Assert.NotNull(ex);

Copy link
Member

@MCwolek MCwolek left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

Maybe it should be discussed what to do about the Get endpoint in the Availabilities service, because it does behave differently.

@m-timmermann m-timmermann merged commit 113aec9 into main Apr 5, 2024
1 check passed
@m-timmermann m-timmermann deleted the add-availabilities.issue-4041 branch April 5, 2024 12:15
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.

2 participants