Load cardlib #46

Merged
merged 4 commits into from Jan 11, 2013

Conversation

Projects
None yet
2 participants
Collaborator

GerryG commented Dec 27, 2012

Now that reference_types is in, most of this list is already in, but it is a nice list, so I will * the items still on this branch uniquely. Oh, maybe just one item ...

It is working really well, but I just ran into the issues with running cucs. We can begin to review this while figuring out the test configuration issues.

Change sets included here:
0) fix_tests (missing from develop still)

  1. Transclude => Include
  2. wagn/model => cardlib rename, but that is split off too (model_cardlib)
  3. errors => render_errors
    1. loading changes (preambles and sets.rb mainly) This also better supports action_events lines of development to follow soon
    2. reference related refactoring to improve the algorithms and use the card cache as needed too.
    3. some user/account related things that move more towards the complete card based accounts (mostly session related)
    4. dependents refactor (note caching discussion needs some resolution)
    5. ruby_initializers cleanup
      a) if_card removed
      b) Array#except removed
      c) plot replaced with map
      d) send_if added
    6. fetch_or_new to fetch :new=>{args} completion
Collaborator

GerryG commented Dec 27, 2012

Line count is a bit high, hope that is mostly because of file moves. Either way, I tried to keep out stuff that would make this more complex. The most complex part is making the loading work right when this is merged forward with reference_types and action_events lines of development. That's the core of this branch.

Gemfile
@@ -62,7 +62,7 @@ group :test, :development do
end
group :test do
- gem 'cucumber-rails', '~> 1.3' # feature-driven-development suite
+ gem 'cucumber-rails', '~> 1.3', :require=>false # feature-driven-development suite
@GerryG

GerryG Dec 28, 2012

Collaborator

This is part of fix_tests, your merge must have went wrong somehow.

- end
-
- end
-end
@GerryG

GerryG Dec 28, 2012

Collaborator

Oops, add this back ...

+end
+
+class ApplicationController
+ include Wagn::Exceptions
@ethn

ethn Dec 29, 2012

Collaborator

why close and reopen?

@GerryG

GerryG Dec 30, 2012

Collaborator

In a few cases, I found we needed to define the class and it's parent before loading other things (typically automatically based on constant loading). In this case, I think we want all the stuff that the 'Card' reference above does complete first before doing any other loading.

It may also be that this is no longer needed with the re-org in sets.rb.

app/controllers/card_controller.rb
+ #include Wagn::Sets::CardControllerMethods
+
+class CardController
+
@ethn

ethn Dec 29, 2012

Collaborator

above seems unnecessary

@GerryG

GerryG Dec 30, 2012

Collaborator

Maybe, same reply as above. I will circle back and see if any of this is needed once everything is actually working. I may end up pulling/changing it anyway.

app/controllers/account_controller.rb
+class AccountController < CardController
+ Card
+ Card::Reference
+
@GerryG

GerryG Dec 30, 2012

Collaborator

I'm going to circle back a recheck what this pre-able stuff does, including the re-openning of some classes. I'm going to get everything working again first, then circle back.

app/controllers/account_controller.rb
@@ -86,7 +93,8 @@ def signin
end
def signout
- self.session_user = nil
+ #self.session_user = nil
+ self.session_user = Card::AnonID
@GerryG

GerryG Dec 30, 2012

Collaborator

session_user is supposed to be a card now. I think it is still nil in the 'session' (session[:user])

@ethn

ethn Jan 9, 2013

Collaborator

may be right. but shouldn't be changed im huge PR

app/controllers/card_controller.rb
opts[:name] ||= name
Card.new opts
else
- Card.fetch_or_new name, opts
+ name = $1.to_i if name =~ /^~(\d+)$/
@GerryG

GerryG Dec 30, 2012

Collaborator

It seems odd this is being added. I think this is the external representation we want for numeric (card_id) access, right?

@GerryG

GerryG Jan 9, 2013

Collaborator

D'oh.

Sorry, fixing.

lib/old_modules/flexmail.rb
+ Account.as( fld_card.updater ) do
+ list = fld_card.extended_list card
+ field == :attach ? list : list * ','
+ end
@GerryG

GerryG Dec 30, 2012

Collaborator

if_card removed

Collaborator

ethn commented Dec 30, 2012

closing: too large

