Issue #17 Contribute BYU Employee Time Reporting and Staff Leave Reporting portlets

[jameswennmacher]

Submitting HRS portlets from BYU:

• Employee Time Reporting
• Staff Leave Reporting
  

The two portlets are by default wired to an in-memory demo dao data source to allow them to be registered and added to a page for demo purposes. For actual use the dao would need to be replaced with an implementation that calls to Peoplesoft and populates the model, and sends model updates to Peoplesoft.

Notes:

• Implementation is not ajax-based. That is clearly a desired future feature.
• Both Leave Reporting controller and javascript calculate totals of rows and columns. Though it is desirable to remove the logic from the controller, this will have to wait for a future time due to budget constraints.

To test:

• Modify property preAuthenticatedUserDetailsService within bean springSecurityPortletFilterChain in portletApplicationContext.xml to use to avoid having a soap connection to Peoplesoft to get HR Roles. These portlets do not use HR Roles.
• The portlets can now be built, registered, and added to a tab.
• The demo dao data sources are very simple. They generate some limited data and process user interactions. Employee Time Reporting adds 5 minutes each clock-in/clock-out cycle. Staff Leave Reporting stores user sick or leave entries.

[apetro]

Would it be better to take this opportunity to go to an org.apereo... package structure, at least for the newly contributed code?

[jameswennmacher]

That's probably a good idea. Should it be org.apereo or org.jasig since everything else is org.jasig at this point (for consistency)?

[apetro]

This is all new code. If it can't be packaged as org.apereo, perhaps nothing can. :)

For that matter, maybe the boilerplate license headers should reference Apereo rather than Jasig at this point.

[apetro]

What's the ordering on the List?

[jameswennmacher]

The order to display on the UI. Good point to clarify in the docs.

[apetro]

Goofy capitalization of TimePUnchEntry.

[apetro]

Worth specifying in the interface the behavior in the case where that employee is already punched into that job code? Is already punched in to another job code?

[jameswennmacher]

Good point that I should enhance the documentation. The intention is to leave that to the dao implementation because the business rules may vary at different institutions. For BYU for instance, they want to allow that situation but return a message to the user because their business process allows a user to self-correct (externally) when they punch into a job code twice or punch into two different job codes. However if the user punches out to punch in again then it goes through a manager review process that they want to minimize.

And to your point, I need to document the mechanism for returning a message to the user (specific run-time exception).

[apetro]

Worth specifying on the interface the behavior if that employee is not punched in to that job code?

[jameswennmacher]

Similar point to above; leaving the handling to the dao implementation because the business rules may vary at different institutions. General expectation is doing nothing and returning a message to the user (via a specific run-time exception that I need to document).

[apetro]

This looks like a placeholder.

[apetro]

Order of List?

[jameswennmacher]

In this case order is not important. It could be a set but I'll leave as a list with a comment.

[apetro]

Worth doing some validation in the constructor? That payPeriodStart is not after payPeriodEnd?

[jameswennmacher]

Good catch.

[apetro]

Order of jobDescriptions List? Or is that an orderless collection better modeled as a Set?

[jameswennmacher]

It drives the order of display on the UI allowing the dao impls to put read-only summary of time worked on the top, holidays on the bottom, or whatever the institution wants for its ordering. Worthy of a note. Thanks.

[apetro]

Approach 2 at https://wiki.jasig.org/display/PLT/Logging+Best+Practices and thereby excising dependency on commons-logging and log4j is probably worth getting to.

[jameswennmacher]

Not sure I am following you. This is approach 1 which is easiest and most maintainable IMHO and the approach I use when upgrading portlets from log4j to logback.

[apetro]

Should this version number be externalized?

[jameswennmacher]

Sounds good.

[apetro]

Looks like a placeholder.

[apetro]

Is the order of this List meaningful?

[jameswennmacher]

Presented on the UI in that order allowing the DAO implementation to order however it sees fit. Worthy of improving the docs. Thanks again for such attention to detail and great questions.

[apetro]

Why is the PortletRequest making its way past the controller abstraction and into the service layer?

[jameswennmacher]

