Skip to content

Implement custom format template interpolation for refbox citation display #1256

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.

Sign up for GitHub

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

Draft
Copilot wants to merge 3 commits into
base: clarin-v7
Choose a base branch
from
Draft

Implement custom format template interpolation for refbox citation display #1256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6565479
Initial plan
Copilot Aug 8, 2025
e385bb2
Fix refbox format interpolation to include dc.publisher field
Copilot Aug 8, 2025
8a7b732
Implement custom format template interpolation for refbox display text
Copilot Aug 8, 2025
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
77 changes: 69 additions & 8 deletions dspace-server-webapp/src/main/java/org/dspace/app/rest/ClarinRefBoxController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,14 +349,61 @@ public ResponseEntity<RefBoxDTO> getRefboxInfo(
* Build the display text for the RefBox based on the Item Metadata.
*/
private String buildDisplayText(Context context, Item item) {
// 1. Authors
// Check for custom format template
String formatTemplate = itemService.getMetadataFirstValue(item, "local", "refbox", "format", DEFAULT_LANGUAGE);

Copilot AI Aug 21, 2025

Copy link

Choose a reason for hiding this comment

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

The metadata field name should follow DSpace metadata naming conventions. Consider using a more standard namespace like dc.format.template or documenting why local.refbox.format is the preferred choice in the code comments.

Copilot uses AI. Check for mistakes.

if (formatTemplate != null && !formatTemplate.isEmpty()) {
return buildDisplayTextFromTemplate(context, item, formatTemplate);
} else {
return buildDisplayTextDefault(context, item);
}
}

/**
* Build display text using a custom format template with variable interpolation.
* Supported variables: {title}, {authors}, {pid}, {repository}, {year}, {publisher}
*/
private String buildDisplayTextFromTemplate(Context context, Item item, String template) {

@kuchtiak-ufal kuchtiak-ufal Aug 21, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The context parameter doesn't seem to be used in the method body.

Similarly - in all buildDisplayText* methods:
buldDisplayText
buildDisplayTextDefault

// Extract all metadata values
List<String> authors = itemService.getMetadata(item, "dc", "contributor", "author", DEFAULT_LANGUAGE)
.stream().map(MetadataValue::getValue).collect(Collectors.toList());
// If there are no authors, try to get the publisher metadata
if (authors.isEmpty()) {
authors = itemService.getMetadata(item, "dc", "publisher", null, DEFAULT_LANGUAGE)
.stream().map(MetadataValue::getValue).collect(Collectors.toList());
String authorText = formatAuthors(authors);

String year = "";
String issued = itemService.getMetadataFirstValue(item, "dc", "date", "issued", DEFAULT_LANGUAGE);
if (issued != null && !issued.isEmpty()) {
year = issued.split("-")[0];
}

String title = itemService.getMetadataFirstValue(item, "dc", "title", null, DEFAULT_LANGUAGE);
String publisher = itemService.getMetadataFirstValue(item, "dc", "publisher", null, DEFAULT_LANGUAGE);
String repository = configurationService.getProperty("dspace.name");

// Get identifier (prefer DOI, fallback to URI)
String pid = itemService.getMetadataFirstValue(item, "dc", "identifier", "doi", DEFAULT_LANGUAGE);
if (pid == null) {
pid = itemService.getMetadataFirstValue(item, "dc", "identifier", "uri", DEFAULT_LANGUAGE);
}

// Perform template interpolation
String result = template;
result = result.replace("{title}", title != null ? title : "");
result = result.replace("{authors}", authorText != null ? authorText : "");

@kuchtiak-ufal kuchtiak-ufal Aug 21, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think, some of the fields like authorText, year, cannot be null (so check for null isn't needed)

result = result.replace("{pid}", pid != null ? pid : "");

@kuchtiak-ufal kuchtiak-ufal Aug 21, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The {pid} should be (likely) replaced with :

<a href="{pid}">{pid}</a>

(in order to render handle as html link)

result = result.replace("{repository}", repository != null ? repository : "");
result = result.replace("{year}", year != null ? year : "");
result = result.replace("{publisher}", publisher != null ? publisher : "");

return result;

Copilot AI Aug 21, 2025

Copy link

Choose a reason for hiding this comment

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

Multiple string replacements on the same string create intermediate String objects. Consider using StringBuilder or a more efficient template engine for better performance, especially if this method is called frequently.

Suggested change
return result;
// Perform template interpolation efficiently
Map<String, String> variables = new HashMap<>();
variables.put("title", title != null ? title : "");
variables.put("authors", authorText != null ? authorText : "");
variables.put("pid", pid != null ? pid : "");
variables.put("repository", repository != null ? repository : "");
variables.put("year", year != null ? year : "");
variables.put("publisher", publisher != null ? publisher : "");
StringBuilder result = new StringBuilder(template);
for (Map.Entry<String, String> entry : variables.entrySet()) {
String placeholder = "{" + entry.getKey() + "}";
int idx;
// Replace all occurrences of the placeholder
while ((idx = result.indexOf(placeholder)) != -1) {
result.replace(idx, idx + placeholder.length(), entry.getValue());
}
}
return result.toString();

Copilot uses AI. Check for mistakes.
}

/**
* Build display text using the default hardcoded format.
*/
private String buildDisplayTextDefault(Context context, Item item) {

@kuchtiak-ufal kuchtiak-ufal Aug 21, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would be more consistent if buildDisplayTextDefault(Item item) delegates to
buildDisplayTextFromTemplate(Item item, String template), based on the properties, in the way that

StringBuilder template = new StringBuilder();
if (authors.isEmpty()) {
    template.append("{author}, ");
}
if (!year.isEmpty()) {
    template.append("{year}, ");
}
if (!title.isEmpty()) {
    template.append("<i>{title</i>");
}
....
// and finally call
buildDisplayTextFromTemplate(Item item, template.toString());

// 1. Authors
List<String> authors = itemService.getMetadata(item, "dc", "contributor", "author", DEFAULT_LANGUAGE)
.stream().map(MetadataValue::getValue).collect(Collectors.toList());
String authorText = formatAuthors(authors);

// 2. Year
Expand All @@ -370,16 +417,26 @@ private String buildDisplayText(Context context, Item item) {
// 3. Title
String title = itemService.getMetadataFirstValue(item, "dc", "title", null, DEFAULT_LANGUAGE);

// 4. Repository name
// 4. Publisher
String publisher = itemService.getMetadataFirstValue(item, "dc", "publisher", null, DEFAULT_LANGUAGE);

// 5. Repository name
String repository = configurationService.getProperty("dspace.name");

// 5. Identifier URI (prefer DOI)
// 6. Identifier URI (prefer DOI)
String identifier = itemService.getMetadataFirstValue(item, "dc", "identifier", "doi", DEFAULT_LANGUAGE);
if (identifier == null) {
identifier = itemService.getMetadataFirstValue(item, "dc", "identifier", "uri", DEFAULT_LANGUAGE);
}

// 6. Format
// 7. Format as: {authors}, {year}, {title}, {publisher}, {repository}, {identifier}
// If no authors, use publisher as author fallback for backwards compatibility
if (authorText == null || authorText.isEmpty()) {

@kuchtiak-ufal kuchtiak-ufal Aug 21, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

useless check for null for authorText - can be ommited

if (publisher != null && !publisher.isEmpty()) {
authorText = publisher;
}
}

Copilot AI Aug 21, 2025

Copy link

Choose a reason for hiding this comment

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

This fallback logic is duplicated from the original implementation but creates confusion since publisher is now treated as author. Consider extracting this logic into a separate method with a clear name like getAuthorTextWithPublisherFallback() to make the intention explicit.

Copilot uses AI. Check for mistakes.

// Using html tags to format the output because this display text will be rendered in the UI
StringBuilder sb = new StringBuilder();
if (authorText != null && !authorText.isEmpty()) {

@kuchtiak-ufal kuchtiak-ufal Aug 21, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The same as above: check for null is not needed

Expand All @@ -392,6 +449,10 @@ private String buildDisplayText(Context context, Item item) {
sb.append(year);
}
sb.append(", \n <i>").append(title != null ? title : "").append("</i>");
if (publisher != null && !publisher.isEmpty() &&
(authorText == null || authorText.isEmpty() || !authorText.equals(publisher))) {

@kuchtiak-ufal kuchtiak-ufal Aug 21, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

check for null not needed for authorText

sb.append(", ").append(publisher);
}
if (repository != null && !repository.isEmpty()) {
sb.append(", ").append(repository);
}
Expand Down
78 changes: 78 additions & 0 deletions dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinRefBoxControllerIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,84 @@ public void testFeaturedServiceWithMalformedLink() throws Exception {
.andExpect(jsonPath("$.featuredServices.featuredService").isEmpty());
}

@Test
public void testRefboxInfoWithBothAuthorAndPublisher() throws Exception {
context.turnOffAuthorisationSystem();
Item itemWithBoth = ItemBuilder.createItem(context, collection)
.withTitle("Test Item")
.withAuthor("Test Author")
.withMetadata("dc", "publisher", null, "Test Publisher")
.withIssueDate("2023-01-01")
.build();
context.restoreAuthSystemState();

String token = getAuthToken(admin.getEmail(), password);
getClient(token).perform(get("/api/core/refbox?handle=" + itemWithBoth.getHandle()))
.andExpect(status().isOk())
.andExpect(jsonPath("$.displayText").value(org.hamcrest.Matchers.containsString("Test Author")))

@kuchtiak-ufal kuchtiak-ufal Aug 21, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The org.hamcrest.Matchers.containsString can be added to static imports

The containsString method is also used in the test methods below.

.andExpect(jsonPath("$.displayText").value(org.hamcrest.Matchers.containsString("Test Publisher")));
}

@Test
public void testRefboxInfoWithCustomFormatTemplate() throws Exception {
context.turnOffAuthorisationSystem();
Item itemWithTemplate = ItemBuilder.createItem(context, collection)
.withTitle("Custom Title")
.withAuthor("Custom Author")
.withMetadata("dc", "publisher", null, "Custom Publisher")
.withMetadata("local", "refbox", "format",
"{authors} ({year}). {title}. {publisher}. {repository}. {pid}")

Copilot AI Aug 21, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] The test uses a hardcoded format string that spans multiple lines. Consider extracting this to a constant or using a more readable multi-line string format to improve test maintainability.

Suggested change
"{authors} ({year}). {title}. {publisher}. {repository}. {pid}")
CUSTOM_FORMAT_TEMPLATE)

Copilot uses AI. Check for mistakes.
.withIssueDate("2024-01-01")
.build();
context.restoreAuthSystemState();

String token = getAuthToken(admin.getEmail(), password);
getClient(token).perform(get("/api/core/refbox?handle=" + itemWithTemplate.getHandle()))

@kuchtiak-ufal kuchtiak-ufal Aug 21, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can also use MockHttpServletRequestBuilder#param method here

getClient(token).perform(
          get("/api/core/refbox")
              .param("handle", itemWithTemplate.getHandle())
      )
      .andExpect(....
      ...

.andExpect(status().isOk())
.andExpect(jsonPath("$.displayText").value(org.hamcrest.Matchers
.containsString("Custom Author (2024). Custom Title. Custom Publisher.")));
}

@Test
public void testRefboxInfoWithCustomFormatTemplatePartialVariables() throws Exception {
context.turnOffAuthorisationSystem();
Item itemWithPartialTemplate = ItemBuilder.createItem(context, collection)
.withTitle("Partial Title")
.withAuthor("Partial Author")
.withMetadata("local", "refbox", "format", "{authors}: {title} [{year}]")
.withIssueDate("2023-12-31")
.build();
context.restoreAuthSystemState();

String token = getAuthToken(admin.getEmail(), password);
getClient(token).perform(get("/api/core/refbox?handle=" + itemWithPartialTemplate.getHandle()))
.andExpect(status().isOk())
.andExpect(jsonPath("$.displayText").value(org.hamcrest.Matchers
.containsString("Partial Author: Partial Title [2023]")));
}

@Test
public void testRefboxInfoWithoutCustomFormatUsesDefault() throws Exception {
context.turnOffAuthorisationSystem();
Item itemWithoutTemplate = ItemBuilder.createItem(context, collection)
.withTitle("Default Title")
.withAuthor("Default Author")
.withMetadata("dc", "publisher", null, "Default Publisher")
.withIssueDate("2024-02-01")
.build();
context.restoreAuthSystemState();

String token = getAuthToken(admin.getEmail(), password);
getClient(token).perform(get("/api/core/refbox?handle=" + itemWithoutTemplate.getHandle()))
.andExpect(status().isOk())
.andExpect(jsonPath("$.displayText").value(org.hamcrest.Matchers
.containsString("Default Author")))
.andExpect(jsonPath("$.displayText").value(org.hamcrest.Matchers
.containsString("Default Publisher")))
.andExpect(jsonPath("$.displayText").value(org.hamcrest.Matchers
.containsString("<i>Default Title</i>")));
}

@Test
public void testDisplayTextWithOneAuthor() throws Exception {
context.turnOffAuthorisationSystem();
Expand Down