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

fix(api-v2): Change link value comment #1582

Merged
merged 6 commits into from Feb 7, 2020
Merged

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Jan 24, 2020

This PR fixes the case where the client changes only the comment on a LinkValue.

Fixes #1574.

@benjamingeer benjamingeer changed the title fix(api-v2): Change link value comment. fix(api-v2): Change link value comment Jan 24, 2020
@benjamingeer benjamingeer self-assigned this Jan 24, 2020
@benjamingeer benjamingeer added this to the 2020-02 milestone Jan 24, 2020
@benjamingeer benjamingeer requested a review from lrosenth Jan 24, 2020
@benjamingeer benjamingeer mentioned this pull request Jan 24, 2020
@benjamingeer benjamingeer requested a review from tobiasschweizer Jan 28, 2020
@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Jan 30, 2020

@benjamingeer I suggest that @lrosenth first try this PR. I am happy to have a look at the implementation details in knora-api after his approval.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 4, 2020

@tobiasschweizer It looks like @lrosenth doesn't have time to review this. Should I ask someone else?

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 4, 2020

I can do it, is Thursday ok?

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 4, 2020

OK thanks!

benjamingeer and others added 2 commits Feb 5, 2020
@@ -2701,6 +2701,44 @@ class ValuesRouteV2E2ESpec extends E2ESpec {
savedTargetIri should ===(linkTargetIri)
}

"update a link between two resources, changing only the comment" in {

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Feb 6, 2020
Contributor

The link value that gets updated here was created in

https://github.com/dasch-swiss/knora-api/blob/b823a3f2d38552654c82236c636b7340b3a26325/webapi/src/test/scala/org/knora/webapi/e2e/v2/ValuesRouteV2E2ESpec.scala#L1697

without a comment. Then a comment gets added in "update a link between two resources, changing only the comment". Shouldn't we ideally have test cases for

  • create a value with an initial comment that then gets changed
  • create a value without a comment that is added

This comment has been minimized.

@benjamingeer

benjamingeer Feb 6, 2020
Author Collaborator

Good idea, I'll do that.

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Feb 6, 2020
Contributor

Ideally, you could also do that for webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala.

Maybe it would not hurt to do it for a non link value too. I guess these have a common template so testing one non linking value would be ok to begin with, right?

This comment has been minimized.

@benjamingeer

benjamingeer Feb 6, 2020
Author Collaborator

Yes, it's only link values that are handled by separate templates.

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Feb 6, 2020
Contributor

Great!

Is this understanding correct?

To create a new version of a value either the value itself has to be different (regardless of its comment being different from the previous version) or just its comment (this includes submitting a comment for a value that did not have any comment before).

This comment has been minimized.

@benjamingeer

benjamingeer Feb 6, 2020
Author Collaborator

That should remove the comment from the value.

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Feb 6, 2020
Contributor

That should remove the comment from the value.

Is there already a test for this? ;-)

This comment has been minimized.

@benjamingeer

benjamingeer Feb 6, 2020
Author Collaborator

Will add a test for that.

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Feb 6, 2020
Contributor

great!

This comment has been minimized.

@benjamingeer

benjamingeer Feb 7, 2020
Author Collaborator

I added a lot of tests, see if this looks OK to you.

@subotic subotic modified the milestones: 2020-02, 2020-01 Feb 7, 2020
Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

That looks paranoid enough to me ;-)

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 7, 2020

That looks paranoid enough to me ;-)

That's high praise. :)

@benjamingeer benjamingeer merged commit faa2e55 into develop Feb 7, 2020
7 checks passed
7 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Unit Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
@benjamingeer benjamingeer deleted the fix/1574-link-value-comment branch Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

3 participants