@ethn ethn closed this Dec 30, 2012

Collaborator

ethn commented Dec 31, 2012

reopening (as part of experiment w Gerry)

@ethn ethn reopened this Dec 31, 2012

app/controllers/account_controller.rb
@@ -1,10 +1,11 @@
# -*- encoding : utf-8 -*-
class InvitationError < StandardError; end
-class AccountController < ApplicationController
+class AccountController < CardController
@GerryG

GerryG Dec 31, 2012

Collaborator

This is really the first step in removing these extra controllers, they are now extending CardController. This simplifies loading too, but then we are factoring this code out soon anyway.

+require_dependency 'card'
+
+class AdminController < CardController
@GerryG

GerryG Dec 31, 2012

Collaborator

Oh, I may need the require_dependency in the account_controller if I delete those lines.

@ethn

ethn Jan 11, 2013

Collaborator

I don't follow. do I need to?

@GerryG

GerryG Jan 11, 2013

Collaborator

Not yet, but when all this loading is being reviewed and tightened up. But by then there won't be an AdminController anyway.

@GerryG

GerryG Jan 11, 2013

Collaborator

All this crazyness will be gone soon. Big progress on action_events branch on this.

lib/authenticated_system.rb
+ new_user = Card==new_user ? card.id : new_user
+ Account.user = new_user
+ new_user = nil if new_user == Card::AnonID
+ @session_user = session[:user] = new_user
@GerryG

GerryG Dec 31, 2012

Collaborator

This doesn't seem quite right yet. I think session[:user] needs to be nil, but @session_user should be the card still.

@ethn

ethn Dec 31, 2012

Collaborator

I don't want to change this logic in a PR this massive. #holdon

@GerryG

GerryG Jan 9, 2013

Collaborator

Ok, then maybe just for clarity, I should make @session_user => @session_card_id ?

I think we already have the change that changes this from being users.id to cards.id, and the nil/Anon should be handled the old way.

Does that match what you think should be done with this?

@ethn

ethn Jan 9, 2013

Collaborator

no idea. point is not to change amid massive PR.

@GerryG

GerryG Jan 9, 2013

Collaborator

Well, I will post that soon, I think it is what you are asking for here.

lib/cardlib.rb
- base.send :include, Cardlib.const_get(const)
+ Cardlib.constants.each do |const|
+ Rails.logger.warn "consts #{const}, #{base}"
+ base.send :include, Cardlib.const_get( const )
@GerryG

GerryG Dec 31, 2012

Collaborator

The re-ordering here and in sets is pretty central to how it all works right.

@ethn

ethn Jan 11, 2013

Collaborator

I haven't studied this yet. What is it you're going for with this refactor?

@GerryG

GerryG Jan 11, 2013

Collaborator

Yes. I was identifying some things in action_events that I would need, but in the interest of smaller commits I was trying to just get mostly the loading stuff. There were al lot of smaller changes that I pulled from reference_types into this branch because I thought we might want to do that first (the migration was still holding us up).

Since that smaller stuff was always in both branches, we have merged most of it already.

Then there is a bit more in the action_* branches. I've got two of those active now, but I haven't quite figured out all the pieces yet. These are now downstream of obj_render, so I want to get that in first. (action_reference and action_events are the two branches).

-
+ if mark.nil?
+ return if opts[:new].nil?
+ # This is fetch_or_new now when you supply :new=>{opts}
@GerryG

GerryG Dec 31, 2012

Collaborator

This change looks simpler with diff -w. Basically, we are skipping the db/cache load of the existing card if there is no mark. There would only be no mark for certain fetch_or_new cases, so :new=>{opts} has to be there.

-require_dependency 'chunks/literal'
-require_dependency 'chunks/reference'
-require_dependency 'chunks/link'
-require_dependency 'chunks/include'
@GerryG

GerryG Dec 31, 2012

Collaborator

chunk_manager will be gone soon, but I thought it good to first organize the chunks under the chunks/chunk.rb file (which defines Chunk::Abstract, not Chunks::Chunk as you might expect). So this is a small move towards more standard usage patterns.

@@ -1,3 +1,13 @@
+
+class Chunks::Abstract
+end
@GerryG

GerryG Dec 31, 2012

Collaborator

