-
-
Notifications
You must be signed in to change notification settings - Fork 544
5169 Rework of Getting Started section #5229
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
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -838,5 +838,4 @@ DEPENDENCIES | |||
web-console | |||
webmock (~> 3.25) | |||
|
|||
BUNDLED WITH | |||
2.6.7 | |||
2.6.8 |
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.
unintentional upgrade?
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.
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> | ||
</div> | ||
</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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" %> |
Copilot uses AI. Check for mistakes.
I let CI start running |
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.