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 (admin): Add edit list item route #1447

Closed
wants to merge 34 commits into from
Closed

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Sep 24, 2019

Resolves #1387
Resolves #1528
Resolves #1520

@subotic subotic self-assigned this Sep 24, 2019
@subotic subotic added this to the 2019-10 milestone Sep 26, 2019
@subotic subotic added API/Admin breaking/api enhancement improve existing code or new feature labels Sep 26, 2019
@subotic subotic modified the milestones: 2019-10, 2019-11 Oct 27, 2019
@benjamingeer benjamingeer modified the milestones: 2019-12, 2020-01 Dec 16, 2019
@tobiasschweizer
Copy link
Contributor

if you need my help integrating this PR into knora-api-s-lib, please let me know. Ideally, this would be automated, see #1554

@benjamingeer
Copy link

if you need my help integrating this PR into knora-api-s-lib, please let me know.

@tobiasschweizer Yes please. The semantics of the list update API have changed, so some of the existing client API functions and test data had to be replaced.

@tobiasschweizer
Copy link
Contributor

What has changed exactly and how could this affect existing code?

@benjamingeer
Copy link

What has changed exactly and how could this affect existing code?

@tobiasschweizer The client function updateListInfo has been removed, and the following client functions have been added:

  • updateListName
  • updateListLabels
  • updateListComments
  • updateListNodeName
  • updateListNodeLabels
  • updateListNodeComments

Each of the new functions has new test data.

@tobiasschweizer
Copy link
Contributor

So this means that the test for updateListInfo has to be removed and tests for the new methods have to be added using the new test data?

This may also affect @kilchenmann and @flavens since the admin method are already in use.

@benjamingeer
Copy link

So this means that the test for updateListInfo has to be removed and tests for the new methods have to be added using the new test data?

That's right.

This may also affect @kilchenmann and @flavens since the admin method are already in use.

Yes. I had understood that the existing functionality wouldn't be changed, but it looks like that wasn't feasible.

@LukasStoeckli
Copy link
Contributor

LukasStoeckli commented Jan 14, 2020

Yes. I had understood that the existing functionality wouldn't be changed, but it looks like that wasn't feasible.

What functionality exactly? The update method being split up into one route per attribute?

@flavens
Copy link

flavens commented Jan 14, 2020

@benjamingeer @LukasStoeckli @tobiasschweizer
this is how we use it at the moment:
this.knoraApiConnection.admin.listsEndpoint.updateListInfo(listInfoUpdateData).subscribe( (response: ApiResponseData<ListInfoResponse>) => { ... }
so, as Tobias explained us, we will have to rewrite and split this method with all the new methods to get the same info, which means quite a lot of refactoring for us actually..

@flavens
Copy link

flavens commented Jan 21, 2020

@benjamingeer @LukasStoeckli will this PR be part of the next release?

@benjamingeer
Copy link

@flavens We can certainly wait if you need more time.

@flavens
Copy link

flavens commented Jan 22, 2020

@flavens We can certainly wait if you need more time.

That would be great! We will do it in February to be part of R2020-02.

@benjamingeer benjamingeer modified the milestones: 2020-01, 2020-02 Jan 27, 2020
@subotic subotic modified the milestones: 2020-02, 2020-01 Feb 7, 2020
@flavens flavens added breaking any breaking change and removed breaking/api labels Sep 24, 2020
@subotic subotic changed the base branch from develop to main November 2, 2020 07:44
@SepidehAlassi
Copy link
Contributor

The issues addressed in this PR are implemented in #1753.

@subotic subotic deleted the wip/1387-edit-list-item branch March 9, 2022 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API/Admin breaking any breaking change enhancement improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redundant list routes? Updating the name of a list Edit list item
6 participants