ruote-web: why are the preserved fields stored in the session, instead of being re-calculated when needed?
Arjan van Bentem
Just wondering, not an issue at all:
In ruote-web/Densha's WorkItemController I found the following code,
which ensures that preserved fields are not changed during an update:
http://github.com/jmettraux/ruote-web/tree/master/app/controllers/workitem_controller.rb#L252-260
def load_workitem (workitem_id)
@worklist = Worklist.new(session[:user])
@workitem = OpenWFE::Extras::Workitem.find workitem_id
@fields, preserved_fields = filter_fields @workitem
session[:workitem] = @workitem.id, preserved_fields
http://github.com/jmettraux/ruote-web/tree/master/app/controllers/workitem_controller.rb#L100-105
def update
user = session[:user]
workitem_id, preserved_fields = session[:workitem]
workitem = OpenWFE::Extras::Workitem.find workitem_id
This makes me wonder: as in the last line the workitem is loaded
anyway, couldn't one simply not store the preserved fields in the
session, and then use:
def update
user = session[:user]
workitem_id = session[:workitem]
workitem = OpenWFE::Extras::Workitem.find workitem_id
normal_fields, preserved_fields = filter_fields @workitem
Here the value of "normal_fields" would not be used (maybe one can
assign to nil in Ruby?).
So, my question: is this is a matter of some performance gain, or just
something that is not really required, or am I missing something --
like some concurrency issues?
Arjan
-- John, note the range you wanted: #L100-105 -- just hold down Shift
when clicking :-)
Arjan van Bentem
> and then use:
>
> def update
> user = session[:user]
> workitem_id = session[:workitem]
> workitem = OpenWFE::Extras::Workitem.find workitem_id
> normal_fields, preserved_fields = filter_fields @workitem
Well, without the @-character of course:
normal_fields, preserved_fields = filter_fields workitem
Arjan.
Arjan van Bentem
> Well, without the @-character of course:
>
> normal_fields, preserved_fields = filter_fields workitem
Allright, I thought I'd simply try, which revealed a small bug in
WorkitemController ;-)
http://github.com/jmettraux/ruote-web/tree/master/app/controllers/workitem_controller.rb#L228-233
def filter_fields (workitem)
fields = {}
preserved_fields = {}
@workitem.fields_hash.each do |k, v|
Above, the class instance variable @workitem is used, instead of the
method parameter.
Arjan.
John Mettraux
<arjan.v...@bidnetwork.org> wrote:
>
> http://github.com/jmettraux/ruote-web/tree/master/app/controllers/workitem_controller.rb
Hello Arjan,
no performance trick here. Do you have time to come up with a patch to
clean my mess here ?
Cheers,
--
John Mettraux - http://jmettraux.wordpress.com
John Mettraux
> On Fri, May 9, 2008 at 5:30 AM, Arjan van Bentem
> <arjan.v...@bidnetwork.org> wrote:
>>
>> http://github.com/jmettraux/ruote-web/tree/master/app/controllers/workitem_controller.rb
Hello Arjan,
I came up with a clean up :
http://github.com/jmettraux/ruote-web/commit/f2cd2b9517debe01c34f8a6f9cdcc050cba92d92
Please tell me if there are other things you'd like to clean up in there.
Arjan van Bentem
You're unstoppable, thanks ;-)
I cannot test the changes right now, but have taken a look at the new
code.
I don't understand what happens to the filtered fields now. In
load_workitem they are not added to @fields, so won't be populated in
ruote-web's form, and thus are not available upon submit. Right? If
true, and now that you're no longer merging the preserved fields with
the submitted fields, don't the preserved fields get lost on line 134?
134 workitem.replace_fields fields
237 def filter_fields (workitem)
238 @workitem.fields_hash.inject({}) do |r, (k, v)|
239 r[k] = v if k[0, 1] != '_'
240 r
241 end
241 end
250 def load_workitem (workitem_id)
:
258 @fields = filter_fields @workitem
Allright, Ruby -- I needed my pink O'Reilly book for that inject
syntax. For a moment I thought using select! or reject! would the same
(wrongly assuming that using the exclamation mark would change
fields_hash), but those methods don't exist or don't work like that :-)
So filter_fields(..) changes @workitem.fields_hash and also yields
that as the return value, right? Or does it return something else?
Shouldn't line 238 refer to parameter "workitem", instead of
"@workitem"? If not, then I assume the parameter is not really
needed :-)
A minor typo:
119 # gather the 'hash_form_field_X' form entries as well
should read
119 # gather the 'workitem_form_field_X' form entries as well
or, if not, then the REGEX is wrong ;-)
Arjan.
-- major internet problems in the Netherlands, since Monday night :-(
John Mettraux
<arjan.v...@bidnetwork.org> wrote:
>
>
>
> You're unstoppable, thanks ;-)
>
> I cannot test the changes right now, but have taken a look at the new
> code.
>
> I don't understand what happens to the filtered fields now. In
> load_workitem they are not added to @fields, so won't be populated in
> ruote-web's form, and thus are not available upon submit. Right? If
> true, and now that you're no longer merging the preserved fields with
> the submitted fields, don't the preserved fields get lost on line 134?
Hi Arjan,
you are right for all the three points. I will fix that ASAP.
Thanks a lot for your code review !
John Mettraux
> On Thu, May 15, 2008 at 4:11 PM, Arjan van Bentem
> <arjan.v...@bidnetwork.org> wrote:
>>
>> I don't understand what happens to the filtered fields now. In
>> load_workitem they are not added to @fields, so won't be populated in
>> ruote-web's form, and thus are not available upon submit. Right? If
>> true, and now that you're no longer merging the preserved fields with
>> the submitted fields, don't the preserved fields get lost on line 134?
>
> Hi Arjan,
>
> you are right for all the three points. I will fix that ASAP.
http://github.com/jmettraux/ruote-web/commit/4f20127a9ff6a4082e5c1a45bc6108d0277d87af
should be OK like that.
Thanks again,