On 1 April 2016 at 16:09, Sam Ruby <ru...@intertwingly.net> wrote:
> On Fri, Apr 1, 2016 at 10:42 AM, Craig Russell <craig.russ...@oracle.com>
> wrote:
>> Just a few questions/comments on the code.
>>
>> 1. entry = ASF::Person.find(@userid).members_txt(true) does some heavy
>> lifting and then a few lines later, message = "Move
>> #{ASF::Person.find(@userid).member_name} to #{@action}” which lifts the same
>> weight. Could this be replaced with message = "Move #{entry.member_name} to
>> #{@action}”?
>
> person.members_txt() returns a string, so entry.member_name won't work.
>
>> Or is Person.find really a lightweight function?
>
> ASF::Person.find caches its results, so subsequent calls are light weight.
>
> If you feel so inclined, feel free to refactor this to person =
> ASF::Person.find(@userid); and then change the two places were
> Person.find is called to use this value.
>
>> 2. There is a section “Deceased”. Would it make sense to add a few lines of
>> code for this section?
>
> It would not be difficult to do. Go for it!
>
>> 3. Where did the @action parameter come from?
>
> It comes from the form. See:
>
> https://github.com/apache/whimsy/blob/ef8a40770cbba9da72da0bfacb45cf334ab248ec/www/roster/views/committer.js.rb#L187
>
> If you were to add other buttons, the name/value pairs on those
> buttons (or input fields, or textareas, etc), will be used to set
> instance variables (i.e., variables whose names start with an '@') to
> the associated value.
>
> Feel free to experiment. If you were to create a button and push that
> change, it would only show up to people listed in the asf-secretary
> LDAP service:
>
> https://whimsy.apache.org/roster/group/asf-secretary
>
>> 4. Could this be called directly from the command line, e.g. ruby
>> views/actions/memstat clr emeritus
>
> Directly? No. But it is quite possible to write a script that sets a
> few instance variables and then reads and evals the script. The tests
> for the board agenda tool do this:
>
> https://github.com/apache/whimsy/blob/master/www/board/agenda/spec/actions_spec.rb#L40
Is it not possible to add a section of the form:
if __FILE__ == $0
@uid = ARGV[0]
@state = ARGV[1]
eval(File.read(__FILE__)) # or some other invocation method
end
This is done in the monitors/*.rb files
> It should be possible to write proper unit tests for this function,
> given dummy test data for members.txt. If this is of interest, I can
> set that up over the weekend, and you could add to it.
>
> The board agenda tests can test for server rendering, and can also
> make use of phantomjs which runs a headless webkit based browser and
> can be used to test javascript.
>
> Tests for pure server functions (without rendering) run very fast.
> When rendering is involved, it is noticeably slower, but not too bad.
> Tests that involve the creation and teardown of a browser are even
> slower, to the point where such tests are generally only worth it for
> non-trivial operations. I wouldn't, for example, be inclined to write
> a test for showing the emeritus/deceased buttons.
>
>> Craig
>>
>> Craig L Russell
>> Secretary, Apache Software Foundation
>> c...@apache.org http://db.apache.org/jdo
>
> - Sam Ruby
