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

feat(clientapi): Support lists admin endpoint #1509

Merged
merged 32 commits into from Dec 17, 2019

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented Nov 8, 2019

Resolves #1507.

@benjamingeer benjamingeer added API/Admin clientapi frontend: Salsah, DSP-APP, BEOL, MLS, etc. labels Nov 8, 2019
@benjamingeer benjamingeer added this to the 2019-11 milestone Nov 8, 2019
@benjamingeer benjamingeer self-assigned this Nov 8, 2019
@benjamingeer benjamingeer mentioned this pull request Nov 8, 2019
@subotic subotic modified the milestones: 2019-11, 2019-12 Nov 27, 2019
@tobiasschweizer
Copy link
Contributor

reproducible with dasch-swiss/dsp-js-lib@5be84f7

@tobiasschweizer
Copy link
Contributor

Shall I try with 948ffed?

@benjamingeer
Copy link
Author

Sorry, I think that was a copy-and-paste error. I believe I've fixed it; I tried importing the generated code into your knora-api-js-lib branch, and the tests passed.

@tobiasschweizer
Copy link
Contributor

ok, will try it now!

@tobiasschweizer
Copy link
Contributor

Ok, the tests pass now.

But shouldn't ListNodeInfo have a member id?

@benjamingeer
Copy link
Author

But shouldn't ListNodeInfo have a member id?

It got moved into StoredListNodeInfo. But I'm not sure we need that class. I'm thinking about it...

@tobiasschweizer
Copy link
Contributor

Is there JSON test data for CreateChildNodeRequest?

@tobiasschweizer
Copy link
Contributor

It got moved into StoredListNodeInfo. But I'm not sure we need that class. I'm thinking about it...

I just think if there is a method getList(iri: string) the IRI should be present on the response so the client knows it got the correct list.

@benjamingeer
Copy link
Author

the IRI should be present on the response so the client knows it got the correct list.

Yes, I believe it's in the response. The question is how to structure the TypeScript classes. Do we need a class hierarchy like ListNodeInfo -> StoredListNodeInfo -> ReadListNodeInfo (in which case the TypeScript function would return a ReadListNodeInfo)? Or should there just be one class, ListNodeInfo? It depends on how the list update functionality (#1447) is going to work. I guess for now it's probably best to keep it simple and just have one class.

Is there JSON test data for CreateChildNodeRequest?

It's in the e2e tests, I'll add it.

@tobiasschweizer
Copy link
Contributor

It's in the e2e tests, I'll add it.

Great, thanks!

I guess for now it's probably best to keep it simple and just have one class.

Sounds reasonable. This would mean that ListNodeInfo would get a member id or listIri and just be used for reading?

@benjamingeer
Copy link
Author

This would mean that ListNodeInfo would get a member id or listIri and just be used for reading?

Yes, id.

I've just noticed that the generated test requests for updating lists use a mixture of anything and images. I'll sort that out after lunch.

@tobiasschweizer
Copy link
Contributor

Ok, just let me know when it's ready. I have already written tests for all methods but createChildNode and they all pass, see dasch-swiss/dsp-js-lib@5c35212

- Generate just one ListNodeInfo class.
@benjamingeer
Copy link
Author

OK I think this is correct, see if it looks OK to you.

@tobiasschweizer
Copy link
Contributor

I could write all the tests and they pass, so I think this looks good.

Do you need to wait for #1520, #1528, and #1447 for this PR to be merged?

@benjamingeer
Copy link
Author

Do you need to wait for #1520, #1528, and #1447 for this PR to be merged?

No, actually, since I found out at the last dev meeting that the existing functionality should still work after #1447, and the other issues will be resolved later, I think we can merge this now.

@tobiasschweizer
Copy link
Contributor

I think if dasch-swiss/dsp-js-lib#128 works for @lrosenth, this PR is good to go.

@benjamingeer benjamingeer modified the milestones: 2019-12, 2020-01 Dec 16, 2019
@benjamingeer
Copy link
Author

@tobiasschweizer Since you've merged dasch-swiss/dsp-js-lib#128, does that mean I can merge this PR?

@tobiasschweizer
Copy link
Contributor

I still have a question that might be relevant for this PR: dasch-swiss/dsp-js-lib#129 (comment)

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Choose a reason for hiding this comment

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

Ok, thanks for this PR. We will figure out the details for caches later.

@daschbot
Copy link
Collaborator

This pull request has been mentioned on Discuss DaSCH. There might be relevant details there:

https://discuss.dasch.swiss/t/knora-v12-0-0-released/139/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API/Admin clientapi frontend: Salsah, DSP-APP, BEOL, MLS, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(clientapi): Support lists admin endpoint
4 participants