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/condensed labels #13

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@diegosteiner
Contributor

diegosteiner commented Nov 15, 2015

Abstract

This PR includes the implementation of a feature that has been requested on ideascale here: http://midata.ideascale.com/a/dtd/Nur-eine-Etikette-pro-Haushalt/94704-26593. It states that it should be possible for multiple people living at the same household to only print one, condensed label.

Usage

To make use of this feature either check the checkbox located in the "Export > Labels"-Menu or add condense_labels=true to the query parameters when calling the export.

image

Implementation

To implement this feature a wrapper class for contactables CondensedContact was created. The 'merge-strategy' is conservative and only allows contactables with matching country, town, zip-code, address, and last_name to be merged. The CondensedContact class acts to implement the same interface as any other contactable, but will raise an error when an ambivalent field is accessed (e.g. id or first_name)

Performance

The condensation of a list runs with O(n^2), as it has to compare every item with another in the list. This is necessary, as it can not be asserted that the list has been sorted correctly.

Conventions

It has been tried to follow the established conventions of the project, which includes testing with rspec and asserting code style with rubocop -c rubocup-must.yml

Contact

Diego Steiner / Filou, Pfadi Züri

@sihu

This comment has been minimized.

sihu commented Nov 16, 2015

very nice 👍

@schmijos

This comment has been minimized.

schmijos commented Nov 17, 2015

nice! 👍

@codez

This comment has been minimized.

Contributor

codez commented Nov 17, 2015

Vielen Dank für den Pull Request und die ausführliche Beschreibung.

Damit das Feature für alle interessant ist, braucht es wohl noch einen anderen Einstiegspunkt ausser dem Query Parameter (sonst weiss das ausser uns niemand ;) ). Leider ist dieser nicht gerade offensichtlich. Eine weitere Sub-Menu Ebene beim Export Button wäre möglich, würde wohl aber anderen Hitobito Instanzen in die Quere kommen. Hast du dafür noch Ideen?

Du hängst die Logik in die Methode PeopleController#filter_entries, welche auch für alle anderen Ansichten/Exportformate verwendet wird. Ist dies ein explizites Bedürfnis oder würde das Zusammenfassen der Addressen für die Etiketten reichen? So wie CondensedContact momentan implementiert ist, funktionieren andere Formate mit condense=true nicht.

Könntest du zudem CondensedContact nach app/domain/person verschieben? In app/models sind primär Active Record Klassen, in domain die Business Logik.

@diegosteiner

This comment has been minimized.

Contributor

diegosteiner commented Nov 18, 2015

Der Einstiegspunkt ist absichtlich so gewählt, weil ich das Feature niemandem aufzwingen wollte. Diejenigen die es brauchen, sollten es aber können. In einem zweiten Schritt habe ich mir überlegt, dass es einen Export-Wizard geben sollte, bei dem man dann auch die Sortierung und (wie auf ideascale schon jemand gewünscht hat) die Auswahl der angezeigten Felder.

Ich bin auch nicht der Ansicht, dass das das wichtigste Feature ist, aber ich wollte mal sehen, ob ich auch etwas beitragen kann.

Die Klasse habe ich verschoben, und zusätzliche Tests geschrieben. Danke für den Hinweis.

@codez

This comment has been minimized.

Contributor

codez commented Nov 19, 2015

Aufzwingen wollen wir das sicher niemandem, aber jedem die Möglichkeit bieten. Ich finde das Feature durchaus praktisch, deshalb sollte es auch für alle im GUI auffindbar sein.

@RolandStuder

This comment has been minimized.

Member

RolandStuder commented Nov 20, 2015

Hallo Diego

Tausend Dank, cool, dass wir jetzt Contributions erhalten. Das ist grandios!
Ich habe mir überlegt, wie man das im UI Abbilden könnte.

Ich präferiere etwas in Richtung B oder D. Fände das gut.

