On Tue, Oct 30, 2012 at 9:11 AM, Ate Douma <[email protected]> wrote: > On 10/29/2012 05:11 AM, Chris Geer wrote: > >> Branch is now merged into trunk. The only change I had to make was to >> the OmdlOutputAdapter class where I had to comment out the @Component >> annotation. This was because that class now needed a constructor that took >> several services to function and @Autowire wasn't working due to order of >> creation issues I believe. The only place I could find it was used in the >> code was a place it was manually constructed so I think we are fine but I >> may have missed something. I ran all the unit tests, integration tests and >> also spot checked a lot of the capabilities. >> > > Hi Chris, > > While trying to reconcile these model-split changes against trunk with the > content-services sandbox, I encountered a few issues, all related to the > RegionWidget - Widget separation. > One minor issue is I think a simple merge error but others aspects are > more structural. > > First of all, currently Rave breaks when accessed from a mobile device: > mobile rendering now throws a JSP rendering exception because the > <rave:render-widget/> tag has an extra and required widget parameter which > hasn't been set (mobile.jsp #78). >
I will try and fix. > > Another issue is that the Profile page no longer renders (static) widgets > because the ProfileController doesn't derive and saves the list of widgets > in the page to the model (as PageController does). I think you did have > this in the branch but it got lost during the merge back into trunk. > I will try and fix. > > This functionality, retrieving and deriving the widgets from the page > region(s) RegionWidgets and storing them as a list on the model so they are > available during rendering, I'm not very happy with. Especially not when it > is stored as a list. A map (WidgetId->Widget) might be better already so we > don't need to iterate on the whole list to find back a matching single > widget, as for example now done in region.tag. > But maybe an even cleaner alternative would be retrieving a Widget 'on > demand' through a custom tag itself, similar to the PersonTag does? > And tags as RegionWidgetTag then can do this internally and then wouldn't > need the extra widget parameter. > > Thoughts? > >> After integration I did find one bug >> RAVE-837<https://issues.**apache.org/jira/browse/RAVE-**837<https://issues.apache.org/jira/browse/RAVE-837>> >> that >> >> I will try and fix in the next day or so. Please file tickets for any >> other >> bugs and we'll squash them. >> >> Just remember now, all the IDs in the interfaces are Strings now, not >> Longs. Longs are still used in the JPA implementation and converted to >> strings at the interface level. >> >> @Jasha - I fixed the category problem prior to merge. >> >> Chris >> >> On Fri, Oct 26, 2012 at 4:30 AM, Jasha Joachimsthal <[email protected] >> >wrote: >> >> Hi Chris, >>> >>> I did a few tests and found the following things: >>> >>> Widget Store shows a log entry about tags: >>> [INFO] [talledLocalContainer] 465793 ravePersistenceUnit WARN >>> [http-8080-1] openjpa.Runtime - Supplied user parameters "[widgetId, >>> tagId]" do not match expected parameters "[widgetId]" for the prepared >>> query "PreparedQuery: [select t from JpaWidgetTag t where >>> t.widgetId=:widgetId and t.tagId=:tagId] --> [SELECT t0.entity_id, >>> t0.created_date, t0.tag_id, t0.user_id, t0.widget_id FROM widget_tag t0 >>> WHERE (t0.widget_id = ? AND t0.tag_id IS NULL)]". >>> >>> In the Widget store if you select a category and then select the empty >>> category you get an error page: >>> URL: >>> >>> http://localhost:8080/portal/**app/store/category?categoryId=** >>> 0&referringPageId=1(logged<http://localhost:8080/portal/app/store/category?categoryId=0&referringPageId=1(logged> >>> in as canonical) >>> [WARNING] [talledLocalContainer] java.lang.NullPointerException >>> [WARNING] [talledLocalContainer] at >>> >>> org.apache.rave.portal.**service.impl.**DefaultWidgetService.** >>> getWidgetsByCategory(**DefaultWidgetService.java:181) >>> >>> The integration tests passed except the last one (OpenID login), but that >>> one has been fixed in the trunk. >>> No real blockers IMO. >>> >>> Jasha >>> >>> >>> On 25 October 2012 19:12, Chris Geer <[email protected]> wrote: >>> >>> All, I will be merging this over the weekend if there are no objections. >>>> (Sunday should be 72 hours). At that time I will do the final reverse >>>> >>> merge >>> >>>> into the branch and merge into trunk. >>>> >>>> Chris >>>> >>>> On Fri, Oct 12, 2012 at 5:29 PM, Chris Geer <[email protected]> >>>> >>> wrote: >>> >>>> >>>> I think we are at a point where the model-split work is far enough >>>>> >>>> along >>> >>>> that we should consider merging it soon. I need to finish up >>>>> >>>> refactoring >>> >>>> WidgetRating but after that there will be enough critical mass to merge >>>>> >>>> it >>>> >>>>> into the baseline IMHO. Prior to that happening it would be good if we >>>>> >>>> can >>>> >>>>> get several people to spend a little time and review the changes that >>>>> >>>> have >>>> >>>>> been made. Any volunteers? >>>>> >>>>> The major changes are: >>>>> 1) All IDs in the interfaces have been changed from int to String. This >>>>> >>>> is >>>> >>>>> to support a more flexible ID structure in the future with backends >>>>> >>>> other >>> >>>> than JPA >>>>> 2) The model has been split into several related chunks. The major >>>>> >>>> chunks >>> >>>> are Users/People, Widget and other right now. Object relationships >>>>> >>>> between >>>> >>>>> those groups have been broken and replaced with IDs. This is to >>>>> >>>> support a >>> >>>> more modular backend eventually. Right now they are all still part of >>>>> >>>> the >>> >>>> same JPA persistance unit but that is short term. >>>>> 3) The widget model has been changed to make Widget the top level >>>>> >>>> object >>> >>>> and WidgetComment, WidgetTag & WidgetRating are subordinate objects. >>>>> >>>> These >>>> >>>>> changes include removing the widgetID attribute from the subordinate >>>>> >>>> object >>>> >>>>> so that they are associated with the Widget they are attached to. We >>>>> >>>> also >>> >>>> consolidated the various services and repositories into the >>>>> WidgetService/Repository since acting on the subordinate >>>>> object independent of the widget isn't ideal. >>>>> 4) The link between RegionWidget and Widget has been replaced with >>>>> WidgetID. This has required that we populate the JSP attributes with >>>>> >>>> the >>> >>>> list of widgets on a page (in the controller). >>>>> >>>>> There are plenty of other changes we could make so if there are any you >>>>> think are crucial before the merge please point those out but I'm >>>>> >>>> hoping >>> >>>> we >>>> >>>>> can get the major changes into trunk and then iterate through the rest. >>>>> >>>>> Thanks, >>>>> Chris >>>>> >>>>> >>>> >>> >> >
