5169 Rework of Getting Started section #5229
Conversation
|
If it's preferable for me to resolve these conflicts I am willing to do so. |
|
They look pretty straight forward to me - please go ahead. |
|
@cielf If there's anything I need to change, please let me know! |
There was a problem hiding this comment.
Hey @Budmin -- thank you for this ! i think it's going to be more intuitive for the new banks.
Just to let you know our process -- I do the functional review, then, once everything seems to work fine, I pass it on to a senior dev for technical comments.
There's 1 thing that has to be changed, plus a couple of nice to haves that became evident when I tried it out.
Needed:
1/ When you click "Dismiss", it's currently going to the organization -- it should stay on the dashboard.
Desired:
Most of the user tasks are pretty straight-forward, but a couple of them will be easier if the user reads the appropriate user guide section. Let's make that easy for them.
1/ beside "Audits", add a link titled: (User guide) pointing to https://rubyforgood.github.io/human-essentials/user_guide/bank/getting_started_inventory.html
2/ beside "Customize your organization", add a link titled (User guide) pointing to https://rubyforgood.github.io/human-essentials/user_guide/bank/getting_started_customization.html
3/ This is a very small semantic difference, but I think we should change "Add custom items" to "Add any custom items".
Your changes to the user guide look good. We'll need to revise the entire "Getting started" section to make it flow more like this, but I think that's out of scope of this PR.
Thank you.
Gemfile.lock
Outdated
|
|
||
| BUNDLED WITH | ||
| 2.6.7 | ||
| 2.6.8 |
There was a problem hiding this comment.
Yes, this change was just a difference in my bundle version that automatically made this change.
As the project is now on 2.6.9, I've made the changes to use 2.6.9 for this.
There was a problem hiding this comment.
Pull Request Overview
This PR reworks the "Getting Started" user workflow by consolidating resource links into one section and introducing a new flag (bank_is_set_up) to track when an organization has completed the setup. Key changes include updating tests, modifying documentation links and text, and revising view templates and controller strong parameters to incorporate the new bank_is_set_up flag.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/system/dashboard_system_spec.rb | Updated functional test to check guide visibility based on bank_is_set_up flag |
| spec/models/organization_spec.rb and spec/factories/organizations.rb | Updated documentation comments for new bank_is_set_up column |
| docs/user_guide/bank/* | Modified text and image links in getting started user guide |
| db/schema.rb and db/migrate/20250513012435_add_bank_is_set_up.rb | Added bank_is_set_up column with migration and schema changes |
| app/views/dashboard/index.html.erb | Changed partial render locals to use bank_is_set_up flag |
| app/views/dashboard/_getting_started_prompt.html.erb | Reworked guide display logic and added a dismiss button |
| app/controllers/organizations_controller.rb, dashboard_controller.rb, admin/organizations_controller.rb | Updated strong parameters and instance variable assignment |
Comments suppressed due to low confidence (1)
app/controllers/dashboard_controller.rb:6
- For clarity and consistency with the model attribute, consider renaming @org_is_set_up to @bank_is_set_up.
@org_is_set_up = current_organization.bank_is_set_up
| </div> | ||
|
|
||
| <p>When you're done, click this box to hide this section</p> | ||
| <%= button_to "Dismiss", "/manage", method: :patch, params: {organization: { bank_is_set_up: true }}, class: "btn btn-primary" %> |
There was a problem hiding this comment.
Consider using a route helper in place of hardcoding the '/manage' path to improve maintainability and clarity of intent.
| <%= button_to "Dismiss", "/manage", method: :patch, params: {organization: { bank_is_set_up: true }}, class: "btn btn-primary" %> | |
| <%= button_to "Dismiss", manage_path, method: :patch, params: {organization: { bank_is_set_up: true }}, class: "btn btn-primary" %> |
|
I let CI start running |
compwron
changed the title
|
@Budmin Are you still working on this? It's been a month since the initial review. |
|
@cielf Yes, sorry for the delay. I'll be submitting an update to this shortly. |
added links to user guide, small language tweaks, and linting fixes
…in/human-essentials into 5169_getting_started_rework
|
@cielf This should include the needed change, and all of the desired changes you specified. If there's anything else I can do please let me know. |
|
@dorner -this is passing functional QA - could you take a look for technical concerns? |
dorner
left a comment
dorner
left a comment
There was a problem hiding this comment.
One question, otherwise looks good to me.
| def organization_params | ||
| params.require(:organization) | ||
| .permit(:name, :street, :city, :state, :zipcode, :email, :url, :logo, :intake_location, :default_email_text, :account_request_id, :reminder_day, :deadline_day, | ||
| .permit(:name, :short_name, :street, :city, :state, :zipcode, :email, :url, :logo, :intake_location, :default_email_text, :account_request_id, :reminder_day, :deadline_day, :bank_is_set_up, |
There was a problem hiding this comment.
I think short_name was removed, no? Can you make sure your branch is up to date with main?
|
Thanks so much! |
|
@Budmin: Your PR |
4 participants
Resolves #5169
Description
Type of change
(Calling this a feature is a bit much, but it is technically changing functionality.)
How Has This Been Tested?
As there was already a test that verified the previous "Getting Started" section. This test was modified to test for the new workflow. No configuration changes should be necessary to run this updated test.