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

General assembly: excuse passives (fixes #417) #471

Merged
merged 8 commits into from
Mar 21, 2024

Conversation

Trigary
Copy link
Collaborator

@Trigary Trigary commented Mar 16, 2024

No description provided.

@Trigary Trigary requested a review from a team as a code owner March 16, 2024 19:42
Copy link
Contributor

@kdmnk kdmnk left a comment

Choose a reason for hiding this comment

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

Nice! We have some test cases for general assemblies. Can you add one to test this behaviour? Add 3 users, 1 active 2 passive, make a general assembly and check the excusedUsers if its set properly.

If you want, we could also make some improvements with the excused users here:
The table where we collect them are called "general_assembly_user" but the naming should conclude something about exclusion. Make a migration to rename it.
And another thing came into my mind is that we should add a comment/reason for the exclusion, especially if we start adding users automatically. We have some examples for pivot attributes (roles, semester status) but a simple value is enough here. You can check https://laravel.com/docs/10.x/eloquent-relationships#retrieving-intermediate-table-columns.
Then you also need to get the value from the UI.
Or you can open a new issue for these.

@Trigary
Copy link
Collaborator Author

Trigary commented Mar 16, 2024

Thanks, I'll make the changes you mentioned.

I just saw #300 (which mentions that observers should be preferred), therefore I just pushed a commit that makes the necessary changes. Let me know if I misunderstood something and should undo the commit.

@Trigary Trigary marked this pull request as draft March 16, 2024 20:20
@kdmnk
Copy link
Contributor

kdmnk commented Mar 16, 2024

No it's good, I just did not want to say it as we you only used the "created" event here. Observers are best if you handle a lot of events there but at least now we are consistent.

@Trigary
Copy link
Collaborator Author

Trigary commented Mar 17, 2024

I made the requested changes :)

I made two separate database migrations, because I figured a table rename is disruptive enough to warrant its own migration file. Let me know if I should combine the migrations instead.

The way the localization is handled for the comment column is sub-ideal: the backend fills the database with a localized string based on the locale of the user who creates the general assembly. Instead of storing a localized string, we could add some sort of flag column that determines whether the contents of the comment column should be displayed as-is or if it should be used as a localization key. Let me know if the current solution is adequate or if I should do something differently.

@Trigary Trigary requested a review from kdmnk March 17, 2024 00:11
@Trigary Trigary marked this pull request as ready for review March 17, 2024 01:48
Copy link
Contributor

@kdmnk kdmnk left a comment

Choose a reason for hiding this comment

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

LGTM. You can store the localization key and translate when you display it. It won't mess up custom values.

@horcsinbalint horcsinbalint merged commit f22f4f0 into EotvosCollegium:development Mar 21, 2024
5 checks passed
@Trigary Trigary deleted the issue-417 branch March 21, 2024 19:19
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.

3 participants