This needs to be defined before it tries to load any of the other chunks. We should probably rename this file to abstract.rb, and put the requires all in lib/chunks.rb. Then we just reference Chunks or load with: require 'chunks'

lib/wagn/set/all/rss.rb
+ module Set
+ module All
+ module Rss
+ include Sets
@GerryG

GerryG Dec 31, 2012

Collaborator

Now I'm not even sure if this is needed. Should I try to see if it is still?

lib/wagn/set/right/template_right.rb
+ end
+ end
+ end
+end
@GerryG

GerryG Dec 31, 2012

Collaborator

Did I add this?

@GerryG

GerryG Jan 9, 2013

Collaborator

Does this seem like it is really part of setting_groups? I need to figure out why this is here or take it out.

@GerryG

GerryG Jan 9, 2013

Collaborator

Hmmm, no, that's not it.

@GerryG

GerryG Jan 9, 2013

Collaborator

This should now be gone

+ module ClassMethods
+ end
+
+ def self.included base
@GerryG

GerryG Dec 31, 2012

Collaborator

I think this has the right basic structure and we can make it nicer from here.

Collaborator

ethn commented Dec 31, 2012

this is still too large, of course. I only opened it so you could close it.

@@ -1,4 +1,8 @@
# -*- encoding : utf-8 -*-
+
+require_dependency 'wagn/sets'
+require_dependency 'card'
@ethn

ethn Dec 31, 2012

Collaborator

doesn't line 4 handle line 3?

@GerryG

GerryG Jan 9, 2013

Collaborator

I think you may be right, but let's trim away at that sort of thing after the rest in where we want it?

In other words, with it fully working, we can drop what we think may be extra, one thing at a time and test each. All before merging this ...

@ethn

ethn Jan 11, 2013

Collaborator

what broke it all? the cardlib loading?

@GerryG

GerryG Jan 11, 2013

Collaborator

It is all about the right order loading for 1) the view/event declaration methods that are needed for 2) loading the Sets.

the cardlib stuff is loaded first, and it has to be fully loaded before we actually instantiate any cards, or stuff will still be missing when it does the include of the cardlib modules down in the middle of Card.

I thought I had it all working still in the action_* branches, but there is still a little bit of funny business around that. That's why I was asking about where you really want the events to be defined. I was having trouble making it CardController because it would immediately load Card too soon.

@@ -10,7 +11,7 @@ class Card < ActiveRecord::Base
SmartName.lookup= Card
SmartName.session= proc { Account.as_card.name }
-class Card < ActiveRecord::Base
+class Card
@ethn

ethn Dec 31, 2012

Collaborator

I would think this is the canonical Card definition, so the active record inheritance should be established here.

@GerryG

GerryG Jan 9, 2013

Collaborator

It is at line 3, so it is optional here. I can add it back if you think it looks better that way.

@ethn

ethn Jan 11, 2013

Collaborator

ah. right. nevermind. What I really want to do is accomplish all of this without closing and re-opening. can do that later.

@GerryG

GerryG Jan 11, 2013

Collaborator

Yes, best left until the event declarations are more settled as that may affect this code too.

- super
- Wagn::Sets.load
+ Card
+
@ethn

ethn Dec 31, 2012

Collaborator

every use of this trick is a kludge. we should be eliminating it; not expanding it.

@GerryG

GerryG Jan 9, 2013

Collaborator

It is all about load order. I think once we understand that fully, we can clean all that up. That is what I most want to get right in this branch. We will need this for the action_event stuff soon.

@GerryG

GerryG Jan 11, 2013

Collaborator

As commented above, I think I figured this out on the action_events branch. We should be able to pull forward most of that, or maybe all of it. With other work factored out, I think it only adds the base event definitions (crud plus)

Collaborator

GerryG commented Jan 6, 2013

I'll close for now, but I think this is pretty close once anything missing from develop in merged in.

@GerryG GerryG closed this Jan 6, 2013

Collaborator

GerryG commented Jan 6, 2013

Oh, wait, wrong PR

@GerryG GerryG reopened this Jan 6, 2013

Gemfile
@@ -48,7 +48,8 @@ group :assets do
gem 'tinymce-rails', '~> 3.4' # wysiwyg editor
- gem 'therubyracer' # execjs is necessary for developing coffeescript. mac users have execjs built-in; don't need this one
+ # execjs is necessary for developing coffeescript. mac users have execjs built-in; don't need this one
+ gem 'therubyrhino', :platform=>:ruby # :ruby is MRI rubies, so if you use a mac ruby ...
end
@ethn

