Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/main/java/org/zendesk/client/v2/Zendesk.java
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,16 @@ public Iterable<Article> getArticleFromSearch(String searchTerm, Long sectionId)
.set("query", searchTerm).set("section", sectionId), handleList(Article.class, "results"));
}

public Iterable<Article> getArticleFromSomeLabels(List<String> labels) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming seems a little off to me, especially with the fact singular is used for returning a collection. How about something like..

getArticlesWithAllLabels
searchArticlesWithLabels

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand what is meant by "get" vs "search" if anything. The part I was looking to make clear was that one method requires at least one label to be found, the other requires all labels to be found. IMO, "WithAll" is good, but the idea of "with at least one" is not captured in the name "searchArticlesWithLabels". Not sure if "search" implies such behavior. You objected to the use of singular for returning a collection, I agree with that, I followed the existing pattern established by the getArticleFromSearch method. I think getArticlesFromSomeLabels and getArticlesFromAllLabels best captures the concerns, though I don't like the apparent API inconsistency with the singular Article vs Articles.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about getArticlesFromAllLabels and getArticlesFromAnyLabels

Copy link
Author

Choose a reason for hiding this comment

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

Works for me, as long as everyone doesn't mind the inconsistency of Article plurality with getArticleFromSearch. Should I make the change, or will someone else do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or introduce corrected method with deprecated annotation on the singular.

Copy link
Author

Choose a reason for hiding this comment

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

Who is the "gatekeeper" of the public-facing API? I'm trying to add functionality but not dictate API design in general. I feel like someone more qualified should handle the naming issue. I'm happy with getArticlesFromAllLabels and getArticlesFromAnyLabels. I'm going to make that change and hope it goes through.

return new PagedIterable<>(tmpl("/help_center/articles/search.json{?label_names}").set("label_names", labels),
handleList(Article.class, "results"));
}

public Iterable<Article> getArticleFromAllLabels(List<String> labels) {
return new PagedIterable<>(tmpl("/help_center/en-us/articles.json{?label_names}").set("label_names", labels),
handleList(Article.class, "articles"));
}

public List<ArticleAttachments> getAttachmentsFromArticle(Long articleID) {
return complete(submit(req("GET", tmpl("/help_center/articles/{id}/attachments.json").set("id", articleID)),
handleArticleAttachmentsList("article_attachments")));
Expand Down
44 changes: 44 additions & 0 deletions src/test/java/org/zendesk/client/v2/RealSmokeTest.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.zendesk.client.v2;

import org.hamcrest.core.IsCollectionContaining;
import org.junit.After;
import org.junit.BeforeClass;
import org.junit.Ignore;
Expand Down Expand Up @@ -38,8 +39,10 @@
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Properties;
import java.util.Set;
import java.util.UUID;

import static org.hamcrest.CoreMatchers.anyOf;
Expand Down Expand Up @@ -824,6 +827,47 @@ public void getArticles() throws Exception {
}
}

@Test
public void getArticleFromSomeLabels() throws Exception {
createClientWithTokenOrPassword();
/*
Given 3 articles
Article 1 with title "SomeLabelOne" and label "SomeLabelA"
Article 2 with title "SomeLabelTwo" and labels "SomeLabelB" and "SomeLabelC"
Article 3 with title "SomeLabelThree" and label "SomeLabelD"
When a search by labels "SomeLabelA", "SomeLabelB"
Then we get Article 1 and Article 2 but not Article 3
because Article 1 and 2 have at least one of the labels, Article 3 has none
*/
Iterable<Article> result = instance.getArticleFromSomeLabels(Arrays.asList("SomeLabelA", "SomeLabelB"));
Set<String> actualTitles = extractTitles(result);
assertThat(actualTitles.size(), is(2));
assertThat(actualTitles, IsCollectionContaining.hasItems("SomeLabelOne", "SomeLabelTwo"));
}

@Test
public void getArticleFromAllLabels() throws Exception {
createClientWithTokenOrPassword();
/*
Given 2 articles
Article 1 with title "AllLabelOne" and label "AllLabelA"
Article 2 with title "AllLabelTwo" and labels "AllLabelA" and "AllLabelB"
When a search by labels "AllLabelA", "AllLabelB"
Then we get Article 2 but not Article 1
because Article 2 has both labels and Article 1 has only one
*/
Iterable<Article> result = instance.getArticleFromAllLabels(Arrays.asList("AllLabelA", "AllLabelB"));
Set<String> actualTitles = extractTitles(result);
assertThat(actualTitles.size(), is(1));
assertThat(actualTitles, IsCollectionContaining.hasItems("AllLabelOne"));
}

private Set<String> extractTitles(Iterable<Article> iter) {
Set<String> result = new HashSet<>();
iter.forEach(article -> result.add(article.getTitle()));
return result;
}

@Test
public void getArticleSubscriptions() throws Exception {
createClientWithTokenOrPassword();
Expand Down