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 ability to join team #11810

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Add ability to join team #11810

wants to merge 13 commits into from

Conversation

nanaya
Copy link
Collaborator

@nanaya nanaya commented Jan 22, 2025

I think it's in relatively good state now 🤔

  • better notification message and everything else

@nanaya nanaya marked this pull request as draft January 22, 2025 14:12
@nanaya nanaya force-pushed the member-join branch 3 times, most recently from 4843c7d to be105db Compare January 23, 2025 13:34
@nanaya nanaya marked this pull request as ready for review January 23, 2025 13:35
@notbakaneko
Copy link
Collaborator

I think the team leader needs some kind of indicator that there are pending join requests because you don't see anything unless you go into the member management page...

app/Models/User.php Show resolved Hide resolved
resources/views/teams/show.blade.php Outdated Show resolved Hide resolved
Comment on lines 1915 to 1920
if ($team->leader_id !== $user->getKey()) {
return null;
}
if ($team->emptySlots() < 1) {
return 'team.member.store.full';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's this new trend of compressing control statement blocks? ಠ_ಠ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤷

app/Jobs/Notifications/TeamApplicationAccept.php Outdated Show resolved Hide resolved
Comment on lines 29 to 32
$team = Team::findOrFail($teamId);
$application = $team->applications()->findOrFail($id);

priv_check('TeamApplicationAccept', $application)->ensureCan();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this needs to be part of the transaction since there are no read locks, nor $member being returned if TeamApplicationAccept uses TeamApplication instead of TeamMember.

resources/views/teams/show.blade.php Outdated Show resolved Hide resolved
@nanaya nanaya mentioned this pull request Feb 3, 2025
2 tasks
Comment on lines +197 to +201
'already_member' => "You're already part of the team.",
'already_other_member' => "You're already part of a different team.",
'currently_applying' => 'You have pending team join request.',
'team_closed' => 'The team is currently not accepting any join requests.',
'team_full' => "The team is full and can't accept any more members.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I forgot to mention, a (long) while ago it was discussed about always using single quotes for all the localization files and escaping as necessary? I forget the specific reason for going with single quotes only @peppy

nanaya and others added 2 commits February 4, 2025 15:50
There isn't any verification popup handler for turbo yet.
return null;
}
if ($team->emptySlots() < 1) {
return 'team.member.store.full';
Copy link
Collaborator

Choose a reason for hiding this comment

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

wrong/missing key

Also not sure if admin should be skipping the team slots check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's fine for admin to be able to skip that 🤔 it's not like some breaking/validation thing (and it's not even a static number in the first place)

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

Successfully merging this pull request may close these issues.

2 participants