On Tue, Oct 30, 2012 at 10:57 AM, Chris Geer <[email protected]> wrote:
> 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. > >> >> Fixed RAVE-839 > 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. > >> >> Fixed RAVE-840 > 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 >>>>>> >>>>>> >>>>> >>>> >>> >> >
