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

Import stuff from old repo #10

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Import stuff from old repo #10

wants to merge 9 commits into from

Conversation

cleverer
Copy link
Member

@cleverer cleverer commented May 30, 2020

@simfeld
Copy link

simfeld commented Jun 1, 2020

@cleverer merci für das Refactoring!

Zum Testing der PageHelper: Wenn ich dich richtig verstehe schlägst du vor, via UI zu überprüfen, ob die per Request gemachten Änderungen auch tatsächlich angezeigt werden? Anstatt wie aktuell via API/Datenbank-Abfrage die Objekte zu vergleichen. Also bspw. ich ändere den Namen eines Events via POST-Request und checke per Assertion, ob der im UI angezeigte Name dem neuen entspricht.

@cleverer
Copy link
Member Author

cleverer commented Jun 1, 2020

Gerne, das Refactoring darf auch gerne in Frage gestellt werden ;)

Ja genau, wir wollen ja keine unit tests machen, sondern verifizieren, das die page-helper das gleiche bewirken wie ui interaktionen.
Ich glaube aber, dass wir das schon mal diskutiert hatten, mag mich aber nicht mehr erinnern. IMHO können wirs auch mal so lassen. Als negativpunkt sehe ich halt, dass die tests kaputt gehen könnten, wenn sich im backend was ändert, ohne dass sich im frontend was ändert …

@cleverer
Copy link
Member Author

Leider hat sich meine Befürchtung nun bewahrheitet (glaube ich zumindest)… siehe Kommentar hier: 2264e32#diff-e0932ba83b9ff01616eeae0bd38bcba9R35

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 das Refactoring! Ich finde die Änderungen sinnvoll, da uns das etwas unabhängiger von den Datenbank-Seeds (Daten) macht und uns erlaubt, uns mehr auf Tests für die Logik (Code) zu konzentrieren.
In Sachen Lesbarkeit, Developer Convenience beim Tests schreiben, sowie Maintainability haben wir wohl noch einen langen Weg vor uns.


export const GET_PERSON_IN_ABTEILUNG_BELOW = (group_id) => `begin
group = Group.find(${group_id})
person = Person.joins("INNER JOIN groups ON groups.lft >= #{group.lft} AND groups.lft < #{group.rgt} AND groups.id = people.primary_group_id").last
Copy link
Member

Choose a reason for hiding this comment

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

Das findet streng genommen nicht jemand in einer Abteilung, sondern in einer Gruppe darunter. Etwas sprechender könnte man dasselbe wohl auch so schreiben:

Person.where(primary_group: group.descendants).last

Comment on lines +24 to +26
rescue StandardError => e
return e
end`
Copy link
Member

Choose a reason for hiding this comment

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

Statt überall zu rescuen könnten wir vielleicht in Zukunft auch mal den appEval Command erweitern

end`

export const JSON_APPROVAL = (participation_id) => `begin
Event::Approval.joins('INNER JOIN event_participations ON event_participations.application_id = event_approvals.application_id')
Copy link
Member

Choose a reason for hiding this comment

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

Auch hier könnte man vielleicht marginal sprechender schreiben:

Event::Approval
  .joins(application: :participation)
  .where(approved: true)
  .where(application: { participations: participation_id })
  .as_json(except: :approved_at)

Comment on lines +23 to +32
export const get_reset_auto_increment_query = (entities) => {
let query = '';
entities.forEach((entity) => {
query += `entity_table_name = \'${entity}\'.constantize.table_name;
new_id = ${entity}.maximum(:id).next;
ActiveRecord::Base.connection.execute("ALTER TABLE #{entity_table_name} AUTO_INCREMENT = #{new_id};");
`
})
return query
}
Copy link
Member

Choose a reason for hiding this comment

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

Gibt es einen Grund weshalb dieser Query nicht in cypress/support/queries.js liegt und gleich formatiert ist wie diese?

Ausserdem würde ich in 2020 eher die funktionale statt imperative Schreibweise nehmen:

return entities.map(entity => { /* ... */ }).join("\n")

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