ethn Jan 9, 2013

Collaborator

I don't understand this comment. (or why we're adding new commits to this bloated PR)

@GerryG

GerryG Jan 9, 2013

Collaborator

As I've always said, anything you want in another request, just say so. It just makes it easier if I don't have to stash these Gemfile changes. Do they conflict with anything for you?

lib/wagn/set/self/head_and_foot.rb
+ module Set
+ module Self
+ module HeadAndFoot
+ include Sets
@GerryG

GerryG Jan 9, 2013

Collaborator

All the whitespace changes in here really add to the line count, and I'm not even sure it was needed now.

Should I see if I can revert that part and have it still work?

Collaborator

GerryG commented Jan 9, 2013

That really reduces the line count. It is still ATP and I testing setup, admin and account functions on the test server, no problems.

So we don't need the nesting in sets, but I still have that on a branch (nested) if we do want to add it after this one. I don't see why now, your call.

Collaborator

GerryG commented Jan 9, 2013

Now at: Showing 34 changed files with 430 additions and 264 deletions.

Collaborator

GerryG commented Jan 9, 2013

I think I have responded to any holds, let me know if there are any new ones.

+ mock(Mailer).change_notice( @ja_id, @ulyss, "updated", 'Book' , [[name, "added"]], is_a(Card))
+ mock(Mailer).change_notice( @jc_id, @ulyss, "updated", @ulyss.name , [[name, "added"]], is_a(Card))
+ c=Card.create :name=>name, :content => "James Joyce"
+ end
@GerryG

GerryG Jan 9, 2013

Collaborator

I added these cases to cover some things that were first breaking in cucs. Now fixed, but tests are good.

@ethn

ethn Jan 11, 2013

Collaborator

woooo!

app/controllers/account_controller.rb
@@ -127,7 +129,7 @@ def user_errors
end
def password_authentication(login, password)
- if self.session_user = User.authenticate( params[:login], params[:password] )
+ if self.session = User.authenticate( params[:login], params[:password] )
@ethn

ethn Jan 11, 2013

Collaborator

hmm, doesn't this mean that in the context of a controller, session is one thing ( a hash) and self.session is another (a user object). that seems highly confusing.

@GerryG

GerryG Jan 11, 2013

Collaborator

I don't think I changed that, just the var names. self.session (or session_user or session_account) is just a attribute defined on the controllers. We probably could get rid of it without much issue. session[] is the real thing, session[:user] from the request/user session.

We'll just need to agree on the best name for the attribute.

@ethn

ethn Jan 11, 2013

Collaborator

#holdon

@ethn

ethn Jan 11, 2013

Collaborator

the name change is what made this messy. I'm not attached to self.session_user, but self.session won't do. too close to "session".

@@ -1,7 +1,6 @@
# -*- encoding : utf-8 -*-
-class Card
-
+class Card < ActiveRecord::Base
@ethn

ethn Jan 11, 2013

Collaborator

curious as to whether this change is needed?

@ethn

ethn Jan 11, 2013

Collaborator

what are the contexts where we currently need to test loading issues? tests, browser (2 loads), rake wagn:create...

@GerryG

GerryG Jan 11, 2013

Collaborator

A big issue was using require_dependency vs. require. A lot of the trying this and that happened before you pointed that out to me.

lib/authenticated_system.rb
- def session_user
- @session_user ||= session[:user]
+ def session_id
+ @session_id ||= session[:user]
@ethn

ethn Jan 11, 2013

Collaborator

this seems confused. is it an id or a user object?

@GerryG

GerryG Jan 11, 2013

Collaborator

the session variable and the session[:user] value are ids. When you assign, you can use either:

self.session = Card['Gerry']
or Card['Gerry'].id

In old code, this was all the User object and it's id

@ethn

ethn Jan 11, 2013

Collaborator

ok. session[:user_id] would be better, but I guess that would break existing sessions...

@ethn

ethn Jan 11, 2013

Collaborator

so, in general, do we refer to Card['Gerry'].id as the "user_id" and Card['Gerry+*account'].id as the account_id?

@ethn

ethn Jan 11, 2013

Collaborator

session_id is not a great name either, since the sessions table has an id field and a session_id field, and this is neither. I think my inclination is to be explicit and call it session_user_id. I would say we should do it as a simple accessor and just assign it as an id every time. so self.session, self.session_user, and self.session_id could all go away, right?

@GerryG

GerryG Jan 11, 2013

Collaborator

it is user_card_id, really, because we want to be clear when it is no longer users.id

I vote for @session_card_id and later @session_account_id when it is the +*account card. Although, I don't think I would make that change.

@ethn

ethn Jan 11, 2013

Collaborator

hmm, I guess it can't be a simple accessor since it has to write to the session object, but still there's no need to handle card objects. and at some point we should just break sessions and call it "user_id" in the hash. so self.session_user_id = session[:user_id].

@@ -190,8 +190,7 @@ def set_read_rule
if !new_card? && updates.for(:type_id)
Account.as_bot do
Card.search(:left=>self.name).each do |plus_card|
- plus_card = plus_card.refresh
- plus_card.update_read_rule
+ plus_card = plus_card.refresh.update_read_rule
@ethn

ethn Jan 11, 2013

Collaborator

don't need the plus_card =, right?

@GerryG

GerryG Jan 11, 2013

Collaborator

No, I think you do. plus_card.refresh will return self, or a new card that it creates for the refresh.

Oh, wait, that's just a local var, right? Then no. If you use it again, you'd have to save it.

+ @trunk_watcher_watched_pairs.each do |watcher, watched|
+ #warn "wp tw #{watcher.inspect}, #{watched.inspect}"
+ next if watcher.nil?
+ Mailer.change_notice( watcher, self.left, 'updated', watched.to_s, [[name, action]], self ).send_if :deliver
end
@ethn

ethn Jan 11, 2013

Collaborator

what prompted these changes? why no compact? why send_if?

+ tcard and pairs = tcard.watcher_watched_pairs
+ #warn "trunk_watcher_pairs TC:#{tcard.inspect}, #{tname}, P:#{pairs.inspect}, k:#{tname.key} member: pr:#{!pairs.nil?}, and I:#{includers.map(&:key).member?(tname.key)}"
+ return pairs if !pairs.nil? and includers.map(&:key).member?(tname.key)
+ #warn "twatch empty ..."
end
@ethn

ethn Jan 11, 2013

Collaborator

above seems messier than older code.

@GerryG

GerryG Jan 11, 2013

Collaborator

I don't like either version that much. We can revert it or make it better if you want.

+
+ else []
+ end
+ #warn "wp r:#{r}"; r
end
end
@ethn

ethn Jan 11, 2013

Collaborator

I don't understand motivation for above changes.

@GerryG

GerryG Jan 11, 2013

Collaborator

I think this was all because I thought I was getting extra emails for some updates. I may have had another underlying problem that I was fixing. The essence of this update is removing duplicates from the output id-list.

- include LocationHelper
+
+ cattr_accessor :current_slot, :ajax_call, :perms, :denial_views, :subset_views, :error_codes, :view_tags, :renderer
+ @@renderer = Renderer
@ethn

ethn Jan 11, 2013

Collaborator

class var set to class??

@GerryG

GerryG Jan 11, 2013

Collaborator

That's the var that is changed when you do: format :html

This is the default value, the base renderer.

@ethn

ethn Jan 11, 2013

Collaborator

seems like perhaps current_slot should be current_renderer, and renderer should be current_class?

@@ -49,7 +49,7 @@ group :assets do
gem 'tinymce-rails', '~> 3.4' # wysiwyg editor
# execjs is necessary for developing coffeescript. mac users have execjs built-in; don't need this one
- gem 'therubyrhino', :platform=>:ruby
+ gem 'therubyrhino', :platform=>:ruby # :ruby is MRI rubies, so if you use a mac ruby ...
end
@GerryG

GerryG Jan 11, 2013

Collaborator

I need your input about the two asset files below. I thought they were left over after a merge mistake, but it seems they are in develop, so I should put them back, right?

@ethn

ethn Jan 11, 2013

Collaborator

I suppose we should put them back. we may not need them, but I'd rather not have to figure that out just yet

@GerryG

GerryG Jan 11, 2013

Collaborator

@ethn ethn merged commit 30d9bb6 into wagn:develop Jan 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment