FFS-3566: Advance through /add_job in activity hub #1236
Conversation
bencalegari
force-pushed
the
bc/income-ffs-3566
branch
from
4746914 to
cc6f396
Compare
bencalegari
changed the title
bencalegari
force-pushed
the
bc/income-ffs-3566
branch
from
9ce3130 to
774393f
Compare
tdooner
left a comment
tdooner
left a comment
There was a problem hiding this comment.
Looking good. Some early feedback.
| end | ||
| end | ||
|
|
||
| def find_flow(user_id) # TODO: Find a better way to determine flow type, maybe inherited controllers? |
There was a problem hiding this comment.
Hmm, yeah, I see the problem. Not sure a better option unless we broke out a new table for argyle_users that had polymorphic foreign keys back to the flow (i.e. flow_id, flow_type). Seems a bit heavy here, I think this is ok because these argyle_user_ids are UUIDs and should never collide between the CbvFlow and ActivityFlows.
|
|
||
| def set_flow_session(flow_id, type) | ||
| session[:flow_id] = flow_id | ||
| session[:flow_id] = flow_id |
There was a problem hiding this comment.
It never hurts to set a variable twice just to make sure 🙃
| def flow_class(symbol = nil) | ||
| (symbol || session[:flow_type]&.to_sym) == :activity ? ActivityFlow : CbvFlow |
There was a problem hiding this comment.
nit: symbol and flow_param are not great names to me since ideally they would convey a bit more meaning. I think symbol is actually something like flow_type.
| def flow_class(symbol = nil) | |
| (symbol || session[:flow_type]&.to_sym) == :activity ? ActivityFlow : CbvFlow | |
| def flow_class(flow_type = session[:flow_type]) | |
| flow_type.to_sym == :activity ? ActivityFlow : CbvFlow |
| <% if activity_type_enabled?(:income) %> | ||
| <section> | ||
| <%= button_to t("activities.income.add"), activities_flow_income_employer_search_path, class: "btn btn-primary usa-button margin-y-5", method: :get %> | ||
| </section> |
There was a problem hiding this comment.
For your own sanity (avoiding merge conflicts) I recommend splitting these reformattings into a separate PR.
| end | ||
|
|
||
| def invitation_id | ||
| activity_flow_invitation_id |
There was a problem hiding this comment.
nit: I think this could be defined via the belongs_to by adding an as: :invitation option.
Ticket
FFS-3566
Context for reviewers
/activites/income/employer_searchto/cbv/<path>. We should have unique paths here. I'm getting an itch that there's probably a refactor to be done to create distinct controllers that inherit from the same one a la FlowController.Acceptance testing