Up 4621 Add Date When Portlet Rating Was Made #1065

phillips1021 · 2017-12-06T23:48:18Z

Checklist

[X ] the individual contributor license agreement is signed
[ X] commit message follows commit guidelines
[X ] tests are included

Description of change

Added date a portlet rating was made and added new end points for the MarketPlaceRESTController so we can get portlet ratings done within the past X days

https://issues.jasig.org/browse/UP-4621

implementation per https://issues.jasig.org/browse/UP-4621

last X days (e.g. within the last 30 days) related to https://issues.jasig.org/browse/UP-4621

a certain number of days - for example all ratings done within last 30 days or all ratings for a specific portlet within last 30 days see: https://issues.jasig.org/browse/UP-4636

… done within a certain number of days - for example all ratings done within last 30 days or all ratings for a specific portlet within last 30 days see: https://issues.jasig.org/browse/UP-4636

portlet rating that can then be returned by the REST end point defined in MarketplaceRESTController see: https://issues.jasig.org/browse/UP-4636

a number of days for searching portlet ratings

the portlet rating date in the past

ChristianMurphy

Thanks @phillips1021! 🙇
Keeping track of the date-time of ratings is a nice addition to uPortal. 👍

If you have't already, could you file a license agreement with Apereo? https://www.apereo.org/licensing/agreements

Continued from #1064

Looks better, thanks for running the Google code style formatter! 🙇

📓 its helpful to note, that additional commits pushed to a git branch with an open pull request, will automatically be included as part of the pull request.

ChristianMurphy · 2017-12-07T00:22:58Z

uPortal-webapp/src/test/java/org/apereo/portal/portlet/dao/jpa/JpaMarketplaceRatingDaoTest.java

+                                        "comment for "
+                                                + ratingPK.getPortletDefinition().getFName());
+                                marketplaceRatingDao.createOrUpdateRating(rating);
+                                i = i + 20;


❓ Could i be named something numberOfDaysBack to give some additional clarity on the purpose?
❓ maybe 20 could become something like final int TEST_DATE_INCREMENT = 20?

continued from #1064 (comment)

made this change

Thanks @phillips1021!Ccould the change also be pushed to GitHub?

It was pushed to GitHub previously - see: phillips1021@d6764ce

Line 395 uses the same constant for the same purpose.

ChristianMurphy · 2017-12-07T00:25:01Z

uPortal-web/src/main/java/org/apereo/portal/portlet/dao/jpa/JpaMarketplaceRatingDao.java

+    public Set<IMarketplaceRating> getAllRatingsInLastXDays(int numberOfDaysBack) {
+
+        final Calendar queryDate = Calendar.getInstance();
+        queryDate.add(Calendar.DAY_OF_YEAR, 0 - numberOfDaysBack);


❓ Could the Java 8 Time processing utils be used to simplify/clarify the code?
http://www.baeldung.com/migrating-to-java-8-date-time-api

continued from #1064 (comment)

made this change

Thanks @phillips1021!Ccould the change also be pushed to GitHub?

It was pushed to GitHub previously - see: phillips1021@3ec0bb9

line 358 uses the same subtract operation and has not been updated.

ChristianMurphy · 2017-12-07T00:32:09Z

uPortal-webapp/src/test/java/org/apereo/portal/portlet/dao/jpa/JpaMarketplaceRatingDaoTest.java

+
+                            for (ILocalAccountPerson person : personList) {
+                                today.add(Calendar.DAY_OF_YEAR, 0 - numberOfDaysBack);
+                                System.out.println(


📓 System.out.format may be able to help here

Removed the println statement - it was not really needed

unit test

number of days back

ChristianMurphy

LGTM, thanks @phillips1021! 🙇

updated automatically with `./gradlew goJF`

drewwills · 2017-12-11T18:57:23Z

uPortal-web/src/main/java/org/apereo/portal/portlet/dao/jpa/MarketplaceRatingImpl.java

@@ -33,6 +34,9 @@
    @Column(name = "REVIEW", length = REVIEW_MAX_LENGTH)
    private String review;

+    @Column(name = "RATINGDATE")
+    private Date ratingDate;


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.

ChristianMurphy

some additional questions:

ChristianMurphy · 2017-12-11T23:14:37Z

...tal-api/uPortal-api-rest/src/main/java/org/apereo/portal/rest/MarketplaceRESTController.java

+     * @param daysBack - number of days back to search for portlet ratings
+     * @return
+     */
+    @RequestMapping(value = "/marketplace/{daysBack}/getRatings", method = RequestMethod.GET)


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

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

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

ChristianMurphy · 2017-12-11T23:16:22Z

...api/uPortal-api-rest/src/main/java/org/apereo/portal/rest/layout/MarketplaceEntryRating.java

    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;


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

I don't understand your questions - the values will be null if not provided.

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?

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.

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.

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

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.

drewwills · 2018-03-06T23:29:40Z

...tal-api/uPortal-api-rest/src/main/java/org/apereo/portal/rest/MarketplaceRESTController.java

+     * @return
+     */
+    @RequestMapping(
+        value = "/marketplace/{fname}/{daysBack}/getRatings",


I'd like to add v5-1.

phillips1021 added 16 commits December 6, 2017 10:35

Added getting and setting the date the rating was made per

03a2d59

https://issues.jasig.org/browse/UP-4621

Added getting and setting the date the rating was made in the

19509d1

implementation per https://issues.jasig.org/browse/UP-4621

added method definitions for finding portlet ratings done within the

24d2f54

last X days (e.g. within the last 30 days) related to https://issues.jasig.org/browse/UP-4621

implement methods for finding portlet ratings done within the

b0d5f28

last X days (e.g. within the last 30 days) related to https://issues.jasig.org/browse/UP-4621

added unit tests for finding portlet ratings done within the

185eeda

last X days (e.g. within the last 30 days) related to https://issues.jasig.org/browse/UP-4621

added two new end points for getting ratings that were done within

abb4656

a certain number of days - for example all ratings done within last 30 days or all ratings for a specific portlet within last 30 days see: https://issues.jasig.org/browse/UP-4636

added unit tests for two new end points for getting ratings that were…

b0756f5

… done within a certain number of days - for example all ratings done within last 30 days or all ratings for a specific portlet within last 30 days see: https://issues.jasig.org/browse/UP-4636

added additional fields so that we can more information related to a

9f91d4a

portlet rating that can then be returned by the REST end point defined in MarketplaceRESTController see: https://issues.jasig.org/browse/UP-4636

reformatted per Google Java Formatter

44bd696

reformatted per Google Java Formatter

994261e

reformatted per Google Java Formatter

6b1df42

reformatted per Google Java Formatter

99396e2

reformatted per Google Java Formatter

21d30eb

reformatted per Google Java Formatter

2587721

changed to use LocalDateTime instead for calculating Today minus

3ec0bb9

a number of days for searching portlet ratings

use a final field and make it clearer how the test code is setting

d6764ce

the portlet rating date in the past

ChristianMurphy requested changes Dec 7, 2017

View reviewed changes

ChristianMurphy reviewed Dec 7, 2017

View reviewed changes

ChristianMurphy added this to the 5.1.0 milestone Dec 7, 2017

phillips1021 added 3 commits December 7, 2017 10:03

removed println statement that is not needed

6545dad

changed to use final integer constant for setting rating date in

136a235

unit test

changed to use Java 8 LocalDateTime for calculating date minus

19d516b

number of days back

ChristianMurphy approved these changes Dec 11, 2017

View reviewed changes

ChristianMurphy added 2 commits December 11, 2017 09:07

style(java): apply Google Java code style

a094ddb

updated automatically with `./gradlew goJF`

Merge branch 'master' of https://github.com/Jasig/uPortal into UP-4621

cdecd30

ChristianMurphy requested review from apetro, drewwills, bjagg and vertein December 11, 2017 18:17

drewwills reviewed Dec 11, 2017

View reviewed changes

ChristianMurphy reviewed Dec 11, 2017

View reviewed changes

ChristianMurphy self-requested a review December 11, 2017 23:17

ChristianMurphy modified the milestones: 5.1.0, 6.0.0 Dec 15, 2017

drewwills reviewed Mar 6, 2018

View reviewed changes

bjagg added work in progress help wanted labels Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Up 4621 Add Date When Portlet Rating Was Made #1065

Up 4621 Add Date When Portlet Rating Was Made #1065

phillips1021 commented Dec 6, 2017

ChristianMurphy left a comment

ChristianMurphy Dec 7, 2017

phillips1021 Dec 7, 2017

ChristianMurphy Dec 8, 2017

phillips1021 Dec 11, 2017

ChristianMurphy Dec 11, 2017

ChristianMurphy Dec 7, 2017

phillips1021 Dec 7, 2017

ChristianMurphy Dec 8, 2017

phillips1021 Dec 11, 2017

ChristianMurphy Dec 11, 2017

ChristianMurphy Dec 7, 2017

phillips1021 Dec 7, 2017

ChristianMurphy left a comment

drewwills Dec 11, 2017

ChristianMurphy left a comment

ChristianMurphy Dec 11, 2017

phillips1021 Dec 12, 2017

drewwills Mar 6, 2018

ChristianMurphy Dec 11, 2017

phillips1021 Dec 12, 2017

ChristianMurphy Dec 12, 2017

phillips1021 Dec 12, 2017 •

edited

Loading

ChristianMurphy Dec 12, 2017

phillips1021 Dec 12, 2017

ChristianMurphy Dec 13, 2017 •

edited

Loading

drewwills Mar 6, 2018

Up 4621 Add Date When Portlet Rating Was Made #1065

Are you sure you want to change the base?

Up 4621 Add Date When Portlet Rating Was Made #1065

Conversation

phillips1021 commented Dec 6, 2017

Checklist

Description of change

ChristianMurphy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianMurphy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianMurphy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillips1021 Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianMurphy Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillips1021 Dec 12, 2017 •

edited

Loading

ChristianMurphy Dec 13, 2017 •

edited

Loading