Aber ich denke es wäre auch Ok, das zuerst als Power User feature festzuhalten und vielleicht zu messsen, wie oft, das dann gebraucht wird.

Was meint ihr hierzu?

Gruäss
Roland

mehrfachsendungen vermeiden

@bsantschi

This comment has been minimized.

Contributor

bsantschi commented Nov 23, 2015

Wau, vielen Dank für das tolle Feature!

Der nächste PBS-Release ist im Januar 2016 geplant. Frage an dich, Diego: Findest du bis dann Zeit, eine der genannten GUI-Varianten umzusetzen?

Gruss
Bruno

@diegosteiner

This comment has been minimized.

Contributor

diegosteiner commented Nov 23, 2015

Hallo zusammen

Ich finde die Vorschläge gut, mir gefällt die Variante C am besten, weil es den jetztigen Workflow am wenigsten durcheinanderbringt. Ich kann das umsetzten, sobald wir uns für eine Variante entschieden haben.

lg Filou

@codez

This comment has been minimized.

Contributor

codez commented Nov 23, 2015

Variante C ist wohl einfacher zu implementieren, da bisher noch kein "Schieber Widget" wie in D existiert (oder gibt's das doch schon, @RolandStuder?).

Ein paar Hints noch:

  • Für den neuen Eintrag mit der Checkbox: Neue Subklasse von Dropdown::Item mit eigener render Methode erstellen, z.B. Dropdown::PeopleExport::ToggleCondensedLabelsItem.
  • In app/assets/javascripts/modules/ können weitere Coffee Script Dateien abgelegt werden, welche automatisch geladen werden.

Bei weiteren Fragen kannst du dich gerne melden.

@codez

This comment has been minimized.

Contributor

codez commented Dec 30, 2015

Wir sind definitiv für Variante C (oder B, Widget werden wir nächstens einbauen). @diegosteiner, hast du Zeit, das zu umzusetzen?

diegosteiner added some commits Nov 14, 2015

implemented change requests from PR !13
* added option to export dropdown menu
* ran rubocop/coffeelint

# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
update naming and translations
# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
styling for dropdowns
# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
fix tests
# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
@diegosteiner

This comment has been minimized.

Contributor

diegosteiner commented Jan 11, 2016

Ich habe das Feedback umgesetzt. Ich hoffe das passt euch so. Einige Hinweise:

  • Die Umsetzung ist gemäss Vorschlag "Variante C". Screenshot liegt bei der Beschreibung bei
  • Bei den Translations habe ich nur mal die "de" reingenommen. Soweit ich das verstanden habe, kümmern sich die Übersetzer um den Rest, korrekt?
  • Mit den letzten Versionen der Wagons "hitobito_youth" und "hitobito_pbs" habe ich Probleme, die Applikation (bzw. den Server) zum laufen zu bringen; /Users/digi/.rbenv/versions/1.9.3-p551/lib/ruby/gems/1.9.1/gems/activesupport-4.2.5/lib/active_support/core_ext/module/aliasing.rb:32:in alias_method': undefined method build_application' for class Event::ParticipationsController' (NameError)`. Die Tests laufen jedoch durch. Hat jemand gerade eine Idee? Dann könnte ich das nochmals manuell testen
@codez

This comment has been minimized.

Contributor

codez commented Jan 11, 2016

Sieht super aus, merci! Ich werde die Änderungen so rasch als möglich integrieren.

Der hitobito_youth wagon war nicht auf dem neusten Stand. Sollte jetzt funktionieren.

@codez

This comment has been minimized.

Contributor

codez commented Jan 11, 2016

Gemerged und das Parameter Handling verschoben, damit das Feature auch bei Anlässen und Abos funktioniert.

@codez codez closed this Jan 11, 2016

@diegosteiner

This comment has been minimized.

Contributor

diegosteiner commented Jan 11, 2016

👍 Danke!

@olibrian

This comment has been minimized.

olibrian commented Jan 12, 2016

Danke Filou!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment