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.

Sign up for GitHub

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

Jump to bottom

PEOPLE: Bemerkungsfelder #658

Merged
merged 12 commits into from
Jul 3, 2024
Merged

PEOPLE: Bemerkungsfelder #658

merged 12 commits into from
Jul 3, 2024

Conversation

hunchr
Copy link
Contributor

@hunchr hunchr commented Jun 19, 2024

Fixes #511

@hunchr hunchr self-assigned this Jun 19, 2024
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch 7 times, most recently from 9f44800 to 7816dfb Compare June 24, 2024 12:49
@hunchr hunchr requested a review from mtnstar June 24, 2024 12:50
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch from 7816dfb to 2382044 Compare June 24, 2024 13:26
@hunchr hunchr marked this pull request as ready for review June 24, 2024 13:26
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch from 2382044 to ff5f6ef Compare June 24, 2024 14:23
@mtnstar mtnstar force-pushed the feature/511-people-additional-fields branch from ff5f6ef to f5e4faf Compare June 25, 2024 11:07
mtnstar
mtnstar requested changes Jun 25, 2024
View reviewed changes
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.

@hunchr das sieht schon wirklich sehr gut aus und funktioniert auch!

ich bin noch nicht komplett mit dem review durch. kannst du dich mal um die naming feedbacks kümmern?

sowie einen eigenen unit test für die ability schreiben?

app/abilities/sac_cas/person_ability.rb Outdated Show resolved Hide resolved
app/controllers/person/remarks_controller.rb Outdated Show resolved Hide resolved
app/domain/sac_cas.rb Outdated Show resolved Hide resolved
@hunchr hunchr requested a review from mtnstar June 25, 2024 15:27
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch from f709c80 to dc03785 Compare June 26, 2024 07:00
mtnstar
mtnstar requested changes Jun 26, 2024
View reviewed changes
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.

chunnt mega guet @hunchr 👍

schau bitte das du das Naming überall konsistent machst und das du den Glossar entsprechend aktualisierst. Sprich die Begriffe die du verwendet sind im Glossar vorhanden.

app/abilities/sac_cas/person_ability.rb Show resolved Hide resolved
app/abilities/sac_cas/person_ability.rb Outdated Show resolved Hide resolved
app/abilities/sac_cas/person_ability.rb Outdated Show resolved Hide resolved
app/controllers/person/remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/remarks_controller.rb Outdated Show resolved Hide resolved
app/models/sac_cas/person.rb Outdated Show resolved Hide resolved
app/models/sac_cas/person.rb Outdated Show resolved Hide resolved
doc/naming.md Outdated Show resolved Hide resolved
spec/abilities/sac_cas/person_ability_spec.rb Outdated Show resolved Hide resolved
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch 7 times, most recently from 424dbd4 to f2a79bd Compare July 1, 2024 06:53
@hunchr hunchr requested a review from mtnstar July 1, 2024 06:56
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch from f2a79bd to 70f929a Compare July 1, 2024 07:06
mtnstar
mtnstar requested changes Jul 1, 2024
View reviewed changes
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.

go go go @hunchr ... wird immer besser 🥳

  • es braucht noch etwas mehr Konsistenz beim Naming
  • definiere an zentraler stelle (z.B. im Controller) welche attribute durch wen updated/gelesen werden können
  • stell sicher das dein Berechtigungskonzept verhebt in dem du die Controller specs ergänzt (z.B. das ich als Sektionsfunktionär nicht irgendwie das GS attribut schreiben/lesen kann

app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/views/person/sac_remarks/edit.html.haml Outdated Show resolved Hide resolved
app/views/person/sac_remarks/index.html.haml Outdated Show resolved Hide resolved
spec/controllers/person/remarks_controller_spec.rb Outdated Show resolved Hide resolved
spec/features/person/person_remarks_spec.rb Outdated Show resolved Hide resolved
spec/features/person/person_remarks_spec.rb Outdated Show resolved Hide resolved
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch from 7879f14 to cad28f0 Compare July 1, 2024 12:30
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch from 074d44b to 3049d7c Compare July 1, 2024 12:37
@hunchr hunchr requested a review from mtnstar July 1, 2024 12:53
mtnstar
mtnstar requested changes Jul 2, 2024
View reviewed changes
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.

fast fertig 👍

  • versuche das mit der collection der erlaubten remark_attrs noch zu implementieren, das sollte machbar sein 😉
  • füge bei den controller test cases noch einen check für den http status ein. evtl. kannst du dort auch noch etwas detaillierter testen (z.B. remark attrs mit den richtigen Werten im Controller vorhanden sind bei #index)

app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
spec/controllers/person/remarks_controller_spec.rb Outdated Show resolved Hide resolved
spec/controllers/person/remarks_controller_spec.rb Outdated Show resolved Hide resolved
spec/features/person/person_remarks_spec.rb Outdated Show resolved Hide resolved
db/migrate/20240619072225_add_remarks_to_people.rb Outdated Show resolved Hide resolved
spec/controllers/person/remarks_controller_spec.rb Outdated Show resolved Hide resolved
@hunchr hunchr requested a review from mtnstar July 2, 2024 06:45
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch from 965303c to 7a9ba4f Compare July 2, 2024 07:52
mtnstar
mtnstar requested changes Jul 2, 2024
View reviewed changes
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, fascht gschaft !
na äs bar chlini inputs zum aluege

go go go 🚀

app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/models/sac_cas/person.rb Show resolved Hide resolved
app/models/sac_cas/person.rb Show resolved Hide resolved
app/controllers/person/sac_remarks_controller.rb Outdated Show resolved Hide resolved
app/views/person/sac_remarks/index.html.haml Outdated Show resolved Hide resolved
spec/features/person/person_remarks_spec.rb Outdated Show resolved Hide resolved
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch from 2c1d98c to 8e26a92 Compare July 2, 2024 09:57
@hunchr hunchr requested a review from mtnstar July 2, 2024 12:32
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch 2 times, most recently from 612b8b0 to c667a17 Compare July 3, 2024 07:24
@hunchr hunchr force-pushed the feature/511-people-additional-fields branch from f56235e to c3035d0 Compare July 3, 2024 09:30
@hunchr hunchr merged commit 1ca1d85 into master Jul 3, 2024
9 checks passed
@hunchr hunchr deleted the feature/511-people-additional-fields branch July 3, 2024 09:49
@hunchr hunchr changed the title PEOPLE: Zusätzliche Bemerkungsfelder PEOPLE: Bemerkungsfelder Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtnstar mtnstar Awaiting requested review from mtnstar

Assignees

@hunchr hunchr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PEOPLE: Zusätzliche Bemerkungsfelder
2 participants
@hunchr @mtnstar