Skip to content
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

4066 - Add Partner User's Name when setting up new users #4072

Merged
merged 26 commits into from
Mar 3, 2024

Conversation

cancelei
Copy link
Contributor

@cancelei cancelei commented Jan 31, 2024

Add name input field in partner show view

Resolves #4066

Description

Please also include relevant motivation and context.
Guide questions:

  • What motivated this change (if not already described in an issue)?
    On a high level: Improve Org user experience when creating a new user, more on that in the issue itself.

  • What alternative solutions did you consider?
    I considered changing the database name default settings that currently apply the "Name Not Provided" when the name is blank on the form. That would involve changing the logic of the controller with the help of the user model (for validations). But I decided to not change it, since there would be a security risk for the database that is in prod.

  • What are the tradeoffs for your solution?
    After merging the solution, new organizational users will be able to activate a new partner user under his umbrella with or without a name. Until now, this process has been done without informing the name. Before and after, Partner Names are also editable from the dashboard.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I implemented tests within the same test files.

Some test files had some pre-existing tests that were not very resilient to the type of changes that were requested in this task. So I expanded the adjustments to pass in the name variable (sometimes called "partner.name" variable), that are needed to continue the user creation process.

Note for the reviewer: I believe that if the solution is good, maybe the tests could improve and I would be happy to receive some feedback to improve.

Screenshots

image

Feature:
image

@cancelei cancelei changed the title 4066 (WIP) - Add Partner User's Name when setting up new users. 4066 - Add Partner User's Name when setting up new users. Feb 2, 2024
@cielf cielf requested a review from dorner February 4, 2024 16:23
cancelei and others added 2 commits February 5, 2024 10:53
Add expected test result.

Co-authored-by: Brock Wilcox <awwaiid@thelackthereof.org>
@cancelei cancelei changed the title 4066 - Add Partner User's Name when setting up new users. 4066 - Add Partner User's Name when setting up new users Feb 5, 2024
@cancelei
Copy link
Contributor Author

cancelei commented Feb 5, 2024

Hello @dorner, I implemented feedback from other reviewers and was also looking for your feedbacks.

dorner
dorner previously requested changes Feb 7, 2024
app/controllers/partners_controller.rb Outdated Show resolved Hide resolved
@cancelei cancelei requested a review from dorner February 19, 2024 16:02
@@ -297,6 +297,7 @@
subject.call
expect(UserInviteService).to have_received(:invite).with(
email: email,
name: partner.name,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch this to use the new generated name

Suggested change
name: partner.name,
name: name,

@cancelei
Copy link
Contributor Author

@awwaiid I believe I have a smooth PR ready for an eye. I reviewed the files and made sure to only leave in what was necessary. Let me know, I can make adjustments till next Sunday for sure.

@cielf cielf requested a review from awwaiid March 1, 2024 14:25
Copy link
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks!!!

@awwaiid awwaiid dismissed dorner’s stale review March 3, 2024 16:07

I think we addressed this through this PR and through the follow-up PR where we fix the in-database default value

@awwaiid awwaiid merged commit fd551b6 into rubyforgood:main Mar 3, 2024
19 checks passed
@cancelei
Copy link
Contributor Author

cancelei commented Mar 4, 2024

Great thanks!!!

Thank you @awwaiid and the Core Team for the support, without it, I wouldn't be so assertive.

Copy link
Contributor

@cancelei: Your PR 4066 - Add Partner User's Name when setting up new users is part of today's Human Essentials production release: 2024.03.17.
Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify the partner-user's name in addition to their email address upon creation
4 participants