Skip to content

Commit

Permalink
add Adrians suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@ern
ern committed Nov 7, 2024
1 parent 3ea593a commit d81d9e2
Show file tree
Hide file tree
Showing 7 changed files with 86 additions and 72 deletions.
11 changes: 6 additions & 5 deletions kernel/kernel-impl/src/main/java/org/sakaiproject/site/impl/BaseSitePage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
package org.sakaiproject.site.impl;

import java.util.*;
import java.util.stream.Collectors;

import lombok.extern.slf4j.Slf4j;

Expand Down Expand Up @@ -261,11 +262,11 @@ protected BaseSitePage(BaseSiteService siteService, SitePage other, Site site, b
.setLazy(((BaseResourceProperties) other.getProperties()).isLazy());

// deep copy the tools
List<ToolConfiguration> otherTools = new ArrayList<>(bOther.getTools());
List<ToolConfiguration> copiedTools = new ArrayList<>(otherTools.size());
for (ToolConfiguration tool : otherTools) {
copiedTools.add(new BaseToolConfiguration(siteService, tool, this, exact));
}
List<ToolConfiguration> otherTools = new ArrayList<>(bOther.getTools());
final List<ToolConfiguration> copiedTools = otherTools.stream()
.map(tool -> new BaseToolConfiguration(siteService, tool, this, exact))
.collect(Collectors.toList());

m_tools = new ResourceVector(copiedTools);
m_toolsLazy = ((BaseSitePage) other).m_toolsLazy;

Expand Down
90 changes: 53 additions & 37 deletions ...r/components/src/java/org/sakaiproject/lessonbuildertool/model/SimplePageToolDaoImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,19 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Collectors;

import javax.persistence.criteria.CriteriaBuilder;
import javax.persistence.criteria.CriteriaQuery;
import javax.persistence.criteria.Root;

import lombok.Setter;
import lombok.extern.slf4j.Slf4j;

import org.hibernate.CacheMode;
import org.hibernate.SessionFactory;
import org.hibernate.query.Query;
import org.hibernate.type.LongType;
import org.hibernate.type.StringType;
Expand All @@ -60,8 +66,6 @@
import org.springframework.dao.DataAccessException;
import org.springframework.orm.hibernate5.HibernateTemplate;
import org.springframework.orm.hibernate5.support.HibernateDaoSupport;
import org.springframework.transaction.support.TransactionSynchronizationAdapter;
import org.springframework.transaction.support.TransactionSynchronizationManager;

import org.sakaiproject.authz.api.AuthzGroupService;
import org.sakaiproject.authz.api.SecurityService;
Expand Down Expand Up @@ -113,6 +117,7 @@ public class SimplePageToolDaoImpl extends HibernateDaoSupport implements Simple
private EventTrackingService eventTrackingService;
private PortalService portalService;
private SecurityService securityService;
private SessionFactory sessionFactory;
private ServerConfigurationService serverConfigurationService;
private SiteService siteService;
private SqlService sqlService;
Expand Down Expand Up @@ -151,7 +156,7 @@ public HibernateTemplate getDaoHibernateTemplate() {
// involving old data, for the sequence number. I'm not currently clearing the session cache, because this
// method is called before anyone has read any items, so there shouldn't be any old data there.
public void setRefreshMode() {
getSessionFactory().getCurrentSession().setCacheMode(CacheMode.REFRESH);
sessionFactory.getCurrentSession().setCacheMode(CacheMode.REFRESH);
}

public boolean canEditPage() {
Expand Down Expand Up @@ -440,22 +445,28 @@ public List<SimplePageItem> findPageItemsByPageId(long pageId) {
return (List<SimplePageItem>) getHibernateTemplate().findByCriteria(d);
}

private List<SimplePageItem> findSubpageItemsByPageId(long pageId) {
DetachedCriteria criteria = DetachedCriteria.forClass(SimplePageItem.class)
.add(Restrictions.eq("pageId", pageId))
.add(Restrictions.eq("type", SimplePageItem.PAGE));
List<SimplePageItem> list = (List<SimplePageItem>) getHibernateTemplate().findByCriteria(criteria);
list.sort(spiComparator);
return list;
}

private SimplePageItem findTopLevelPageItem(long pageId) {
DetachedCriteria criteria = DetachedCriteria.forClass(SimplePageItem.class)
.add(Restrictions.eq("sakaiId", Long.toString(pageId)))
.add(Restrictions.eq("type", SimplePageItem.PAGE));
List<SimplePageItem> l = (List<SimplePageItem>) getHibernateTemplate().findByCriteria(criteria);
return !l.isEmpty() ? l.get(0) : null;
}
private List<SimplePageItem> findSubPageItemsByPageId(long pageId) {
CriteriaBuilder cb = sessionFactory.getCriteriaBuilder();
CriteriaQuery<SimplePageItem> query = cb.createQuery(SimplePageItem.class);
Root<SimplePageItem> root = query.from(SimplePageItem.class);
query.select(root).where(cb.and(cb.equal(root.get("pageId"), pageId), cb.equal(root.get("type"), SimplePageItem.PAGE)));
List<SimplePageItem> results = sessionFactory.getCurrentSession().createQuery(query).getResultList();
results.sort(spiComparator);
return results;
}

private Optional<SimplePageItem> findTopLevelPageItem(long pageId) {
CriteriaBuilder cb = sessionFactory.getCriteriaBuilder();
CriteriaQuery<SimplePageItem> query = cb.createQuery(SimplePageItem.class);
Root<SimplePageItem> root = query.from(SimplePageItem.class);
query.select(root).where(cb.and(cb.equal(root.get("sakaiId"), pageId), cb.equal(root.get("type"), SimplePageItem.PAGE)));
List<SimplePageItem> result = sessionFactory.getCurrentSession().createQuery(query).getResultList();
if (result.isEmpty()) return Optional.empty();
else {
if (result.size() > 1) log.warn("query found more than one SimplePageItem where sakaiId={} and type=2", pageId);
return Optional.of(result.get(0));
}
}

// find the student's page. In theory we keep them from doing a second page. With
// group pages that means students in more than one group can only do one. So return the first
Expand Down Expand Up @@ -1018,15 +1029,21 @@ public List<SimplePage> getSitePages(String siteId) {
}

public SimplePage findPage(long pageId) {
DetachedCriteria d = DetachedCriteria.forClass(SimplePage.class).add(Restrictions.eq("pageId", pageId));
List<SimplePage> l = (List<SimplePage>) getHibernateTemplate().findByCriteria(d);
return (l != null && l.size() > 0) ? l.get(0) : null;
CriteriaBuilder cb = sessionFactory.getCriteriaBuilder();
CriteriaQuery<SimplePage> query = cb.createQuery(SimplePage.class);
Root<SimplePage> root = query.from(SimplePage.class);
query.select(root).where(cb.equal(root.get("pageId"), pageId));
List<SimplePage> result = sessionFactory.getCurrentSession().createQuery(query).getResultList();
return (result != null && !result.isEmpty()) ? result.get(0) : null;
}

public SimplePage findPageWithToolId(String toolId) {
DetachedCriteria d = DetachedCriteria.forClass(SimplePage.class).add(Restrictions.eq("toolId", toolId));
List<SimplePage> l = (List<SimplePage>) getHibernateTemplate().findByCriteria(d);
return (l != null && l.size() > 0) ? l.get(0) : null;
CriteriaBuilder cb = sessionFactory.getCriteriaBuilder();
CriteriaQuery<SimplePage> query = cb.createQuery(SimplePage.class);
Root<SimplePage> root = query.from(SimplePage.class);
query.select(root).where(cb.equal(root.get("toolId"), toolId));
List<SimplePage> result = sessionFactory.getCurrentSession().createQuery(query).getResultList();
return (result != null && !result.isEmpty()) ? result.get(0) : null;
}

public SimplePageLogEntry getLogEntry(String userId, long itemId, Long studentPageId) {
Expand Down Expand Up @@ -1144,7 +1161,7 @@ public SimplePageQuestionAnswer makeQuestionAnswer(String text, boolean correct)
// only implemented for existing questions, i.e. questions with id numbers
public boolean deleteQuestionAnswer(SimplePageQuestionAnswer questionAnswer, SimplePageItem question) {
if(!canEditPage(question.getPageId())) {
log.warn("User tried to edit question on page without edit permission. PageId: {}", question.getPageId());
log.warn("User tried to edit question on page without edit permission. PageId: {}", question.getPageId());
return false;
}

Expand Down Expand Up @@ -1546,7 +1563,7 @@ public int clearNeedsFixup(String siteId) {
try {
conn.setAutoCommit(wasCommit);
} catch (Exception e) {
log.info("transact: (setAutoCommit): {}", e);
log.warn("transact: (setAutoCommit): {}", e.toString());
}

sqlService.returnConnection(conn);
Expand All @@ -1564,7 +1581,7 @@ public void setNeedsGroupFixup(String siteId, final int value) {
final String property = "groupfixup " + siteId;
getHibernateTemplate().flush();

Session session = getSessionFactory().openSession();
Session session = sessionFactory.openSession();
Transaction tx = session.getTransaction();
try {
tx = session.beginTransaction();
Expand Down Expand Up @@ -1622,7 +1639,7 @@ public int clearNeedsGroupFixup(String siteId) {

int retval = 0;

Session session = getSessionFactory().openSession();
Session session = sessionFactory.openSession();
Transaction tx = session.getTransaction();
try {
tx = session.beginTransaction();
Expand Down Expand Up @@ -1677,8 +1694,8 @@ public Object readSqlResultRecord(ResultSet result) {
objectMap.put(newObject, "");
return null;
} catch (SQLException e) {
log.warn("findTextItemsInSite: {}", e);
return null;
log.warn("findTextItemsInSite: {}", e.toString());
return null;
}
}
});
Expand Down Expand Up @@ -1738,7 +1755,7 @@ public boolean deleteAllSavedStatusesForChecklist(SimplePageItem checklist) {
}
return true;
} catch (DataAccessException dae) {
log.warn("Unable to delete all saved status for checklist: {}", dae);
log.warn("Unable to delete all saved status for checklist: {}", dae.toString());
return false;
}
}
Expand All @@ -1751,7 +1768,7 @@ public boolean deleteAllSavedStatusesForChecklistItem(long checklistId, long che
}
return true;
} catch (DataAccessException dae) {
log.warn("Unable to delete all checklist item statuses for checklist item {}", dae);
log.warn("Unable to delete all checklist item statuses for checklist item {}", dae.toString());
return false;
}
}
Expand Down Expand Up @@ -1903,7 +1920,7 @@ public String getLessonSubPageJSON(final String userId, final String siteId, fin
try {
String sakaiToolId = siteService.getSite(siteId).getPage(p.getToolId()).getTools().get(0).getId();
m.put(p, sakaiToolId);
findSubpageItemsByPageId(p.getPageId()).forEach(spi -> lessonsSubNavBuilder.processResult(
findSubPageItemsByPageId(p.getPageId()).forEach(spi -> lessonsSubNavBuilder.processResult(
sakaiToolId,
p,
spi,
Expand All @@ -1919,11 +1936,10 @@ public String getLessonSubPageJSON(final String userId, final String siteId, fin
}

for (SimplePage p : lp) {
SimplePageItem spi = findTopLevelPageItem(p.getPageId());
if (spi != null) {
findTopLevelPageItem(p.getPageId()).ifPresent(spi -> {
SimplePageLogEntry le = getLogEntry(userId, spi.getId(), -1L);
lessonsSubNavBuilder.processTopLevelPageProperties(m.get(p), p, spi, le);
}
});
}

return lessonsSubNavBuilder.toJSON();
Expand Down
1 change: 1 addition & 0 deletions lessonbuilder/components/src/webapp/WEB-INF/components.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
<property name="portalService" ref="org.sakaiproject.portal.api.PortalService"/>
<property name="securityService" ref="org.sakaiproject.authz.api.SecurityService" />
<property name="serverConfigurationService" ref="org.sakaiproject.component.api.ServerConfigurationService" />
<property name="sessionFactory" ref="org.sakaiproject.springframework.orm.hibernate.GlobalSessionFactory"/>
<property name="siteService" ref="org.sakaiproject.site.api.SiteService" />
<property name="sqlService" ref="org.sakaiproject.db.api.SqlService" />
<property name="toolManager" ref="org.sakaiproject.tool.api.ActiveToolManager" />
Expand Down
2 changes: 1 addition & 1 deletion library/src/webapp/js/portal/portal.sidebar.sites.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class SitesSidebar {
this._subPageData = config?._subPageData;
this._pinnedSiteList = document.getElementById("pinned-site-list");
this._recentSiteList = document.getElementById("recent-site-list");
this._currentSite = config.currentSite;
this._currentSite = config?._currentSite;

const sitesListItems = element.querySelectorAll(".site-list-item");

Expand Down
48 changes: 22 additions & 26 deletions library/src/webapp/js/sub-page-navigation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,29 +34,29 @@ class SubPageNavigation {
const element = document.querySelector(`#toolMenu a[href*="/tool/${pageId}"], #toolMenu [href*="/tool-reset/${pageId}"]`);
let pageName = element.innerText?.trim() || this.topLevelPageProps[pageId].name;
const siteListItem = element.parentElement;
const mainLink = element.href ? element.href.replace(/\/tool\//, "/tool-reset/") : undefined;
const mainLink = element.href?.replace(/\/tool\//, "/tool-reset/");

const collapseId = `page-${pageId}-lessons-subpages`;
const isExpanded = (subpages[0].toolId === this.getCurrentPlacement());
const isExpanded = subpages[0].toolId === this.getCurrentPlacement();
const template = `
<div class="d-inline-flex align-items-stretch">
<button class="btn btn-nav btn-subsite rounded-end text-start ${(isExpanded) ? `` : `collapsed`} border-0 ps-4"
<button class="btn btn-nav btn-subsite rounded-end text-start ${(isExpanded) ? "" : "collapsed"} border-0 ps-4"
data-bs-toggle="collapse"
data-bs-target="#${collapseId}"
aria-expanded="${(isExpanded) ? `true` : `false`}"
aria-expanded="${(isExpanded) ? "true" : "false"}"
aria-controls="${collapseId}">
<i class="${(isExpanded) ? `bi-chevron-down` : `bi-chevron-right`}" aria-hidden="true"></i>
<i class="${(isExpanded) ? "si-expanded" : "si-collapsed"}" aria-hidden="true"></i>
<span>${pageName}</span>
</button>
</div>
<div id="${collapseId}" class="lessons-subpages-collapse ${(isExpanded) ? `show` : `collapse`}">
<div id="${collapseId}" class="lessons-subpages-collapse ${(isExpanded) ? "show" : "collapse"}">
<ul class="nav flex-column pe-2">
<li class="nav-item">
<a class="btn btn-nav rounded-end text-start ps-5" href="${mainLink}">
<i class="me-2 si si-sakai-lessonbuildertool" aria-hidden="true"></i>
<span>${this.i18n.main_link_name}</span>
${(props.disabled === 'true') ? `<i class="bi-slash-circle ms-2"></i>` : ``}
${(props.hidden === 'true' && props.disabled !== 'true') ? `<i class = "si si-hidden ms-2"></i>` : ``}
${(props.disabled === 'true') ? `<i class="bi-slash-circle ms-2"></i>` : ""}
${(props.hidden === 'true' && props.disabled !== 'true') ? `<i class = "si si-hidden ms-2"></i>` : ""}
</a>
</li>
${subpages.map((subpage) => `
Expand Down Expand Up @@ -92,20 +92,18 @@ class SubPageNavigation {
}

buildSubpageUrlFor(subpage) {
let url = `/portal/site/${subpage.siteId}`;
url += `/tool/${subpage.toolId}`;
url += `/ShowPage?sendingPage=${subpage.sendingPage}`;
url += `&itemId=${subpage.itemId}`;
url += `&path=clear_and_push`;
url += `&title=${subpage.name}`;
url += `&newTopLevel=false`;
return url;
return `/portal/site/${subpage.siteId}`
+ `/tool/${subpage.toolId}`
+ `/ShowPage?sendingPage=${subpage.sendingPage}`
+ `&itemId=${subpage.itemId}`
+ "&path=clear_and_push"
+ `&title=${subpage.name}`
+ "&newTopLevel=false";
}

getCurrentPlacement() {
const url = new URL(window.location.href);
const parts = url.pathname.split('/');
return (parts.length >= 6) ? parts[5] : '';
const parts = (new URL(window.location.href)).pathname.split('/');
return parts.length >= 6 ? parts[5] : '';
}

getSubPageElement() {
Expand All @@ -114,12 +112,12 @@ class SubPageNavigation {
const subPageNavItemIdInput = document.getElementById('lessonsSubnavItemId');
let subPageElement = null;

if ((subPageNavToolIdInput !== null) && (subPageNavPageIdInput !== null) && (subPageNavItemIdInput !== null)) {
if (subPageNavToolIdInput && subPageNavPageIdInput && subPageNavItemIdInput) {
subPageElement = document.querySelector(`#toolMenu a[href*="/tool/${subPageNavToolIdInput.value}/ShowPage?sendingPage=${subPageNavPageIdInput.value}&itemId=${subPageNavItemIdInput.value}&"]`);
}

// If the current page is not a subpage, then highlight the main page.
if (subPageElement == null && subPageNavToolIdInput != null) {
if (subPageElement && subPageNavToolIdInput) {
subPageElement = document.querySelector(`#toolMenu a[href$="/tool-reset/${subPageNavToolIdInput.value}"]`);
}

Expand All @@ -129,14 +127,12 @@ class SubPageNavigation {
setCurrentPage() {
let subpageElement = this.getSubPageElement();

if (subpageElement == null) {
if (!subpageElement) {
// We're not on a ShowPage (e.g., Index of Pages, ShowItem, etc.), so highlight Main Page in the site nav.
subpageElement = document.querySelector(`#toolMenu a[href$="/tool-reset/${this.getCurrentPlacement()}"]`);
}
if (subpageElement) {
subpageElement.classList.add("selected-page");
}
subpageElement?.classList.add("selected-page");
}
}

window.SubPageNavigation = SubPageNavigation;
window.SubPageNavigation = SubPageNavigation;
2 changes: 1 addition & 1 deletion portal/portal-api/api/src/java/org/sakaiproject/portal/api/PortalService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public interface PortalService
* current request. It should be a string, and should be implimented where
* the request is portlet dispatched.
*/
String PLACEMENT_ATTRIBUTE = PortalService.class.getName() + "_placementid";
String PLACEMENT_ATTRIBUTE = PortalService.class.getName() + "_placementid";

/**
* this is the property in the tool config that defines the portlet context
Expand Down
4 changes: 2 additions & 2 deletions ...l/portal-impl/impl/src/java/org/sakaiproject/portal/charon/site/AbstractSiteViewImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ public AbstractSiteViewImpl(PortalSiteHelperImpl siteHelper, HttpServletRequest
}


public boolean isEmpty()
{
@Override
public boolean isEmpty() {
return mySites.isEmpty();
}

Expand Down

0 comments on commit d81d9e2

Please sign in to comment.