Skip to content

blocked deposit page, hide host field when only one collection (e.g. root) #11301

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

Merged
ofahimIQSS merged 10 commits into from
Jun 16, 2025
Merged

blocked deposit page, hide host field when only one collection (e.g. root) #11301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0ba0814
fix blocking page
jo-pol Mar 4, 2025
e2a6757
using zero as default search argument
jo-pol Mar 6, 2025
56177ba
hide host dataverse when none available to choose
jo-pol Mar 17, 2025
95860a6
Merge remote-tracking branch 'iqss/develop' into blocked-deposit-page
jo-pol Mar 25, 2025
1ebe7f2
release note
jo-pol Apr 3, 2025
34cef44
zero is not always root
jo-pol Apr 14, 2025
e587510
change request and simplified test
jo-pol Jun 2, 2025
ed92750
changes release note according to applied change request
jo-pol Jun 3, 2025
69b854d
keep initial dataverse on invalid input
jo-pol Jun 5, 2025
a9ca7bb
release not change according to code
jo-pol Jun 5, 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
5 changes: 5 additions & 0 deletions doc/release-notes/11301-blocked-deposit-page.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
### Reduced chance of losing metadata on Edit Dataset Metadata page

The remedy for the problem consists of two parts:
* Do not show the _host dataverse_ field when there is nothing to choose. This mimics the behaviour for templates.
* When you accidentally start typing in the _host dataverse_ field, undo the change with backspace, fill in the other metadata fields and save the draft, the page used to get blocked due to an exception. Reloading the page would erase all your input. The exception (caused by an invalid argument) is remedied returning the currently selected value.
7 changes: 6 additions & 1 deletion src/main/java/edu/harvard/iq/dataverse/DatasetPage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@
import jakarta.faces.event.AjaxBehaviorEvent;
import jakarta.servlet.ServletOutputStream;
import jakarta.servlet.http.HttpServletResponse;
import jakarta.servlet.http.HttpServletRequest;

import org.apache.commons.text.StringEscapeUtils;
import org.apache.commons.lang3.mutable.MutableBoolean;
Expand Down Expand Up @@ -335,6 +334,7 @@ public enum DisplayMode {
private List<SelectItem> linkingDVSelectItems;
private Dataverse linkingDataverse;
private Dataverse selectedHostDataverse;
private boolean hasDataversesToChoose;

public Dataverse getSelectedHostDataverse() {
return selectedHostDataverse;
Expand Down Expand Up @@ -1767,6 +1767,11 @@ public void setDataverseTemplates(List<Template> dataverseTemplates) {
this.dataverseTemplates = dataverseTemplates;
}

public boolean isHasDataversesToChoose() {
this.hasDataversesToChoose = dataverseService.findAll().size() > 1;
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The method isHasDataversesToChoose() calls dataverseService.findAll().size() every time it's invoked, which could be expensive if there are many dataverses. Consider caching this value or using a more efficient query to count dataverses.

Suggested change
this.hasDataversesToChoose = dataverseService.findAll().size() > 1;
this.hasDataversesToChoose = dataverseService.countDataverses() > 1;

Copilot uses AI. Check for mistakes.
Copy link
Member

@qqmyers qqmyers Aug 20, 2025

Choose a reason for hiding this comment

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

DataverseServiceBean.getDataverseCount() exists and could be used here. - countDataverses() doesn't exist.

Copy link
Contributor Author

@jo-pol jo-pol Oct 2, 2025

Choose a reason for hiding this comment

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

@qqmyers mised these comment so far. Got another suggestion form copilot, perhaps more elaborate to implement but might be more effiecient

Query q = em.createQuery("SELECT 1 FROM Dataverse d");
q.setMaxResults(1);
boolean exists = !q.getResultList().isEmpty();

Copy link
Member

@qqmyers qqmyers Oct 2, 2025

Choose a reason for hiding this comment

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

Hmm - with maxResults set to one, you'd never know if there was more than one (thanks Copilot!), but I guess it could be limited to 2 instead.
That said, I think they key thing is to avoid getting a list of objects - that's a big expense.

From a quick test, getting two rows is cheaper than getting the count, but both are orders of magnitude less than getting the objects.

I guess since limiting to 2 rows will scale better than getting the count, I guess this approach makes more sense if you're willing to implement it.

Copy link
Contributor Author

@jo-pol jo-pol Oct 6, 2025

Choose a reason for hiding this comment

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

Just logged the duration on a VM with almost 100K dataverses

findall: PT15.632S
getDataverseCount: PT-0.026S

Looks good enough with less effort.

return this.hasDataversesToChoose;
}

public Template getDefaultTemplate() {
return defaultTemplate;
}
Expand Down
11 changes: 10 additions & 1 deletion src/main/java/edu/harvard/iq/dataverse/DataverseConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,29 @@
import jakarta.faces.convert.Converter;
import jakarta.faces.convert.FacesConverter;

import java.util.logging.Logger;

/**
*
* @author skraffmiller
*/
@FacesConverter("dataverseConverter")
public class DataverseConverter implements Converter {
private static final Logger logger = Logger.getLogger(DatasetPage.class.getCanonicalName());
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The logger is incorrectly configured to use DatasetPage.class.getCanonicalName() instead of DataverseConverter.class.getCanonicalName(). This will cause log messages to appear as if they're coming from DatasetPage rather than DataverseConverter.

Suggested change
private static final Logger logger = Logger.getLogger(DatasetPage.class.getCanonicalName());
private static final Logger logger = Logger.getLogger(DataverseConverter.class.getCanonicalName());

Copilot uses AI. Check for mistakes.


//@EJB
DataverseServiceBean dataverseService = CDI.current().select(DataverseServiceBean.class).get();

@Override
public Object getAsObject(FacesContext facesContext, UIComponent component, String submittedValue) {
return dataverseService.find(new Long(submittedValue));
if (submittedValue == null || !submittedValue.matches("[0-9]+")) {
logger.fine("Submitted value is not a host dataverse number but: " + submittedValue);
return CDI.current().select(DatasetPage.class).get().getSelectedHostDataverse();
}
else {
return dataverseService.find(Long.valueOf(submittedValue));
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

Using Long.valueOf() can throw NumberFormatException even though the regex check should prevent this. However, the regex [0-9]+ doesn't account for potential overflow of Long values. Consider using Long.parseLong() with proper exception handling or add bounds checking.

Suggested change
return dataverseService.find(Long.valueOf(submittedValue));
try {
return dataverseService.find(Long.parseLong(submittedValue));
} catch (NumberFormatException e) {
logger.severe("Submitted value is out of range for a Long: " + submittedValue);
return null;
}

Copilot uses AI. Check for mistakes.
}
//return dataverseService.findByAlias(submittedValue);
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/webapp/dataset.xhtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,7 @@
<!-- Create/Edit editMode -->
<ui:fragment rendered="#{DatasetPage.editMode == 'METADATA' or DatasetPage.editMode == 'CREATE'}">
<p:focus context="datasetForm"/>
<div class="form-group">
<div class="form-group" jsf:rendered="#{DatasetPage.hasDataversesToChoose and DatasetPage.editMode == 'CREATE'}">
<label jsf:for="#{DatasetPage.editMode == 'CREATE' ? 'selectHostDataverse_input' : 'select_HostDataverse_Static'}" class="col-md-3 control-label">
#{bundle.hostDataverse}
<span class="glyphicon glyphicon-question-sign tooltip-icon"
Expand Down