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

Subscriptions endpoint #1398

Merged
merged 9 commits into from
Oct 15, 2021

Conversation

Michael-Schaer
Copy link
Contributor

Allows access to the subscriptions (!= subscribers) of a mailing list.

Copy link
Member

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

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

Danke für den PR! Ist das schon bereit aus deiner Sicht? In der ersten Commit Message erwähnst du noch dass die Tags noch nicht kommen, und die sehe ich auch nicht wenn ich den Endpoint aufrufe. Die gewählten Tags sind ähnlich wie die Rollen in einer separaten Tabelle subscription_tags gespeichert.

So oder so: Könntest du noch den Endpoint in der Dokumentation zumindest erwähnen? Und um ein paar Specs für den Endpoint wäre ich auch noch froh.

@Michael-Schaer
Copy link
Contributor Author

Die subscription_tags kann man leider nicht so wie die Rollen einfach dazu nehmen, sonst erhält man die ID der Tags ohne Labels. Es bräuchte also noch einen Join, bzw. ein wenig Rails Magic, die ich nicht verstehe ;)
Nach mir können wir die Tags für diese erste Version gerne auch weglassen, wir brauchen sie nicht.

Bei den Specs habe ich mich gefragt, was ich denn testen soll. Einfach, dass es die Schnittstelle gibt? Logik gibt es ja eigentlich keine...

@carlobeltrame
Copy link
Member

Die Tags sehe ich dass die nicht ganz so simpel sind. Etwas schade, aber das können wir aus meiner Sicht auch noch später nachliefern.

Bei den Tests frage ich mich jeweils gerne, "bei welchen Teilen der Implementation ist es denkbar dass sie in Zukunft mal kaputt gehen". Nach dieser Logik würde wohl ein einfacher Test genügen, der sicherstellt dass der Controller auf format: :json reagiert und eine Test-Subscription (am besten eine von jedem Typ) ausgibt. Orientieren könntest du dich z.B. an https://github.com/hitobito/hitobito/blob/master/spec/controllers/event/participations_controller_spec.rb#L145, oder an anderen Orten in *_spec.rb-Dateien in denen format: :json erwähnt wird.

@Michael-Schaer
Copy link
Contributor Author

@carlobeltrame Ich habe ein paar einfache specs geschrieben. Passt das so?

Der build fail hat glaube ich nichts mit dem PR zu tun, da geht es um Personen (?). Das angepasste spec file läuft bei mir jedenfalls gut durch...

@carlobeltrame
Copy link
Member

Merci @Michael-Schaer. Der Test war tatsächlich nicht deine Schuld. Ich habe deine Tests noch etwas erweitert, die Tags gleich auch noch verfügbar gemacht und einen Eintrag in Dokumentation und Changelog gemacht.

@carlobeltrame carlobeltrame merged commit c59532a into hitobito:master Oct 15, 2021
@Michael-Schaer
Copy link
Contributor Author

Sehr toll! Danke für das schnelle Aufnehmen und das Aktualisieren von Dok und Changelog!

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.

2 participants