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

Add personalized and admin label-formats #130

Closed
wants to merge 12 commits into from

Conversation

dinuu
Copy link
Contributor

@dinuu dinuu commented Nov 29, 2016

#76

@dinuu
Copy link
Contributor Author

dinuu commented Nov 29, 2016

Im Issue #76 wurde nicht definiert welche Etiketten beim Export angezeigt werden.
Zurzeit werden nun alle angezeigt, auch die von anderen Personen. Wenn dies nicht so gewünscht ist, bitte Rückmeldung.

@adriananderegg
Copy link
Contributor

@dinuu ein User soll seine eigenen sowie die öffentlichen (vom Admin definierten) Etikettenformate sehen.
Die von anderen Benutzern sollte nicht sichtbar sein, sonst wird das bei 2000 Benutzern sehr schnell sehr unübersichtlich.

@RolandStuder
Copy link
Contributor

Genau

  • Benutzer soll jeweils die Eigenen Etikettenformate und die vom Admin definierten sehen. Etikettenformate von anderen Benutzern soll er nicht sehen.
  • Benutzer kann aktivieren, dass er nur noch seine eigenen sieht und nicht mehr die vom Admin.

@@ -15,6 +15,8 @@ class VariousAbility < AbilityDsl::Base
on(LabelFormat) do
class_side(:index).if_admin
permission(:admin).may(:manage).all
permission(:any).may(:show_nav, :index, :show, :edit, :destroy, :create).all
Copy link
Contributor

Choose a reason for hiding this comment

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

:show_nav hat nichts mit LabelFormat zu tun und braucht es wohl auch nicht, wenn sowieso alle die 'Einstellungen' sehen können.
Die übrigen Actions können wie sonst auch mit :manage zusammengefasst werden. Allerdings ist all falsch, hier braucht es eine neue Methode, z.B. own, welche die user_id prüft.

@@ -15,6 +15,8 @@ class VariousAbility < AbilityDsl::Base
on(LabelFormat) do
class_side(:index).if_admin
permission(:admin).may(:manage).all
permission(:any).may(:show_nav, :index, :show, :edit, :destroy, :create).all
permission(:admin).may(:create_global).all
Copy link
Contributor

Choose a reason for hiding this comment

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

Ein class_side(:manage_global).if_admin trifft das besser.

@@ -13,10 +13,39 @@ class LabelFormatsController < SimpleCrudController
self.sort_mappings = { name: 'label_format_translations.name',
dimensions: %w(count_horizontal count_vertical) }

helper_method :personal_entries
Copy link
Contributor

Choose a reason for hiding this comment

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

Wie sonst auch via Instanzvariablen mit der View kommunizieren, nicht mit Helper Methoden. Also z.B.

before_render_index :load_personal_entries

def load_personal_entries
  @personal_entries = ...
end

if normal_user? || (admin_user? && current_user_set?)
entry.update(user_id: current_user.id)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Dafür die assign_attributes Methode überschreiben und nicht zweimal auf der DB updaten. Berechtigung etwas einfacher mit params[:global] && can?(:create, LabelFormat.new) prüfen.


def personal_entries
LabelFormat.where(user_id: current_user.id)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Das käme in die list_entries Methode: super.list.where(user_id: current_user.id). Da die persönlichen Formate nun eigentlich die Standardliste sind, die globale Liste via load_global_entries laden. .list Scope nicht vergessen!

end
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, machen wir anders. Siehe unten.

- t.col(t.sort_header(:height)) { |p| '%.2f' % p.render_html(p.height) }
- action_col_edit(t)
- action_col_destroy(t)

Copy link
Contributor

Choose a reason for hiding this comment

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

Die Tabelle und Add Button ins _list.html.haml verschieben und die jeweiligen Einträge übergeben. Und die bestehende Methoden verwenden, à la

table(entries, class: 'table table-striped table-hover') do |t|
  t.sortable_attr(:name) { |f| link_to(f.name, label_format_path(f.id)) }
  t.sortable_attrs(:name, :page_size, :landscape, :dimensions, :font_size, :width, :height)
  add_table_actions(t)
end

@@ -4,7 +4,7 @@
-# https://github.com/hitobito/hitobito.

%ul.nav-left-list
- if can?(:index, LabelFormat)
- if can?(:show_nav, LabelFormat)
Copy link
Contributor

Choose a reason for hiding this comment

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

Das bleibt so, geht hier um die LabelFormat Index Action.

.btn-toolbar
%label
%input#admin_toggle{ type: 'checkbox', data: {hide: 'admin_labels'} }
= I18n.t('label_formats.see_own_labels')
Copy link
Contributor

Choose a reason for hiding this comment

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

Soweit ich das verstanden habe, sollte diese Einstellung auch persistiert werden und bei der Anzeige im Export Dropdown Button berücksichtigt werden. Könnte in dem Fall gleich via AJAX auf eine label_formats#show_global Action gepostet werden.

@@ -0,0 +1,5 @@
class AddUserIdToLabelFormat < ActiveRecord::Migration
def change
add_column :label_formats, :user_id, :integer
Copy link
Contributor

Choose a reason for hiding this comment

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

Zusätzliche Spalte people#show_global_label_formats, boolean, default true.

@codez codez assigned dinuu and unassigned codez Dec 7, 2016
CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
# Hitobito Changelog

## Version 1.13
Copy link
Contributor

Choose a reason for hiding this comment

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

Wird 1.14

else
user.update!(display_only_own_label_formats: true)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

update_column verwenden, damit die Timestamps nicht geändert werden (Die Personendaten wurden nicht geändert). Und als Wert direkt params[:display_only_own_label_formats].present?, dann entfällt das if.

label_formats = label_formats.where(user: user)
end
label_formats.list.for_user(user).each do |label_format|
parent.sub_items << Item.new(label_format, export_label_format_path(label_format.id),
Copy link
Contributor

Choose a reason for hiding this comment

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

Würde auch gleich den display_only_own_label_formats? check ins for_user auslagern.

@@ -0,0 +1,2 @@
var onlyOwn = $('#display_only_own_label_formats').get(0).checked;
// do something?
Copy link
Contributor

Choose a reason for hiding this comment

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

Siehe views/group/person_add_requests/_approvers.html.haml + Group::PersonAddRequestIgnoredApproversController für ähnliches Verhalten.

- title ti(:title, :models => models_label)

#main
-if can?(:manage_global, LabelFormat)
Copy link
Contributor

Choose a reason for hiding this comment

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

Dass soll jeder User entscheiden können, nicht nur :manage_global (Bzw alle mit :label_format_settings Berechtigung => Gleiche Berechtigung prüfen wie im verlinkten Controller).

def change
add_column :people, :display_only_own_label_formats, :boolean, default: false
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Migrationen zusammenfassen

end

end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Die Tests sind noch etwas dünn, wenn es keine Rolle spielt, ob sie current_user oder global verwenden...

@codez
Copy link
Contributor

codez commented Dec 21, 2016

spec/controllers/label_formats_controller_spec.rb läuft nicht durch (siehe auch Kommentar oben), show_global_label_formats fehlt im schema.rb.

@codez codez assigned dinuu and unassigned codez Dec 21, 2016
@dinuu dinuu assigned codez and unassigned dinuu Dec 22, 2016
@codez codez assigned dinuu and unassigned codez Jan 3, 2017
@LukasSkywalker LukasSkywalker assigned codez and unassigned dinuu Mar 5, 2017
@codez
Copy link
Contributor

codez commented Mar 7, 2017

Merged by squashing commits

@codez codez closed this Mar 7, 2017
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.

None yet

5 participants