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

Up 4621 Add Date When Portlet Rating Was Made #1065

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
03a2d59
Added getting and setting the date the rating was made per
phillips1021 Dec 6, 2017
19509d1
Added getting and setting the date the rating was made in the
phillips1021 Dec 6, 2017
24d2f54
added method definitions for finding portlet ratings done within the
phillips1021 Dec 6, 2017
b0d5f28
implement methods for finding portlet ratings done within the
phillips1021 Dec 6, 2017
185eeda
added unit tests for finding portlet ratings done within the
phillips1021 Dec 6, 2017
abb4656
added two new end points for getting ratings that were done within
phillips1021 Dec 6, 2017
b0756f5
added unit tests for two new end points for getting ratings that were…
phillips1021 Dec 6, 2017
9f91d4a
added additional fields so that we can more information related to a
phillips1021 Dec 6, 2017
44bd696
reformatted per Google Java Formatter
phillips1021 Dec 6, 2017
994261e
reformatted per Google Java Formatter
phillips1021 Dec 6, 2017
6b1df42
reformatted per Google Java Formatter
phillips1021 Dec 6, 2017
99396e2
reformatted per Google Java Formatter
phillips1021 Dec 6, 2017
21d30eb
reformatted per Google Java Formatter
phillips1021 Dec 6, 2017
2587721
reformatted per Google Java Formatter
phillips1021 Dec 6, 2017
3ec0bb9
changed to use LocalDateTime instead for calculating Today minus
phillips1021 Dec 6, 2017
d6764ce
use a final field and make it clearer how the test code is setting
phillips1021 Dec 6, 2017
6545dad
removed println statement that is not needed
phillips1021 Dec 7, 2017
136a235
changed to use final integer constant for setting rating date in
phillips1021 Dec 11, 2017
19d516b
changed to use Java 8 LocalDateTime for calculating date minus
phillips1021 Dec 11, 2017
a094ddb
style(java): apply Google Java code style
ChristianMurphy Dec 11, 2017
cdecd30
Merge branch 'master' of https://github.com/Jasig/uPortal into UP-4621
ChristianMurphy Dec 11, 2017
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
Original file line number Diff line number Diff line change
Expand Up @@ -165,4 +165,72 @@ public ModelAndView saveUserRating(
return new ModelAndView(
"json", "rating", new MarketplaceEntryRating(Integer.parseInt(rating), review));
}

/**
* Get all ratings done on portlets since daysBack ago.
*
* @param request
* @param daysBack - number of days back to search for portlet ratings
* @return
*/
@RequestMapping(value = "/marketplace/{daysBack}/getRatings", method = RequestMethod.GET)
Copy link
Member

Choose a reason for hiding this comment

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

❓ what if data from two weeks ago to last week is wanted?
Perhaps supporting a date range could future proof the API more?

Copy link
Author

Choose a reason for hiding this comment

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

Christian - if you or others would like to add more end points please do so. I'm done with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to add v5-1 to the URI.

public ModelAndView getPortletRatings(HttpServletRequest request, @PathVariable int daysBack) {

Set<IMarketplaceRating> portlets = marketplaceRatingDAO.getAllRatingsInLastXDays(daysBack);

List<MarketplaceEntryRating> ratings = new ArrayList<>();

for (IMarketplaceRating portlet : portlets) {

MarketplaceEntryRating portletRating =
new MarketplaceEntryRating(
portlet.getMarketplaceRatingPK().getUserName(),
portlet.getMarketplaceRatingPK().getPortletDefinition().getName(),
portlet.getMarketplaceRatingPK().getPortletDefinition().getFName(),
portlet.getRating(),
portlet.getReview(),
portlet.getRatingDate());

ratings.add(portletRating);
}

return new ModelAndView("json", "ratings", ratings);
}

/**
* Get all ratings done on a specific portlet since daysBack ago.
*
* @param request
* @param fname - portlet fname
* @param daysBack - number of days back to search for portlet ratings
* @return
*/
@RequestMapping(
value = "/marketplace/{fname}/{daysBack}/getRatings",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to add v5-1.

method = RequestMethod.GET
)
public ModelAndView getPortletRatings(
HttpServletRequest request, @PathVariable String fname, @PathVariable int daysBack) {

Set<IMarketplaceRating> portlets =
marketplaceRatingDAO.getAllRatingsForPortletInLastXDays(daysBack, fname);

List<MarketplaceEntryRating> ratings = new ArrayList<>();

for (IMarketplaceRating portlet : portlets) {

MarketplaceEntryRating portletRating =
new MarketplaceEntryRating(
portlet.getMarketplaceRatingPK().getUserName(),
portlet.getMarketplaceRatingPK().getPortletDefinition().getName(),
portlet.getMarketplaceRatingPK().getPortletDefinition().getFName(),
portlet.getRating(),
portlet.getReview(),
portlet.getRatingDate());

ratings.add(portletRating);
}

return new ModelAndView("json", "ratings", ratings);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,56 @@
*/
package org.apereo.portal.rest.layout;

import java.util.Date;

public class MarketplaceEntryRating {

public MarketplaceEntryRating() {}

public MarketplaceEntryRating(int rating, String review) {
this.rating = rating;
this.review = review;
}

private int rating;
private String review;
private String user;
private String portletName;
private Date ratingDate;
Copy link
Member

Choose a reason for hiding this comment

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

❓ Is the column automatically created?
❓ How are missing dates handled?

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 your questions - the values will be null if not provided.

Copy link
Member

Choose a reason for hiding this comment

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

I'm working to understand how this should be versioned, if this will be SemVer minor or SemVer major.

The first question has to do with if this code is run on an existing database, from before this column is added, does HSQL automatically add the column or does a manual migration have to be run.

For the second, when legacy data is brought in with date being null, does the code have a graceful fllaback? or will it NPE?

Copy link
Author

@phillips1021 phillips1021 Dec 12, 2017

Choose a reason for hiding this comment

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

I'll have to test this update but I think it will require a manual migration.

I'll run the current branch locally and do some ratings of portlets.

Then I'll run my branch with the same HSQL database.

How do I run my branch locally with uPortal-Start?

I don't think I introduced any code that depends on ratingDate having a value but I will double check.

Copy link
Member

Choose a reason for hiding this comment

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

How do I run my branch locally with uPortal-Start?

From the branch on uPortal ./gradlew install, this will build uPortal and create a snapshot on maven local.
Then head over to uPortal-start update gradle.properties to have uPortalVersion=5.0.3-SNAPSHOT.
Then run uPortal-start as usual, it will pull from the mavel local version.

📓 further updates to uPortal branch will require running ./gradlew install on the uPortal side again.

Copy link
Author

Choose a reason for hiding this comment

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

Here is how I tested

Ran uPortal-start with 5.0.1 version of uPortal

Created a few ratings as student and staff user

Built and installed locally my forked version of uPortal that includes adding RatingDate and new REST end points for market place controller.

Changed uPortal-start to use 5.0.3-SNAPSHOT and redeployed to Tomcat

Viewing any previous ratings or trying to create new ratings fails with SQL Exception due to missing RATINGDATE column in up_portlet_rating table

Added RATINGDATE column to up_portlet_rating table in HSQL database running locally using Squirrel SQL Client

Now can view previous ratings and can create new ratings

Tested new end points (e.g. http://localhost:8080/uPortal/api/marketplace/calendar/30/getRatings and http://localhost:8080/uPortal/api/marketplace/30/getRatings) and verified they work correctly.

Tested previous end points (e.g. http://localhost:8080/uPortal/api/marketplace/snooper/getRating and http://localhost:8080/uPortal/api/v5-0/marketplace/weather/ratings) and verified they work as before.

No Null Pointer Exceptions were generated when accessing ratings that were done previously and do not have a RATINGDATE value.

What is uPortal's process for updating the database schema to support a new release?

Do you feel this must be a major semantic version update (e.g. 6.X.X) due to the change in the database schema?

Bruce

Copy link
Member

@ChristianMurphy ChristianMurphy Dec 13, 2017

Choose a reason for hiding this comment

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

Thanks @phillips1021 ! 🙇


What is uPortal's process for updating the database schema to support a new release?

Whenever possible an automated migration path is provided. When an automated process is not possible a notice is added in the release notes describing the manual steps to migrate.

My understanding is import/export is frequently used for helping with migrations, @drewwills would be able to elaborate more on that.


Do you feel this must be a major semantic version update (e.g. 6.X.X) due to the change in the database schema?

short answer
There is still ongoing discussion on if/how the DB schema included in the API.
Personally, I lean towards yes, which means this would be SemVer major (6.0).
I think this could be made SemVer minor by providing a default value if there is a Null.

long answer
uPortal is still in a monolithic repo, with monolithic versioning, which leads to some ambiguity around, section 1 of the SemVer Spec

Software using Semantic Versioning MUST declare a public API. This API could be declared in the code itself or exist strictly in documentation. However it is done, it should be precise and comprehensive.

uPortal hasn't finished clearly defining what is included as an API.

There is a colorable argument that the project is a monolith, that database is intended for internal usage only, and therefore is outside semantic versioning.
There is also a colorable argument that the subprojects in the monorepo are independent, each exposes its own API, whenever one of the subproject API's is adds a new required element or removes an element, the Major version should be incremented.

I lean toward the former view. Meaning changes to the API that add a new required element to the API should be SemVer major.
Making the column optional could bring this back to SemVer minor.

private String portletFName;

public MarketplaceEntryRating(
String user,
String portletName,
String portletFName,
int rating,
String review,
Date ratingDate) {
this.user = user;
this.portletName = portletName;
this.portletFName = portletFName;
this.rating = rating;
this.review = review;
this.ratingDate = ratingDate;
}

@Override
public String toString() {
return "MarketplaceEntryRating{"
+ "rating="
+ rating
+ ", review="
+ review
+ ", user="
+ user
+ ", portletName="
+ portletName
+ ", ratingDate="
+ ratingDate
+ ", portletFName="
+ portletFName
+ '}';
}

public int getRating() {
return rating;
Expand All @@ -39,4 +80,36 @@ public String getReview() {
public void setReview(String review) {
this.review = review;
}

public String getUser() {
return user;
}

public void setUser(String user) {
this.user = user;
}

public String getPortletName() {
return portletName;
}

public void setPortletName(String portletName) {
this.portletName = portletName;
}

public Date getRatingDate() {
return ratingDate;
}

public void setRatingDate(Date ratingDate) {
this.ratingDate = ratingDate;
}

public String getPortletFName() {
return portletFName;
}

public void setPortletFName(String portletFName) {
this.portletFName = portletFName;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,28 @@ public void testGetUserRating() {
Assert.assertNull(modelAndView.getModel().get("rating"));
}

@Test
public void testAllPortletRatingDaysBack() {

String remoteUser = "jdoe";
Mockito.when(req.getRemoteUser()).thenReturn("jdoe");
IPortletType portletType =
new PortletTypeImpl("John Doe", "http://localhost:8080/uportal/test");
portletType.setDescription("Portlet Type");

PortletDefinitionImpl tempPortlet =
new PortletDefinitionImpl(
portletType, "John", "Doe", "Course", "app-id", "Courses", true);

Mockito.when(
marketplaceService.getOrCreateMarketplacePortletDefinitionIfTheFnameExists(
remoteUser))
.thenReturn(null);

ModelAndView modelAndView = marketplaceRESTController.getPortletRatings(req, F_NAME, 30);
Assert.assertNull(modelAndView.getModel().get("rating"));
}

@Test
public void testSaveUserRating() {
String rating = "3";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,21 @@ IMarketplaceRating createOrUpdateRating(

/** Aggregates the IMarketplaceRating into IPortletDefinition */
void aggregateMarketplaceRating();

/**
* Get all the ratings done in the last X days.
*
* @param numberOfDaysBack - number of days to go back to find ratings
* @return collection of IMarketplaceRating - can be null
*/
Set<IMarketplaceRating> getAllRatingsInLastXDays(int numberOfDaysBack);

/**
* Get all the ratings done for a specific portlet in the last X days.
*
* @param numberOfDaysBack - number of days to go back to find ratings
* @param fname - portlet fname
* @return collection of IMarketplaceRating - can be null
*/
Set<IMarketplaceRating> getAllRatingsForPortletInLastXDays(int numberOfDaysBack, String fname);
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/
package org.apereo.portal.portlet.marketplace;

import java.util.Date;
import org.apereo.portal.portlet.dao.jpa.MarketplaceRatingPK;

public interface IMarketplaceRating {
Expand All @@ -39,4 +40,8 @@ public interface IMarketplaceRating {
* @param review a text review of the portlet
*/
public void setReview(String review);

public void setRatingDate(Date ratingDate);

public Date getRatingDate();
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package org.apereo.portal.portlet.dao.jpa;

import com.google.common.base.Function;
import java.util.Date;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand All @@ -34,6 +35,7 @@
import org.apereo.portal.portlet.dao.IPortletDefinitionDao;
import org.apereo.portal.portlet.marketplace.IMarketplaceRating;
import org.apereo.portal.portlet.om.IPortletDefinition;
import org.joda.time.LocalDateTime;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.stereotype.Repository;
Expand Down Expand Up @@ -117,6 +119,7 @@ public IMarketplaceRating createOrUpdateRating(
temp.setMarketplaceRatingPK(tempPK);
temp.setRating(rating);
temp.setReview(review);
temp.setRatingDate(new Date());
return this.createOrUpdateRating(temp);
}

Expand Down Expand Up @@ -302,4 +305,82 @@ public void aggregateMarketplaceRating() {
}
}
}

@Override
public Set<IMarketplaceRating> getAllRatingsForPortletInLastXDays(
int numberOfDaysBack, final String fname) {

LocalDateTime todayMinusNumberOfDaysBack = LocalDateTime.now().minusDays(numberOfDaysBack);

final TypedQuery<MarketplaceRatingImpl> query =
this.createQuery(
this.createCriteriaQuery(
new Function<
CriteriaBuilder, CriteriaQuery<MarketplaceRatingImpl>>() {
@Override
public CriteriaQuery<MarketplaceRatingImpl> apply(
CriteriaBuilder input) {
final CriteriaQuery<MarketplaceRatingImpl> criteriaQuery =
input.createQuery(MarketplaceRatingImpl.class);
final Root<MarketplaceRatingImpl> definitionRoot =
criteriaQuery.from(MarketplaceRatingImpl.class);
Predicate ratingDatePredicate =
input.greaterThanOrEqualTo(
definitionRoot.<java.util.Date>get(
"ratingDate"),
todayMinusNumberOfDaysBack.toDate());
Predicate conditionPortlet =
input.equal(
definitionRoot
.get("marketplaceRatingPK")
.get("portletDefinition"),
portletDefinitionDao
.getPortletDefinitionByFname(
fname));
Predicate allConditions =
input.and(conditionPortlet, ratingDatePredicate);
criteriaQuery.select(definitionRoot).where(allConditions);

return criteriaQuery;
}
}));

return new HashSet<IMarketplaceRating>(query.getResultList());
}

@Override
@PortalTransactionalReadOnly
@OpenEntityManager(unitName = PERSISTENCE_UNIT_NAME)
public Set<IMarketplaceRating> getAllRatingsInLastXDays(int numberOfDaysBack) {

LocalDateTime todayMinusNumberOfDaysBack = LocalDateTime.now().minusDays(numberOfDaysBack);

final TypedQuery<MarketplaceRatingImpl> query =
this.createQuery(
this.createCriteriaQuery(
new Function<
CriteriaBuilder, CriteriaQuery<MarketplaceRatingImpl>>() {
@Override
public CriteriaQuery<MarketplaceRatingImpl> apply(
CriteriaBuilder input) {
final CriteriaQuery<MarketplaceRatingImpl> criteriaQuery =
input.createQuery(MarketplaceRatingImpl.class);
final Root<MarketplaceRatingImpl> definitionRoot =
criteriaQuery.from(MarketplaceRatingImpl.class);
Predicate ratingDatePredicate =
input.greaterThanOrEqualTo(
definitionRoot.<java.util.Date>get(
"ratingDate"),
todayMinusNumberOfDaysBack.toDate());

criteriaQuery
.select(definitionRoot)
.where(ratingDatePredicate);

return criteriaQuery;
}
}));

return new HashSet<IMarketplaceRating>(query.getResultList());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/
package org.apereo.portal.portlet.dao.jpa;

import java.util.Date;
import javax.persistence.Column;
import javax.persistence.EmbeddedId;
import javax.persistence.Entity;
Expand All @@ -33,6 +34,9 @@ class MarketplaceRatingImpl implements IMarketplaceRating {
@Column(name = "REVIEW", length = REVIEW_MAX_LENGTH)
private String review;

@Column(name = "RATINGDATE")
private Date ratingDate;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great addition, but (for a few reasons) we need to proceed with care.

One of those reasons is that this enhancement includes a database schema change.


@Override
public MarketplaceRatingPK getMarketplaceRatingPK() {
return marketplaceRatingPK;
Expand Down Expand Up @@ -69,12 +73,21 @@ public void setRating(int rating) {
this.rating = rating;
}

public Date getRatingDate() {
return ratingDate;
}

public void setRatingDate(Date ratingDate) {
this.ratingDate = ratingDate;
}

@Override
public String toString() {
return new ToStringBuilder(this)
.append("RatingPK: ", this.marketplaceRatingPK)
.append("Rating: ", this.rating)
.append("Review: ", this.review)
.append("Date: ", this.ratingDate)
.toString();
}
}
Loading