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

(Add) Internal notes for staff invitations #4291

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

IonBazan
Copy link
Contributor

Fixes #4045

Adds an internal_note for invitations which will be used to create an internal note when user registers.

@IonBazan IonBazan changed the title Add internal note for staff invitations (Add) Internal notes for staff invitations Oct 30, 2024
@HDVinnie HDVinnie changed the base branch from master to 8.x.x October 30, 2024 11:16
@HDVinnie HDVinnie requested a review from Roardom October 30, 2024 11:41
Copy link
Collaborator

@Roardom Roardom left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -93,10 +93,18 @@ public function create(array $input): RedirectResponse | User
$user->emailUpdates()->create();

if (config('other.invite-only') === true) {
Invite::where('code', '=', $input['code'])->update([
$invite = Invite::where('code', '=', $input['code'])->withoutTrashed()->first();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't need to do ->withoutTrashed() here. It's only included above because soft deletion isn't considered for Rule::exists() validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I wasn't sure why is it used above so made it explicit just in case. Fixed in my next commit.

<textarea
id="internal_note"
class="form__textarea"
name="internal_note"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might need to add placeholder=" " here for the autoexpand to work in browsers that don't yet support field-sizing: content as well as to get the label to float.

@IonBazan
Copy link
Contributor Author

@Roardom thanks for the review! I chose a simple task for my first contribution in this repository but I'll try to help with more improvements 🚀

I wasn't sure if CreateNewUser is tested anywhere - it would be good to verify it's working as expected after my changes but I'm quite new to Fortify so I might need some guidance there.

@IonBazan IonBazan requested a review from Roardom October 30, 2024 16:05
@Roardom
Copy link
Collaborator

Roardom commented Oct 31, 2024

@Roardom thanks for the review! I chose a simple task for my first contribution in this repository but I'll try to help with more improvements 🚀

I wasn't sure if CreateNewUser is tested anywhere - it would be good to verify it's working as expected after my changes but I'm quite new to Fortify so I might need some guidance there.

Thanks for the PR! If I'm going to be honest, UNIT3D is lacking a lot of proper testing. There shouldn't be too many issues with the code though as it follows the same logic as the other items also created upon user creation. All of the attributes are mass assignable and there are no database fields without a default that are omitted. The field is properly validated to only be filled by staff and only shown to staff 👍 . But, I've been known to be wrong before.

Regarding Fortify, we've personally had a terrible experience with it and I definitely wouldn't recommend using it if you had a choice. Main reason is all the workarounds we've had to add to it to increase its security (lack of rate limits, timing attacks with emails, user enumeration) and handle our logic with account invitations as well as disabled and banned accounts. We'll probably migrate away from it at some point.

But basically, when a user registers using the form which posts to fortify's register route, it will call Fortify's RegisteredUserController::store which has the CreateNewUser dependency injected and calls the create() method.

With regards to testing it, we never had our tests migrated over when switching from laravel/ui. If you wish, you could add a new test covering it, but it'd be beyond the scope of this PR.

Copy link
Collaborator

@Roardom Roardom left a comment

Choose a reason for hiding this comment

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

Oh, I see you've added the Fortify tests since I last checked the PR last night. Thank you!

@HDVinnie
Copy link
Collaborator

Thank you very much for the contribution. 🖖

@HDVinnie HDVinnie merged commit 65000a2 into HDInnovations:8.x.x Oct 31, 2024
5 checks passed
@IonBazan IonBazan deleted the feature/invite-internal-note branch October 31, 2024 12:01
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.

[Request] Apply note to invite
3 participants