Add alternative organizations to user by egorovdunice · Pull Request #1 · egorovdunice/zammad

egorovdunice · 2018-04-26T15:04:17Z

No description provided.

thorsteneckel

I just reviewed your changes and added some inline comments. There are two other broader topics I want to cover here because they are related to multiple files:

Changes in generic files

There are quite a lot of changes to frontend files which meant to be generic. They now contain special organization logic which is not the way we should solve this requirement. Here is a list of related files:

app/assets/javascripts/app/controllers/_application_controller_form.coffee
app/assets/javascripts/app/controllers/_ui_element/autocompletion_ajax.coffee
app/assets/javascripts/app/controllers/_ui_element/select.coffee
app/assets/javascripts/app/lib/app_post/searchable_select.coffee
app/assets/javascripts/app/lib/bootstrap/transition.js
app/assets/javascripts/app/views/agent_ticket_create.jst.eco
app/assets/javascripts/app/views/customer_ticket_create.jst.eco
app/assets/javascripts/app/views/generic/searchable_select.jst.eco
app/assets/stylesheets/zammad.scss

I think the issue here is that you changed the existing generic files instead of inheriting from them and create a new custom element for the organization requirements (like e.g. app/assets/javascripts/app/lib/app_post/z_searchable_ajax_select.coffee does). This should resolve all of those file conflicts.

Debugging code

There are some changes and added files that I think are related to debugging. To be honest I don't know which are and which are not. So I added comments to the ones where I'm not sure. Would be a great help if you could mark them with TODO: DEBUGGING or something so I can get an idea what your thoughts where while adding/changing things.

There are some other things we need to get straight before merging (e.g. only one migration) but it's ok for now.

To be honest I don't have tested it yet because I think the required changes I described above might solve/introduce some bugs. After you changed things I'm happy to start testing.

Feel free to ask any questions!

thorsteneckel · 2018-05-02T11:05:04Z

app/assets/javascripts/app/controllers/customer_ticket_create.coffee

      sender_id:    sender.id
      form_id:      @form_id
      content_type: 'text/html'
+      organization_id: +params.organization_id


The + in front of params looks wrong to me.

thorsteneckel · 2018-05-02T11:06:02Z

app/assets/javascripts/app/lib/app_post/searchable_select.coffee

+        js_shadow_ids.each( -> ids.push $(this).val() )
+      if !ids.includes(data_value) && $(".js-shadow[name='organization_id']").val() != data_value
+        newInput = @input.closest('.searchableSelect').find('div.items')
+        input =  "<input class='searchableSelect-shadow form-control js-shadow-ids' name='organization_ids'>"


Please use templates for HTML structures and don't build them in JS.

thorsteneckel · 2018-05-02T11:07:03Z

app/assets/javascripts/app/models/ticket.coffee

      { name: 'title',                    display: 'Title',        tag: 'input',    type: 'text', limit: 100, null: false },
      { name: 'customer_id',              display: 'Customer',     tag: 'input',    type: 'text', limit: 100, null: false, autocapitalize: false, relation: 'User' },
-      { name: 'organization_id',          display: 'Organization', tag: 'select',   relation: 'Organization', readonly: 1 },
+#      { name: 'organization_id',          display: 'Organization', tag: 'select',   relation: 'Organization', readonly: 1 },


A ticket should still be assigned to one Organization.

thorsteneckel · 2018-05-02T11:08:16Z

app/controllers/long_polling_controller.rb

          return
        end
-        8.times do
+        28.times do


What's the purpose of this increased value?

thorsteneckel · 2018-05-02T11:09:10Z

app/controllers/tickets_controller.rb


    # overwrite params
    if !current_user.permissions?('ticket.agent')
-      %i[owner owner_id customer customer_id organization organization_id preferences].each do |key|


Why did you remove the organization_id key?

thorsteneckel · 2018-05-02T11:11:42Z

