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

DSP-740 Update List Name #1727

Merged
merged 10 commits into from Oct 9, 2020
Merged

DSP-740 Update List Name #1727

merged 10 commits into from Oct 9, 2020

Conversation

SepidehAlassi
Copy link
Contributor

@SepidehAlassi SepidehAlassi commented Oct 6, 2020

resolves DSP-740

also

  • make labels, and comments in payload of update request optional.
  • update documentation
  • add tests
  • add test data

closes #1520 and closes #1528

Note: This does not break JS-lib but Knora-py.

@SepidehAlassi SepidehAlassi self-assigned this Oct 6, 2020
@SepidehAlassi SepidehAlassi added API/Admin enhancement improve existing code or new feature labels Oct 6, 2020
@SepidehAlassi SepidehAlassi reopened this Oct 7, 2020
@SepidehAlassi SepidehAlassi added the breaking any breaking change label Oct 7, 2020
@SepidehAlassi SepidehAlassi marked this pull request as ready for review October 7, 2020 14:48
labels: Seq[StringLiteralV2],
comments: Seq[StringLiteralV2]) extends ListADMJsonProtocol {
name: Option[String] = None,
labels: Option[Seq[StringLiteralV2]] = None,

Choose a reason for hiding this comment

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

If I understand correctly, the reason for the Option is that if you submit an empty list, it should remove all labels. Is that right? If so, could you add a test for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, if you submit labels=[] it will return a BedRequestException. At the time being, the idea is that the update route should not be used to delete the properties. This will change later.

Choose a reason for hiding this comment

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

So why use Option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because without it, the user needs to repeat the labels and comments in the payload even if she wants to only change the name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So when labels and comments are optional, user can simply use the following request body to update only the name of the list without having to give/repeat the values of labels and comments

   {
       "listIri": "listIri",
       "projectIri": "someprojectiri",
       "name": "another name"
  }

Choose a reason for hiding this comment

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

OK, I understand, thanks.

@SepidehAlassi
Copy link
Contributor Author

@benjamingeer I just added your points about making the checks clear. Also, the test data is added.

@subotic
Copy link
Collaborator

subotic commented Oct 9, 2020

@SepidehAlassi could you add the fixed to the docs for PR #1724 in this PR?

Also, just found about it yesterday myself. We actually have developer guidelines regarding the naming of PRs: https://docs.dasch.swiss/developers/dsp/contribution/#pull-request-guidelines

Copy link

@benjamingeer benjamingeer left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@SepidehAlassi SepidehAlassi changed the title Update List Name DSP-740 Update List Name Oct 9, 2020
@SepidehAlassi
Copy link
Contributor Author

@SepidehAlassi could you add the fixed to the docs for PR #1724 in this PR?

Also, just found about it yesterday myself. We actually have developer guidelines regarding the naming of PRs: https://docs.dasch.swiss/developers/dsp/contribution/#pull-request-guidelines

Done! :-)

@SepidehAlassi
Copy link
Contributor Author

Looks good, thank you!

Thank you for reviewing this.

@SepidehAlassi
Copy link
Contributor Author

@benjamingeer the update of expected-client-test-data.txt was missing, I committed it in a4db9d6. I am going to merge this PR after tests pass and make a new release candidate, is that ok for you?

@benjamingeer
Copy link

Yes please!

@SepidehAlassi SepidehAlassi merged commit 6c2e903 into develop Oct 9, 2020
@SepidehAlassi SepidehAlassi deleted the wip/DSP-740-UpdateListName branch October 9, 2020 18:50
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
3 participants