Navigation Menu

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

Conversation

benjamingeer
Copy link

@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 mentioned this pull request Jan 24, 2020
@tobiasschweizer
Copy link
Contributor

@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
Author

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

@tobiasschweizer
Copy link
Contributor

I can do it, is Thursday ok?

@benjamingeer
Copy link
Author

OK thanks!

@@ -2701,6 +2701,44 @@ class ValuesRouteV2E2ESpec extends E2ESpec {
savedTargetIri should ===(linkTargetIri)
}

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, I'll do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

That should remove the comment from the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

That should remove the comment from the value.

Is there already a test for this? ;-)

Copy link
Author

Choose a reason for hiding this comment

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

Will add a test for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

great!

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

That looks paranoid enough to me ;-)

@benjamingeer
Copy link
Author

That looks paranoid enough to me ;-)

That's high praise. :)

@benjamingeer benjamingeer merged commit faa2e55 into develop Feb 7, 2020
@benjamingeer benjamingeer deleted the fix/1574-link-value-comment branch February 7, 2020 16:52
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.

Error updating LinkValue with change of commentary only
3 participants