db/seeds/object_manager_attributes.rb

        filter: Ticket::State.by_category(:viewable_agent_new).pluck(:id),
      },
      'ticket.customer' => {
-        item_class: 'column',


Why was this line removed?

it's needed to fit 2 select (Ticket.state and Ticket.organization_id) in 1 row

thorsteneckel · 2018-05-23T07:48:29Z

app/assets/javascripts/app/controllers/_application_controller_form.coffee

 class App.ControllerForm extends App.Controller
  constructor: (params) ->
    super
+    if params['screen'] == 'create_middle_org'


Please don't introduce hard coded attribute logic into generic files.

thorsteneckel · 2018-05-23T07:49:58Z

app/assets/javascripts/app/controllers/_ui_element/select_organization.coffee

@@ -0,0 +1,5 @@
+class App.UiElement.select_organization extends App.UiElement.select
+  @render: (attribute, params) ->
+    if attribute['name'] == 'organization_id' && attribute['default'] == 0


Why is there a check for the attribute name if the name of the class is already limited to the attribute?

thorsteneckel · 2018-05-23T07:50:26Z

app/assets/javascripts/app/controllers/agent_ticket_create.coffee

+      el:             @$('.ticket-form-organization')
+      form_id:        @formId
+      model:          App.Ticket
+      screen:         'create_middle_org'


See comment above.

thorsteneckel · 2018-05-23T07:50:45Z

app/assets/javascripts/app/controllers/customer_ticket_create.coffee

+      el:             @el.find('.ticket-form-organization')
+      form_id:        @form_id
+      model:          App.Ticket
+      screen:         'create_middle_org'


See comment above.

thorsteneckel · 2018-05-23T07:52:33Z

app/assets/javascripts/app/lib/app_post/searchable_select.coffee

    @shadowInput.val event.currentTarget.getAttribute('data-value')
    @shadowInput.trigger('change')

+


Please remove empty lines.

thorsteneckel · 2018-05-23T07:56:59Z

app/models/organization.rb

  include Organization::SearchIndex

  has_many                :members,  class_name: 'User'
+  has_and_belongs_to_many :members,   after_add: :cache_update, after_remove: :cache_update, class_name: 'User'


There are two associations with the same name members which looks wrong to me.

thorsteneckel · 2018-05-23T07:58:12Z

config/database.yml.pkgr

  encoding: utf8
  username: zammad
-  password:
+  password:


Please revert this changes.

thorsteneckel · 2018-05-23T07:58:32Z

db/migrate/20180418140353_add_input_to_ticket_create.rb

+        active: true,
+
+        screens: {
+          create_middle_org: {


See first comment.

Uniq screen for new field as 'create_middle', 'create_top' and etc.

If I would use generic hooks then on ticket create form there appear wrong additional fields with the same hooks.

Then we need to change the priority of those so that the order matches the expectations.

thorsteneckel · 2018-05-23T07:59:10Z

app/assets/stylesheets/zammad.scss

  }

  .dropdown-menu {
-    margin-top: -3px;


Why were these changes required? Looks like they might break other parts of the application.

thorsteneckel · 2018-05-23T07:59:55Z

db/seeds/object_manager_attributes.rb

+    active: true,
+
+    screens: {
+        create_middle_org: {


See first comment.

Uniq screen for new field as 'create_middle', 'create_top' and etc.

See comments above regarding generic hooks.

If I would use generic hooks then on ticket create form there appear wrong additional fields with the same hooks.

Then we need to change the priority of those so that the order matches the expectations.

…ork select logic

…ults

thorsteneckel · 2018-05-30T08:56:19Z

app/assets/javascripts/app/lib/app_post/z_searchable_multiple_select.coffee

+    ids = []
+    if js_shadow_ids.length > 0
+      js_shadow_ids.each( -> ids.push $(this).val() )
+    unless ids.includes(data_value) || $(".js-shadow[name='organization_id']").val() == data_value


Please don't introduce hard coded attribute logic into generic files (organization_id ).

thorsteneckel · 2018-05-30T09:21:37Z

app/assets/javascripts/app/views/agent_ticket_create.jst.eco


            <div class="formset-inset">
              <div class="ticket-form-middle horizontal two-columns"></div>
+              <div class="ticket-form-organization horizontal two-columns"></div>


The field should use the existing generic hooks (ticket-form-middle in this case) as the other attributes do.

thorsteneckel · 2018-05-30T09:22:05Z

app/assets/javascripts/app/views/customer_ticket_create.jst.eco

-              <div class="ticket-form-middle horizontal two-columns"></div>
+              <div class="two-columns horizontal">
+                <div class="ticket-form-middle column"></div>
+                <div class="ticket-form-organization column"></div>


Same here (generic hooks).

thorsteneckel · 2018-05-30T09:22:45Z

app/assets/javascripts/app/views/generic/searchable_multiple_select.jst.eco

+  <% if @attribute.value: %>
+    <% for value in @attribute.value: %>
+      <div>
+        <span class='selected_organization' onclick="$(this).closest('div').remove()">


Please don't introduce hard coded attribute logic into generic files (organization).

thorsteneckel · 2018-05-30T09:23:00Z

app/assets/javascripts/app/views/generic/searchable_multiple_select_item.jst.eco

@@ -0,0 +1,4 @@
+<div>
+    <span class='selected_organization' onclick="$(this).closest('div').remove()"><%= @title %></span>
+    <input class='searchableSelect-shadow form-control js-shadow-ids' name='organization_ids' value=<%= @data_value %>>


Please don't introduce hard coded attribute logic into generic files (organization_ids).

thorsteneckel · 2018-05-30T09:23:48Z

app/assets/stylesheets/zammad.scss

 .searchableSelect {
  position: relative;

+  .selected_organization {


What is required for? There should be no hard coded attribute logic in generic files (selected_organization).

thorsteneckel · 2018-05-30T09:27:17Z

db/seeds/object_manager_attributes.rb

+    active: true,
+
+    screens: {
+        create_middle_org: {


See comments above regarding generic hooks.

thorsteneckel · 2018-05-30T09:36:45Z

spec/models/ticket_spec.rb

+    end
+  end
+
+  describe '#check_defaults' do


Please don't test private/internal methods because they should be easy to change. Instead test functionality. You might want to use run_callbacks instead.

Indentation corrected

thorsteneckel · 2018-07-03T12:22:52Z

db/seeds/object_manager_attributes.rb

+    active: true,
+
+    screens: {
+        create_middle_org: {


Then we need to change the priority of those so that the order matches the expectations.

thorsteneckel · 2018-07-03T12:23:00Z

db/migrate/20180418140353_add_input_to_ticket_create.rb

+        active: true,
+
+        screens: {
+          create_middle_org: {


Then we need to change the priority of those so that the order matches the expectations.

thorsteneckel · 2018-07-03T12:24:26Z

app/assets/javascripts/app/lib/app_post/searchable_select.coffee

    @input.trigger('change')
    @shadowInput.val event.currentTarget.getAttribute('data-value')
    @shadowInput.trigger('change')
+    $(".js-shadow-ids[value=#{event.currentTarget.getAttribute('data-value')}]").closest('div').remove()


What is this line for?

If we keep it:
It's not really expressive. We should add a comment for it and extend the tests to cover the functionality.

thorsteneckel · 2018-07-03T12:25:28Z

app/assets/javascripts/app/lib/app_post/searchable_select.coffee

    '.js-input':                  'input'
    '.js-shadow':                 'shadowInput'
    '.js-optionsList':            'optionsList'
+    '.items':                     'itemList'


This is only used in a descendant of the class so there is no need to have it in the base class.

thorsteneckel · 2018-07-03T12:27:51Z

app/assets/javascripts/app/lib/app_post/z_searchable_multiple_select.coffee

+    ids = []
+    if js_shadow_ids.length > 0
+      js_shadow_ids.each( -> ids.push $(this).val() )
+    unless ids.includes(data_value) || $(".js-shadow[name='"+@attribute.name+"']").val() == data_value


Please don't use unless statements.

thorsteneckel · 2018-07-03T12:28:36Z

app/assets/javascripts/app/views/generic/searchable_multiple_select.jst.eco

+  <% if @attribute.value: %>
+    <% for value in @attribute.value: %>
+      <div>
+        <span class='selected_item' onclick="$(this).closest('div').remove()">


Please don't use inline JS. Move it to the controller that handles the template.

thorsteneckel · 2018-07-03T12:29:07Z

app/assets/javascripts/app/views/generic/searchable_multiple_select_item.jst.eco

@@ -0,0 +1,4 @@
+<div>
+    <span class='selected_item' onclick="$(this).closest('div').remove()"><%= @title %></span>


Please don't use inline JS. Move it to the controller that handles the template.

thorsteneckel · 2018-07-03T12:30:02Z

app/models/ticket.rb

    return true if !customer
-    return true if organization_id == customer.organization_id
-    self.organization_id = customer.organization_id
+    return true if (customer.organization_ids.include?(organization_id) || customer.organization_id? && customer.organization_id == organization_id)


Please split this line/if statment if possible. It carries way to much complexity.

thorsteneckel · 2018-07-03T12:30:15Z

app/models/ticket.rb

-    if !owner_id
-      self.owner_id = 1
-    end
+    self.owner_id = 1 unless owner_id


Please don't use unless.

thorsteneckel · 2018-07-03T12:30:40Z

app/models/user/assets.rb

        end

+        if local_attributes['organization_id']
+          unless data[:Organization] || data[:Organization]&[local_attributes['organization_id']]


Please don't use unless.

… to if.

thorsteneckel · 2018-07-05T09:30:13Z

app/assets/javascripts/app/views/generic/searchable_multiple_select.jst.eco

+    <% for value in @attribute.value: %>
+      <div>
+        <span class='selected_item'>
+          <%- App.Organization.findNative(value).name %>


Please don't use specific code in generic components.

…tate new is set to open on second update by customer.

… if access is via roles

…st specs

…ixes zammad#2235)

…e they are migrated to rspec now).

…stamp e. g. "2018-09-19 15:25".

…k for invalid attribute names (fixes zammad#2236)

…ing bulk action form (fixes zammad#2026)

…te field.

…ixes zammad#2233)

Fixed typo in 404: Ressource -> resource

…Error: undefined method `match' for nil:NilClass>"`.

…eferences[:delivery_status_message] has no utf8 charset.

…eptions::UnprocessableEntity: Invalid email>"`.

…rough HTML nodes.

… Also added hiding of empty string attribute fields.

…nt race conditions (if callback comes later).

…if file is corrupt in filesystem and remove it in case).

…_to_organization_ids_for_user

thorsteneckel suggested changes May 2, 2018

View reviewed changes

thorsteneckel suggested changes May 23, 2018

View reviewed changes

egorovdunice added 5 commits May 24, 2018 15:17

Add class for multiple select with css. Create template and small rew…

50a828d

…ork select logic

Small fixes

06db315

Small rework of check_defaults logic. Added Rspec test for check_defa…

3591b97

…ults

Added Rspec for access_condition

ac671f1

Added selector2sql spec

d37faa6

thorsteneckel suggested changes May 30, 2018

View reviewed changes

martini and others added 9 commits June 5, 2018 16:51

Merge branch 'develop'

3c6f644

Merge branch 'develop'

f1b68cb

Merge branch 'develop'

cbaab06

Update zammad_ssl.conf

0a619bc

Update zammad_ssl.conf

f50bd63

Indentation corrected

Changed controller(el), view(remove div), seed for Ticket creating.

849b469

Small change attribute logic in generic file.

dabd12f

Small change in multiple select attibute logic

47c8d5b

Change RSpec callback checking.

964b94d

thorsteneckel suggested changes Jul 3, 2018

View reviewed changes

egorovdunice added 2 commits July 4, 2018 11:14

Move js from view to controller. Change screen for OMA. Change unless…

8d1fa77

… to if.

Small change in migration for OMA Ticke.organization_id.

0875e3a

thorsteneckel suggested changes Jul 5, 2018

View reviewed changes

egorovdunice and others added 9 commits July 5, 2018 14:32

Move specific code from generic components

63ce018

Devs are responsible for their ENV management themselves.

aeb5dbc

Improve NULL handling to Ticket#selector2sql (fixes zammad#1316)

901cc3f

Add RSpec for newly refactored CanLookup

6ca5054

Rename CanLookup RSpec shared example group to follow existing pattern

113d13c

Updated nokogiri gem - CVE-2018-8048.

dd48e4d

Merge changes from zammad/zammad development

39a98c0

Fixed issue zammad#2118 - A ticket created by customer via web with s…

468b5bf

…tate new is set to open on second update by customer.

Fixes zammad#2088 User doesn't appear in drag&drop overlay in a group…

9cdb0d5

… if access is via roles

martini and others added 30 commits September 19, 2018 09:22

Added missing admin.time_accounting permission entry.

769fb19

Refactored all controller mini tests and migrated them to rspec reque…

29b616d

…st specs

Fixed issue zammad#2235 - Overview setting isn't applied on submit (f…

1efcb9d

…ixes zammad#2235)

Removed obsolete test exeuction of controller tests in travis (becaus…

208ed18

…e they are migrated to rspec now).

Added new pretty_date_format option "timestamp" to show only the time…

dd848ad

…stamp e. g. "2018-09-19 15:25".

Fixed zammad#2236 by adding instance_method_already_implemented? chec…

dc87488

…k for invalid attribute names (fixes zammad#2236)

Fixed issue zammad#2026 by checking for change permission before show…

45fee1b

…ing bulk action form (fixes zammad#2026)

Fixed issue zammad#2245 - Unable to process spam mail with invalid da…

9a71181

…te field.

Fixed issue zammad#2233 - Boolean object set to false is not visible (f…

8bf3ab2

…ixes zammad#2233)

Fixed typo in 404: Ressource -> resource

818f88c

Merge pull request zammad#2251 from mbeijen/404

e6f4343

Fixed typo in 404: Ressource -> resource

Fixed issue zammad#2252 - Unable to process email `"ERROR: #<NoMethod…

1568909

…Error: undefined method `match' for nil:NilClass>"`.

Fixed issue zammad#2253 - Unable to index elasticsearch if article.pr…

567daf0

…eferences[:delivery_status_message] has no utf8 charset.

Fixed issue zammad#2254 - Unable to process spam email `"ERROR: #<Exc…

e31d275

…eptions::UnprocessableEntity: Invalid email>"`.

Added signatureIdentifyByHtml to detect signatures and blockquotes th…

9c02d2e

…rough HTML nodes.

Extended the fix for issue zammad#2233 to cover User objects as well.…

280ab74

… Also added hiding of empty string attribute fields.

Improved quote line, reduced length by removing email address.

cff3dba

Fixed issue zammad#2255 - CTI: Limit used caller log states and preve…

ce9ebe8

…nt race conditions (if callback comes later).

Improved error handling if file already exists in file store (verify …

6c82931

…if file is corrupt in filesystem and remove it in case).

Improved external credential managment via rest controller.

8e99a01

Test and fix time accounting API endpoint bug (replaces zammad#2243)

97b17ca

Merge branch 'master' of https://github.com/zammad/zammad into change…

5705b19

…_to_organization_ids_for_user

Merged with upstream

1bfb7a0

Fixing domain assignment. Adding logging fot testing purpose

67700dd

Added more logs. Added gem for tests

9567720

Changed Ticket check_defaults

059dbb5

Fixed error in check_defaults. Added new logs

a185c98

Testing with rework of user assets

1175940

Investigated workflow

e13cd28

Created new user to check test

168e7cf

		@shadowInput.val event.currentTarget.getAttribute('data-value')
		@shadowInput.trigger('change')

		@@ -0,0 +1,4 @@
		<div>
		<span class='selected_item' onclick="$(this).closest('div').remove()"><%= @title %></span>

Conversation

egorovdunice commented Apr 26, 2018

Uh oh!

thorsteneckel left a comment

Choose a reason for hiding this comment

Changes in generic files

Debugging code

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!