Hide danger zone if not manager #1382
Conversation
| stubs: ['camp-settings', 'camp-address', 'camp-periods', 'camp-categories', 'camp-material-lists'] | ||
| }) | ||
|
|
||
| getByText('components.camp.campDangerzone.title') |
There was a problem hiding this comment.
Ja, die getBy* Funktionen in vue testing library werfen eine Exception wenn sie eine Weile lang keinen Treffer finden. Daher muss man beim Absenz-Test stattdessen die queryBy* Funktionen verwenden. Wir könnten die Präsenz-Assertion aber alternativ auch als expect(getByText('...')).toBeInTheDocument() schreiben, falls dir das intuitiver scheint.
Siehe auch https://testing-library.com/docs/guide-disappearance/#asserting-elements-are-not-present
There was a problem hiding this comment.
Würde ich intuitiver finden, da man die Details der getByText funktion nicht kennen müsste.
Ist aber geschmackssache.
Wie lange ist das Timeout von getByText?
There was a problem hiding this comment.
Okay, hab ich angepasst. Wenn der Test fehlschlägt ists halt weiterhin wegen dem Error den getByText wirft, nicht wegen dem expect. Aber liest sich vermutlich wirklich intuitiver.
Das mit dem Timeout hab ich verwechselt, getBy* und queryBy* haben keins, das Element wird nur einmal gesucht. Wenn man retries will muss man entweder waitFor verwenden, oder die dritte Familie von Queries findBy* mit async/await. waitFor hat als Standard-Timeout 1 Sekunde, und findBy* sind Convenience Funktionen für die Kombination von waitFor und getBy*, also ebenfalls standardmässig 1 Sekunde.
There was a problem hiding this comment.
Danke für die Erklärung.
Bei timeouts in "Unittests" werde ich immer hellhörig. Bin schon genug reingelaufen.
| @@ -56,6 +56,11 @@ export default { | |||
| data () { | |||
| return {} | |||
| }, | |||
| computed: { | |||
| isManager () { | |||
| return this.camp().role === 'manager' | |||
There was a problem hiding this comment.
Ich werde durch alles durchgehen, und zumindest den String 'manager' auslagern, vielleicht finde ich auch was schöneres
There was a problem hiding this comment.
Findest du es ein Problem dass hier der String manager hardcoded ist? Ich habe mal angenommen dass wir uns dazu im Backend commiten, und jede Änderung daran wäre nach dem Go-Live ein breaking change.
Ich finde es mässig cool, dass dieses Feld auf dem Camp mitgeliefert wird, und so verschiedene User verschiedene Daten vom selben Camp bekommen. Das macht es in ferner Zukunft eine Spur schwieriger, gegebenenfalls einen Caching-Layer (ähnlich wie hier) einzubauen. Aber da die Berechnung via Profil, User und Collaborators recht viel komplizierter wäre habe ich mich mal dafür entschieden dieses Feld zu nutzen.
There was a problem hiding this comment.
Ich werde es mir mit #1238 so oder so noch anschauen.
Je nach dem werde ich eine schlauere Lösung brauchen, als überall im code this.camp().role === 'manager' zu verteilen.
Und falls nicht, würde ich die verschiedenen möglichen rollen als "Enum" zentral mappen, sodass man nicht mit search und replace renamen muss.
Aber das werde ich anschauen, du musst da nichts machen.
While fixing the delete dialogs, I noticed that deleting a camp is only allowed for a manager. So for now, we can hide the danger zone for users that are not managers.
I also introduce vue testing library, which is a wrapper around vue-test-utils with the goal to encourage us to write tests that are more maintainable and easier to understand. We will definitely need some common way to mock API calls, see ecamp/hal-json-vuex#65. But for now, this should do.