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

Feature/612 join zusatzsektion model #651

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

amaierhofer
Copy link
Contributor

@amaierhofer amaierhofer commented Jun 18, 2024

fixes #612

@amaierhofer amaierhofer force-pushed the feature/612-join-zusatzsektion-model branch from fd9c497 to e419216 Compare June 18, 2024 14:25
@mtnstar mtnstar force-pushed the feature/612-join-zusatzsektion-model branch from e419216 to fd8666f Compare June 19, 2024 05:15
Copy link
Member

@mtnstar mtnstar left a comment

Choose a reason for hiding this comment

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

nice, das sieht doch schon sehr gut aus 🥳

ich würde hier beim Naming noch etwas mehr darauf achten das der Code verständlicher wird. Ich verwende auch gerne den sac prefix für gewisse terms.

siehe Kommentare

def initialize(person, reference_date: Time.zone.today)
@person = person
@reference_date = reference_date
end

def calculate
return CATEGORY_FAMILY if family_member?
def calculate(consider_family: true)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def calculate(consider_family: true)
def calculate(concern_sac_family: true)

consider ist für mich jetzt eher überlegen ob ich was machen soll oder nicht.
concerning ist berücksichtigen auf deutsch, oder?!
ich weiss, das war so spezifiziert 😉

ggf. einfach auch for_sac_family

Copy link
Contributor Author

Choose a reason for hiding this comment

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

habs auf for_sac_family

@group = join_section.group_for_neuanmeldung
@created_at = Time.zone.now

raise 'missing subgroup' unless group
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise 'missing subgroup' unless group
raise 'missing neuanmeldungen subgroup' unless group

def initialize(join_section, person, join_date, family_membership: false, **params)
super(join_section, person, join_date, **params)
@family_membership = family_membership
@group = join_section.group_for_neuanmeldung
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@group = join_section.group_for_neuanmeldung
@neuanmeldungen_group = join_section.group_for_neuanmeldung

etwas längerer name, dafür aber gleich klar um was es sich handelt ?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

via delegation gelöst

def prepare_roles(person)
group.roles.build(
person: person,
type: group.class.const_get('NeuanmeldungZusatzsektion'),
Copy link
Member

Choose a reason for hiding this comment

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

👍

spec/domain/sac_cas/beitragskategorie/calculator_spec.rb Outdated Show resolved Hide resolved
]
end

it 'is invalid if person has requested membership via section' do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it 'is invalid if person has requested membership via section' do
it 'is invalid if person has requested membership in join section with approval' do

]
end

it 'is invalid if person has requested membership via nv' do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it 'is invalid if person has requested membership via nv' do
it 'is invalid if person has requested membership in join section' do


describe 'saving' do
let(:person) { Fabricate(:person) }
let(:group) { groups(:matterhorn) }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let(:group) { groups(:matterhorn) }
let(:sac_section) { groups(:matterhorn) }

end

context 'single person' do
describe 'group priority in sektion' do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe 'group priority in sektion' do
describe 'neuanmeldungen priority in sac section' do

expect(role.group).to eq groups(:matterhorn_neuanmeldungen_sektion)
expect(role.type).to eq 'Group::SektionsNeuanmeldungenSektion::NeuanmeldungZusatzsektion'
end

Copy link
Member

Choose a reason for hiding this comment

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

hier auch das Naming ... Grundsätzlich gibt es zwei Arten von Neuanmeldunge. ...

  1. mit Freigabe durch Sektion
  2. ohne Freigabe durch Sektion

die Begriffe hier sind sehr technisch und man muss genau wissen warum diese unterschiedlichen Gruppen vorhanden sind.

@amaierhofer amaierhofer force-pushed the feature/612-join-zusatzsektion-model branch from fd8666f to d0d1680 Compare June 19, 2024 11:59
@amaierhofer amaierhofer force-pushed the feature/612-join-zusatzsektion-model branch from d0d1680 to 4f0326b Compare June 19, 2024 12:21
@amaierhofer amaierhofer force-pushed the feature/612-join-zusatzsektion-model branch from 4f0326b to e8b836a Compare June 19, 2024 12:33
@amaierhofer amaierhofer merged commit 9e68919 into master Jun 19, 2024
7 checks passed
@amaierhofer amaierhofer deleted the feature/612-join-zusatzsektion-model branch June 19, 2024 12:33
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.

PEOPLE: Memberships::JoinZusatzsektion model class
2 participants