Skip to content

Membership Approval#145

Merged
dpslwk merged 21 commits intomasterfrom
approval
May 28, 2017
Merged

Membership Approval#145
dpslwk merged 21 commits intomasterfrom
approval

Conversation

@dpslwk
Copy link
Member

@dpslwk dpslwk commented May 12, 2017

finally close #111

This wil take users form approval to awaiting payment stage, notifying the membership team about the need to review and approve details

dpslwk added 10 commits May 12, 2017 08:12
So larval’s mail system is tightly coupled to eloquent models and as such expects public name and email properties available on the an model that is pass to Mail::to($model)
Declaring email public is an easy switch but early on in dev we renamed the name column to first name,
so rather than changing the DB back I’ve use the mapping to override it in the entity
Im also leaving the Firstname getters and setter in place
@dpslwk dpslwk added this to the Base System milestone May 12, 2017
@dpslwk
Copy link
Member Author

dpslwk commented May 13, 2017

last commit will also close #143

@dpslwk dpslwk force-pushed the approval branch 3 times, most recently from 7a08f4e to 37ff3f8 Compare May 15, 2017 09:58
@dpslwk dpslwk force-pushed the approval branch 2 times, most recently from 85e25f3 to 137e70a Compare May 15, 2017 12:13
Add `TRUSTEE_SLACK_WEBHOOK` & `TEAM_SLACK_WEBHOOK` to your .env to test (with suitable incoming web hook url’s)
if ($this->name = 'team.Trustees') {
return Meta::get('trustee_slack_webhook');
if ($this->name == self::TEAM_TRUSTEES) {
return env('TRUSTEE_SLACK_WEBHOOK', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do this as a configuration parameter? Pulling from the same env var?

* @return array
*/
public function searchLike(string $searchQuery)
public function searchLike(string $searchQuery, ?bool $hasAccount = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nullable bool?

Is this because some callers might not pass a value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm calling with $this->userRepository->searchLike($searchQuery, $request->input('withAccount', false))
and $request->input can return null

* @param Illuminate\Http\Request $request
* @return User
*/
public function updateUserProfileFromRequest(User $user, Request $request)
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this request been sanitized before it gets here? I forget laravels functionings.

return redirect()->route('home');
}

$this->validate($request, [
Copy link
Contributor

Choose a reason for hiding this comment

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

The answer is yes :)


namespace App\Http\Controllers;

use HMS\Entities\Role;
Copy link
Contributor

Choose a reason for hiding this comment

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

As an aside the number of includes here tells me this is quite the fat controller. Might be worth figuring out how we can offload some of this stuff to some service classes.

'reason' => 'required|string|max:500',
]);

\Mail::to($user)->send(new MembershipDetailsRejected($user, $request['reason']));
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

$membershipTeamRole = $this->roleRepository->findByName(Role::TEAM_MEMBERSHIP);
$membershipTeamRole->notify(new NewMemberApprovalNeeded($user, true));

flash('Your detials have been updated and another review requested, thank you.')->success();
Copy link
Contributor

Choose a reason for hiding this comment

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

'detials' typo

@@ -27,6 +40,6 @@ public function __construct()
public function handle(MembershipInterestRegistered $event)
{
\Mail::to($event->invite->getEmail())
Copy link
Contributor

Choose a reason for hiding this comment

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

ALready using listeners to mail for some things.


<form role="form" method="POST" action="{{ route('membership.update', $user->getId()) }}">
{{ csrf_field() }}
{{ method_field('PUT') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why put from a POST form?


@section('content')
<h1>New member detials review</h1>
<p>Please review the details below and check they are all sane.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe 'sane' isn't the most PC term.

"Please review the details below to check they are all correct"?

Copy link
Contributor

@cooperaj cooperaj left a comment

Choose a reason for hiding this comment

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

👍

# Conflicts:
#	app/HMS/Entities/User.php
@dpslwk dpslwk merged commit 5dafa30 into master May 28, 2017
@dpslwk dpslwk deleted the approval branch May 28, 2017 14:41
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.

Regsitration

2 participants