This is a strategy that Drew suggested to me in other situations to reduce maintenance and make the APIs less brittle to signature changes which would potentially impact other institutions that have custom implementations. Though I was initially resistant to the idea because of separation of concerns, I've found that his suggestion has a lot of merit at the service layer (less so at a dao layer) and I've seen its beneficial use in several of the portlet projects. Though it's not as useful in this particular case because these service layers are pretty much a wrapper for the dao, I had chosen to follow it as a convention.

[apetro]

Looks like a placeholder.

[apetro]

Do tell. :)

[jameswennmacher]

I do appreciate your excellent attention to detail! Thanks.

[apetro]

This kind of thing might want to be a log.trace since it's just tracing path through the call stack.

[jameswennmacher]

I can see trace level, but it is at debug to allow reporting on or troubleshooting caching.

[apetro]

Failing to punch out is more serious than a debug, no? This want to be a warn?

[jameswennmacher]

Intent is to display a message to the user on the UI, so I think debug is adequate.

[apetro]

Is it anticipated that a caller will want to differentially catch and handle HrPortletRuntimeException, or would this be just as well implemented as IllegalArgumentException?

[jameswennmacher]

I am generically using HrPortletRuntimeException to allow the DAO implementations (and other methods as well) to pass a message to display to the user on the page; e.g. a potentially user-recoverable error or at least alert message. I did that here as well for consistency though I see merit to using IllegalArgumentException as more descriptive.

[jameswennmacher]

Changed to IllegalArgumentException. Thanks for the suggestion.

[jameswennmacher]

Updated pull request with feedback, plus adding PortletRequest to dao to allow BYU to use arbitrary user attributes or portlet preferences in their dao implementation. Please review.

[apetro]

I'm really struggling with the abstraction violation of PortletRequest piercing the abstraction boundary all the way down to the DAO layer. 😕 PortletRequest should end at the Controller layer. To the extent that the data access layer needs a richer domain model to accommodate richer input or richer return values, well, need a richer domain model.

Got an example of what something is doing with a PortletRequest at the DAO layer that wouldn't be better factored without a PortletRequest at the DAO layer?

[timlevett]

I also agree that the PortletRequest shouldn't go into the DAO.

[apetro]

I don't think hrs-portlets-employee-time-reporting has a src/main/resources/xsd/hrs directory, does it? Not using JAXB to generate domain model / DTO classes? So can drop this <plugin/> ?

[apetro]

hrs-portlets-employee-time-reporting has no unit tests, so it has no test dependencies? Can drop these three dependencies?

[apetro]

How does this JobDescription relate to edu.wisc.hr.dm.person.Job (generated from hrs-portlet-person-1.0.xsd ) ?

[apetro]

How does this LeaveTimeBalance relate to edu.wisc.hr.dm.absbal.AbsenceBalance (generated from hrs-portlet-absbal-1.0.xsd )?

[jameswennmacher]

Related. I am using the approach that notifications, Courses, and other portlets use to have a java data model. The specific data sources would pull in their data however they do and incorporate it into the java data model, thus providing isolation and reuse. The front end controller, service, and view tiers are thus isolated from details of a Peoplesoft v3, highly customized Peoplesoft v2, or other data source.

I think this type of strategy is required to maximize re-use and simplify integrations. Note I did not go to the effort of insuring the java data model has all elements of the existing PS data model. I was hoping that would happen as Wisconsin evolved their portlets.

[apetro]

EDITED:

TL;DR

-0.9 for incorporation of this code into hrs-portlets. (Previously, -1.) +1 for birthing a separate project to house this new code.

Summary of Hangups:

Would be better factored as a separate project

These new portlets bring with them new APIs such that they do not meaningfully intersect with the existing hrs-portlets codebase. These are not additional portlets making use of the existing hrs-portlets APIs. This is not enhancements to the existing hrs-portlets APIs to make the new portlets possible. This is a separate codebase that doesn't benefit from being in hrs-portlets. It would be better factored as a separate codebase until and unless there's enough to gain from combining.

DAO API should not include PortletRequest

DAO APIs should not include PortletRequest parameters on their methods, because PortletRequest is a Controller-tier consideration not a DAO-tier consideration.

Even when, exceptionally, in practice some DAO implementations want or need access to the PortletRequest, including PortletRequest as a method parameter is unnecessary, in that access to the PortletRequest is available via RequestContextHolder.

