Adding a resident-extern role #358

viktorcsimma · 2023-10-28T14:52:41Z

Closes #347. It's largely done; there might be some bugs that I haven't noticed.

This commit fixes the style issues introduced in 0425f4c according to the output from PHP CS Fixer. Details: #358

viktorcsimma · 2023-12-30T15:25:52Z

What should we do? I really think this would be useful; now that this is going to be the new status quo. @machiato32?

sszajbely

The changes are alright. If this status really needs to stay, I guess I'll have to approve it. What do you think @kdmnk?

sszajbely · 2023-12-30T16:26:32Z

Also, please fix the merge conflicts @viktorcsimma

This commit fixes the style issues introduced in 0425f4c according to the output from PHP CS Fixer. Details: #358

viktorcsimma · 2023-12-30T16:51:10Z

Something is not OK yet; it crashes at the economic committee page. I'll check it out soon.

sszajbely · 2023-12-30T17:35:18Z

Ah, sorry didn't test it, just looked at the code.

kdmnk

Can not we name this "LIMITED_RESIDENT" role internally? It is getting to be really confusing.
Alternatively, I was also thinking on making the roles time constrained. That way, we could have regular collegists, plus an additional resident role that is set to be valid for 1 year/indefinite. We can also use that for guests, instead of the guest_until attribute and solve #291. (Also, the general collegist role is also valid only until the end of their studies by default which we can set this way).

@machiato32 what do you think, does putting effort into this would worth it?
(We have some time to work on this, the earliest this will be relevant is next summer)

kdmnk · 2024-01-01T16:47:53Z

app/Http/Controllers/Secretariat/SemesterEvaluationController.php

@@ -157,7 +157,7 @@ public function store(Request $request)
                        return back()->with('error', "A státusz megadása kötelező!")->with('section', $request->section);
                    }
                    $user->setStatusFor(Semester::next(), $request->next_status, $request->next_status_note);
-                    if ($request->has('resign_residency') && $user->isResident()) {
+                    if ($request->has('resign_residency') && $user->resides()) {


resides() is a scope, which I think returns a query builder (test it in tinker). Check if these checks work as expected. In the tests it also seems fine, just double check it, I would not call scopes this way. I would rather make it a simple function

kdmnk · 2024-01-01T16:47:59Z

app/Models/ApplicationForm.php

@@ -286,7 +289,7 @@ public function missingData(): array
            $missingData[] =  'Megjelölt kar';
        }

-        if (!$user->isResident() && !$user->isExtern()) {
+        if (!$user->resides() && !$user->isExtern()) {


kdmnk · 2024-01-02T10:10:51Z

Can not we name this "LIMITED_RESIDENT" role internally? It is getting to be really confusing. Alternatively, I was also thinking on making the roles time constrained. That way, we could have regular collegists, plus an additional resident role that is set to be valid for 1 year/indefinite. We can also use that for guests, instead of the guest_until attribute and solve #291. (Also, the general collegist role is also valid only until the end of their studies by default which we can set this way).

@machiato32 what do you think, does putting effort into this would worth it? (We have some time to work on this, the earliest this will be relevant is next summer)

I'm starting to like this idea. The role_user table would have a created_at and valid_until field, and we would use a global scope to filter on those. This way, we don't delete roles just invalidate them, making a log out of the table.

viktorcsimma · 2024-01-03T09:01:31Z

Can not we name this "LIMITED_RESIDENT" role internally? It is getting to be really confusing. Alternatively, I was also thinking on making the roles time constrained. That way, we could have regular collegists, plus an additional resident role that is set to be valid for 1 year/indefinite. We can also use that for guests, instead of the guest_until attribute and solve #291. (Also, the general collegist role is also valid only until the end of their studies by default which we can set this way).
@machiato32 what do you think, does putting effort into this would worth it? (We have some time to work on this, the earliest this will be relevant is next summer)

I'm starting to like this idea. The role_user table would have a created_at and valid_until field, and we would use a global scope to filter on those. This way, we don't delete roles just invalidate them, making a log out of the table.

That sounds good:) Maybe we should open a new issue for that and freeze this until that gets solved.

viktorcsimma · 2024-01-03T09:02:29Z

What should we do with this one? Should we merge it and then reverse it when working on that issue, or abandon it altogether?

kdmnk · 2024-01-03T09:23:01Z

I'll close this for now.

viktorcsimma requested a review from kdmnk October 28, 2023 14:52

viktorcsimma self-assigned this Oct 28, 2023

deepsource-autofix bot added a commit that referenced this pull request Oct 28, 2023

style: format code with PHP CS Fixer

7d424f0

This commit fixes the style issues introduced in 0425f4c according to the output from PHP CS Fixer. Details: #358

viktorcsimma pushed a commit that referenced this pull request Oct 28, 2023

style: format code with PHP CS Fixer

983f71d

This commit fixes the style issues introduced in 0425f4c according to the output from PHP CS Fixer. Details: #358

viktorcsimma force-pushed the resident-externs branch from 7d424f0 to 3d309bd Compare October 28, 2023 15:05

kdmnk requested a review from sszajbely November 4, 2023 15:13

sszajbely reviewed Dec 30, 2023

View reviewed changes

viktorcsimma and others added 3 commits December 30, 2023 17:45

Largely doing it; from the database to the logic and the room editor

478db5f

style: format code with PHP CS Fixer

0e81013

This commit fixes the style issues introduced in 0425f4c according to the output from PHP CS Fixer. Details: #358

Adding documentation comments

f2d8631

viktorcsimma force-pushed the resident-externs branch from 3d309bd to f2d8631 Compare December 30, 2023 16:47

viktorcsimma marked this pull request as draft December 30, 2023 16:51

kdmnk reviewed Jan 1, 2024

View reviewed changes

kdmnk mentioned this pull request Jan 3, 2024

Restructure Roles #392

Open

kdmnk closed this Jan 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a resident-extern role #358

Adding a resident-extern role #358

viktorcsimma commented Oct 28, 2023

viktorcsimma commented Dec 30, 2023

sszajbely left a comment

sszajbely commented Dec 30, 2023

viktorcsimma commented Dec 30, 2023

sszajbely commented Dec 30, 2023

kdmnk left a comment •

edited

Loading

kdmnk Jan 1, 2024

kdmnk Jan 1, 2024

kdmnk commented Jan 2, 2024

viktorcsimma commented Jan 3, 2024

viktorcsimma commented Jan 3, 2024

kdmnk commented Jan 3, 2024

Adding a resident-extern role #358

Adding a resident-extern role #358

Conversation

viktorcsimma commented Oct 28, 2023

viktorcsimma commented Dec 30, 2023

sszajbely left a comment

Choose a reason for hiding this comment

sszajbely commented Dec 30, 2023

viktorcsimma commented Dec 30, 2023

sszajbely commented Dec 30, 2023

kdmnk left a comment • edited Loading

Choose a reason for hiding this comment

kdmnk Jan 1, 2024

Choose a reason for hiding this comment

kdmnk Jan 1, 2024

Choose a reason for hiding this comment

kdmnk commented Jan 2, 2024

viktorcsimma commented Jan 3, 2024

viktorcsimma commented Jan 3, 2024

kdmnk commented Jan 3, 2024

kdmnk left a comment •

edited

Loading