-
-
Notifications
You must be signed in to change notification settings - Fork 504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Single step invite and process partner process capability #4075
Add Single step invite and process partner process capability #4075
Conversation
95170f4
to
e3d71ab
Compare
# == Schema Information | ||
# | ||
# Table name: events | ||
# | ||
# id :bigint not null, primary key | ||
# data :jsonb | ||
# event_time :datetime not null | ||
# eventable_type :string | ||
# type :string not null | ||
# created_at :datetime not null | ||
# updated_at :datetime not null | ||
# eventable_id :bigint | ||
# organization_id :bigint | ||
# user_id :bigint | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app added this. Doesn't detract from the change, and I think it's more useful to have. Opting to keep it in.
db/schema.rb
Outdated
@@ -839,7 +839,8 @@ | |||
end | |||
|
|||
create_table "versions", force: :cascade do |t| | |||
t.string "item_type", null: false | |||
t.string "item_type" | |||
t.string "{:null=>false}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I added not null constraint, got this. Don't think it should be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is really weird... maybe it was a mistake that happened while you were developing? Check your database table for versions
and drop this column if it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it since it shouldn't be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great first draft! I had some suggestions/comments.
@@ -97,7 +97,8 @@ def organization_params | |||
:repackage_essentials, :distribute_monthly, | |||
:ndbn_member_id, :enable_child_based_requests, | |||
:enable_individual_requests, :enable_quantity_based_requests, | |||
:ytd_on_distribution_printout, partner_form_fields: [] | |||
:ytd_on_distribution_printout,:use_single_step_invite_and_approve_partner_process , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a mouthful... can we rename it to something smaller, like single_step_partner_invite
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about one_step_partner_invite
. Saw it somewhere mentioned in a previous attempt at the ticket. Flows better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed.
app/helpers/ui_helper.rb
Outdated
@@ -130,6 +130,11 @@ def invite_button_to(link, options = {}, properties = {}) | |||
_link_to link, { icon: "envelope", type: "warning", text: "Invite", size: "xs" }.merge(options), properties | |||
end | |||
|
|||
def invite_and_approve_button_to(link, options = {}, properties = {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need a separate helper here? You should be able to reuse existing helpers and just pass the text and icon into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I changed it to reuse existing helper.
db/schema.rb
Outdated
@@ -839,7 +839,8 @@ | |||
end | |||
|
|||
create_table "versions", force: :cascade do |t| | |||
t.string "item_type", null: false | |||
t.string "item_type" | |||
t.string "{:null=>false}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is really weird... maybe it was a mistake that happened while you were developing? Check your database table for versions
and drop this column if it exists.
|
||
click_on "Save" | ||
expect(page).to have_content("Yes") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a request spec that checks the correct button being shown on the partners page based on the value of the flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
1a7b5c9
to
656b34c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Thanks for the contribution!
@vincent-truong-dev there are some failing checks - can you please address them? |
1b00ce9
to
eb5a841
Compare
Addressed the failing checks, thanks. Do you need to approve the workflows for CI to run again? @dorner |
@vincent-truong-dev Sorry - still some failures! |
e8d5ee7
to
706b010
Compare
706b010
to
2377839
Compare
Ok, it's good now! @dorner |
@vincent-truong-dev now there's a conflict. 😭 |
Conflict addressed. 🤞 @dorner |
Awesome! Thanks for your contribution! |
…rubyforgood#4075)" This reverts commit f8caf36.
@vincent-truong-dev: Your PR |
Closes #3432
Checklist:
Description
Certain banks want to skip the approval process and simply invite and approve an agency in a single step so this PR:
Invite and approve
button to partner agency which invites and approves a partner when clicked.Type of change
How Has This Been Tested?
Ran specs
QA Test
Enabling/disabling the feature
http://localhost:3000/diaper_bank/organization
Use single step invite and approve partner process
to yes if no, or no if yes.**Inviting and approving w/ setting on **
Invite and approve
Invite and approve
Approved
afterwardsWith setting off
Screenshots