When it is necessary, access to the PortletRequest via RequestContextHolder is better than making it a parameter on the DAO API since the PortletRequest is more an environmental context akin to the environmental context that makes @Value dependency injection work or that makes @Transactional interesting than it is a part of the domain API. API is about, here, HRS domain data considerations specifically, not environmental context generally. Using RequestContextHolder also has the technical advantage of the same DAO implementation can access session and request attributes from PortletRequest and ServletRequest interchangeably and so could drop unmodified into a portlet or servlet context, since RequestContext provides an abstraction for getting attributes independent of where those attributes are coming from.

---

So, big picture:

Thanks @jameswennmacher for all the work on this, I think this is great functionality to come available and it's been made better through the improvements through this pull request.

What we've got here are lovely entirely new portlets.

Whereas the existing hrs-portlets use XSD and JAXB and really ambitious occult technologies involving goat sacrifices to generate the domain model, these new portlets adopt the entirely sensible approach of a lovingly handcrafted Java domain model.

These new portlets don't make use of any of the existing domain model or API in hrs-portlets.

Now, I'm not saying these new portlets are wrong to avoid using what's here in hrs-portlets. There are certainly days when I think this JAXB / XSD-driven domain model generation, well, needs refactored away.

But it seems to me that packaging these portlets into the same web application, the same hrs-portlets-api, the same project with the existing portlets adds complexity without adding enough value to be worth it. The very first thing any adopter has to do is tease these right back apart again, figuring out how to implement only the parts of the API they actually need, only deploying the portlets they actually need. They're going to be understandably confused by some domain being defined in Java and some in XSD.

So here's my suggestion: the world needs a TimeClockPortlet project. Birth Unicon/time-clock-portlet or even Jasig/time-clock-portlet. These new portlets can take root there, not dragging the anchor of un-used hrs-portlet-api and XSD code generation and all that noise.

And then both Jasig/hrs-portlets and Jasig/time-clock-portlet evolve. Ideally, co-evolve. We spiffy up Jasig/hrs-portlets to deliver some finer-grained better factored APIs and implementations that Jasig/time-clock/portlet comes to depend upon via pom.xml Maven dependencies.

There's some duplication here. Let all the flowers bloom. Maybe we come to find that the APIs defined by Jasig/time-clock-portlet are just better, time-clock-portlet evolves to expose some great well-factored API .jars and implementation .jars, and we evolve Jasig/hrs-portlets to make use of those.

As you might guess, I'm no fan of the JAXB / XSD technology stack, personally. I'd be thrilled to retire some code. And maybe through that evolution we discover that these portlets should be collapsed together into one project and one portlet application.

But I'm not there yet. Looking at this, I don't think hrs-portlets is the right home for these new portlets. They're different. I'd rather see embracing that difference and seeing where it leads than making both more complex and making the adoption story more difficult by shoehorning these disparate portlets into one hrs-portlets project.

-0.9 for merge of this pull request as it stands. (Previously, -1).
+1 for making these portlets available as well-factored open source code in a new repo, dropping PortletRequest from the DAO API in the recognition that DAO implementations can nonetheless dereference PortletRequest via RequestContextHolder as much as they need to.

[jameswennmacher]

I think having these portlets in another portlet project is not the way to go. I think the better, longer-term strategy is to combine efforts. One sure way to have the portlets atrophy is to have a lot of fragmentation and little re-use, If everyone has their own portlet implementation, what are we gaining? The HRS portlets are very useful, but the portlet project will not be a heavily-used project and will not have a lot of adopters. By keeping the related portlets together, they have the greatest opportunity of code re-use and other's contributions helping out current adopters.

In the case of BYU, they are writing the back end (using the JAXB/XSD approach) to create their specific DAO implementations to map to the common Java object model. They will be able to (and are planning on) re-using some of that infrastructure with the Payroll Information and other HRS portlets that they are rolling out in phase 2 and 3. TBD is to find out if I have to create a Java Object Model for the Payroll Information implementation or if they can use their JAXB-generated objects with the current Wisconsin controller and JSP.

I suggest including these portlets into the HRS suite and (over time) adjust the current Wisconsin implementation to map the JAXB-generated objects to Java objects. If we focus our efforts on a good Java object model, a clean and flexible UI, and improve the mechanism of allowing adopters to select the portlets they want and provide their own DAO implementations for those portlets (ideally leveraging as much of the existing as possible by copy/paste example if nothing else), I think the project will be much farther along in the mid-term and adopted more widely.

[jameswennmacher]

RE PortletRequest going into the DAO layer, as I mentioned in a previous comment I too was initially resistant to the abstraction boundary transgression, but at least in this case I think there is merit to allowing the PortletRequest into the DAO layer.

The specific use case was that we are passing through the employeeID to the dao to access the data. It came up that they also need userID (don't recall exact reasons if for audit logging or security). Though I am OK with changing the API to pass in two strings; userId and employeeId as I can see those two potentially being required, I think we have an expectation in this case for different dao implementations that may have additional needs and it may be helpful to be able to leverage portlet preferences or different request attributes in the dao implementations, say a portlet preference for a template URL to access Peoplesoft or other HR system, or a need to get the Client IP (which I'm specifically passing in but wouldn't need to with PortletRequest) or potentially other user attributes for auditing purposes.

I agree there is not a strong compelling case requiring PortletRequest in the dao layer. It is a convenience (suggested by Drew based on his experiences) to make the API less brittle in certain use cases where the API's definition is likely to need to be more flexible. I felt in this case the benefit of flexibility out weighted the negatives of a strong, rigid boundary.

[apetro]

I think the better, longer-term strategy is to combine efforts.

I too am hopeful for collaboration opportunities over the long term. We're disagreeing about whether this is the juncture at which the long term has arrived.

If everyone has their own portlet implementation, what are we gaining?

The same thing we're gaining when portlets that don't technically benefit from being co-located are smashed into the same portlet application, and each adopter then teases them back apart to implement nonintersecting sets of portlets using nonintersecting portions of the APIs and implementations?

Which is, to be sure, greater than zero gain, in that at least some of the code is publicly available and transparent. Much to be said for sharing code publicly. But that public availability and transparency doesn't require sharing a portlet.xml, and it doesn't require a Java-centric API being placed next to an XSD-centric API.

Were this pull request merged, everyone would still have their own portlets, and their own backing APIs and implementations, it's just some of those are in the same repo rather than in separate repos. We're not so much collaborating here yet -- we're sharing parallel code.

A would-be adopter of the (have I mentioned, really quite attractive?) new time entry portlet is going to have a cleaner, clearer, better starting point if that portlet is factored as its own project carrying only the dependencies and complexity that it's using. That would-be adopter doesn't need to know anything about existing Jasig/hrs-portlets -- what's here in this repo is entirely a distraction to that effort at this juncture. That would-be adopter shouldn't thank us for making his life harder by putting these separate codebases into the same bucket. Releasing versions of the new portlet has nothing to do with releasing versions of the old portlets, or new or old APIs, or new or old implementations -- these are all separate stuff. These codebases should be combined when they gain enough from being combined.

Existing hrs-portlets is too immature and complected to be easily adoptable by anyone. I regret that the new portlets don't benefit from what's here, but it's understandable that they don't given where we're at. I'd probably have created the new JobDescription I actually needed for a new portlet too, rather than dealing with extending or evolving or composing with the defined-in-XSD Job domain object.

By all means, let's revisit this when the codebases mature to the point where either or both benefit from the other. Future BYU phases in which existing hrs-portlets will be implemented will I hope be an opportunity for collaboration and for reconciling how the architecture and APIs of these new portlets relate to the architecture and APIs of the existing hrs-portlets. That occasion might be the next pull request. It isn't this one.

[apetro]

wrt making context about the currently authenticated user available through the whole stack request-to-response:

The specific use case was that we are passing through the employeeID to the dao to access the data. It came up that they also need userID (don't recall exact reasons if for audit logging or security).

Consider implementing Spring Security for this kind of thing. Components can get the currently applicable Authentication off of the ThreadLocal via SecurityContextHolder. Models that this kind of cross-cutting-concern isn't so much part of the API and domain model so much as it's part of the environment context.

[drewwills]

A few (important!) words on passing a PortletRequest beyond a controller class...

This is a strategy Jen & I had developed over several quarters and many conversations. It has payed off hugely, and I consider it a keystone design principal. This idea doesn't, however, mean passing a PortletRequest to every type imaginable, every method.

In open source portlet development, we often found we needed pluggable strategies for things like...

• Authentication with remote systems
• Pulling data from remote systems
• Generating deep links to remote systems
• etc.

We don't have the resources to build and maintain separate Blackboard, Moodle, and Sakai portlets, so we focused on a single Courses portlet with pluggable data sources. (Even if we had the resources, separate portlets would be wasting them... there are better uses for those resources.)

These "pluggable" data sources are of course based on Java interfaces and basic polymorphism. Initially, we took the approach of putting method signatures on these interfaces based on simple types -- int, String, Date, etc. But we found that -- using this approach -- we were creating interfaces that looked too much like the first (initial) implementation, and that forced us to revise those interfaces constantly as we added support for new data sources.

This is not a good situation to be in. Many adopters of Apereo open source portlets have custom data sources that pull data (e.g.) from custom database tables or a custom RESTful JSON API. When you make changes to the interfaces for data sources, now newer versions of the portlet are not drop-in replacements... they're projects. We encourage folks to stay current. One way we make that more feasible for them is by making newer builds of portlets backwards-compatible with the work they did previously developing custom data sources.

So we developed a design pattern focused on making these interfaces as non-brittle as possible. The NotificationPortlet is a decent example. It's interface for data sources is called INotificationService: https://github.com/Jasig/NotificationPortlet/blob/master/notification-portlet-webapp/src/main/java/org/jasig/portlet/notice/INotificationService.java. We have several implementations of INotificationService that variously rely (iirc) on...

• REMOTE_USER
• Attributes in the USER_INFO map
• PortletPreferences
• The PortletSession
• Portlet events
• windowId

Passing the PortletRequest to data sources keeps the world of possibilities open to them, as well as quite a bit of clutter out of the controllers. It's cleaner for the controllers not to know how the data sources go about gathering data. It's less clean for implementation details of data sources to "spill out" into the controllers... particularly when you need to support very different strategies simultaneously.

The best reason I can think of not to pass a PortletRequest to a service interface would be if I thought it would need to be called outside a portlet lifecycle phase -- such as in a ServletRequest or in a Quartz job. (Though the first of those is easily handled with a method overload.) As long as a Java interface of class is compiled and packaged as a portlet project war file, chances are code that its methods will be invoked within portlet lifecycle phases. So this pattern/recommendation would not apply (necessarily) to utility jars that are pulled into a portlet project as a <dependency>.

But in the paragraph above I used the expression service interface. I prefer this term or the term "data source" for where this pattern/advice applies. Often this structure might go something like...

• Controller*
  | - IFooSource*
  | - JpaFooService*
  | - JpaFooDao

Where the Java types with an asterisk (*) would get a reference to the PortletRequest. In this pattern...

• The Controller does not need to know how the Jpa (or any) data source gathers data (it only knows the contract of IFooService, which is clean and resilient to change)
• The JpaFooService knows about what the JPA strategy needs, but does not actually implement the JPA logic
• The JpaFooDao implements the actual JPA logic, and is available to Quartz jobs (or anything) outside the portlet lifecycle

I consider all 3 of these points to be advantages. AFAIK most of the portlet code since uMobile has been developed with these principals.

[apetro]

So, without including the PortletRequest as a parameter on the DAO methods, from any request-servicing thread in a Spring PortletMVC portlet such as this one, one can get at the PortletRequest via something like:

RequestAttributes requestAttributes = 
    RequestContextHolder.getRequestAttributes();
PortletRequestAttributes portletRequestAttributes = 
    (PortletRequestAttributes) requestAttributes;
PortletRequest portletRequest = 
    portletRequestAttributes.getRequest();

Right?

[drewwills]

That works, in the majority of cases, but it strikes me more as a workaround for an uncommon need (and a very handy) than a mainstream way of modeling a core interaction between 2 layers or components. If my SpecialFooImpl class gets the request from the RequestContextHolder, how does client code know I need or use one? What if I want to narrow my domain behavior implementing method to an ActionRequest?

[apetro]

Changed -1 to -0.9. I don't think this code is best placed here and I don't think PortletRequest belongs on the DAO API, but I won't singlehandedly stand in the way of what might otherwise be consensus for path forward.

[timlevett]

Thanks for the clarification @drewwills . That seems to make sense to me. I think that we could go either way with separate portlets for everything or a bundle. As long as we can choose not to implement specific portlets I think we are okay with having them bundled. +1 for merge.

[ChristianMurphy]

@jameswennmacher Merge conflicts have cropped up, would you be able to resolve them?