[WIP] fix missing variables in trigger body content

rosscdh · September 30, 2020, 2:51pm

As discussed in

I need some guidance as to where to source from fields used when you type ::

thorsteneckel · October 5, 2020, 12:54pm

Hey @rosscdh - thanks for your patience and sorry for the delay. I found the time to have a look. The root cause is rather simple. We use NotificationFactory::Mailer.template to perform the replacement of tags (in the body and subject). As you can easily see here for sending notification emails:

github.com

zammad/zammad/blob/9269385acdd223803cd34db7237c3b12d3ed0e79/app/models/ticket.rb#L1615-L1627


# get subject
subject = NotificationFactory::Mailer.template(
  templateInline: value['subject'],
  objects:        objects,
  quote:          false,
)
subject = subject_build(subject)

body = NotificationFactory::Mailer.template(
  templateInline: value['body'],
  objects:        objects,
  quote:          true,
)

and for SMS (a bit different):

github.com

zammad/zammad/blob/9269385acdd223803cd34db7237c3b12d3ed0e79/app/models/ticket.rb#L1723-L1727


body = NotificationFactory::Renderer.new(
  objects:  objects,
  template: value['body'],
  escape:   false
).render.html2text.tr(' ', ' ') # convert non-breaking space to simple space

However, the NotificationFactory::Mailer.template call is missing for the created note article:

github.com

zammad/zammad/blob/9269385acdd223803cd34db7237c3b12d3ed0e79/app/models/ticket.rb#L1023-L1036


Ticket::Article.create!(
  ticket_id:     id,
  subject:       value[:subject],
  content_type:  'text/html',
  body:          value[:body],
  internal:      value[:internal],
  sender:        Ticket::Article::Sender.find_by(name: 'System'),
  type:          Ticket::Article::Type.find_by(name: 'note'),
  preferences:   {
    perform_origin: perform_origin,
  },
  updated_by_id: 1,
  created_by_id: 1,
)

Creating the note should be moved to a dedicated method as it is currently for Email and SMS notification.

You can extend the existing tests in spec/models/ticket_spec.rb (describe '#perform_changes' do) accordingly.

Hope this helps

rosscdh · October 5, 2020, 2:06pm

Thanks for the direction, yes that helps allot.

I just dont understand what you mean by move to another method like email and sms as neither calls are in a dedicated method (afaics) ?

I mean im happy to create one, but just wanted to 2x check what you mean

rosscdh · October 5, 2020, 2:09pm

Also, how would i add the article id to the set of :: is there is no way to call the api with the ticket#number i need to populate a trigger with the ticket#id and this is not available atm

rosscdh · October 5, 2020, 2:10pm

ah I think i see…

you meant like in

1053

perform_notification.each do |key, value|

      # send notification
      case key
      when 'notification.sms'
        send_sms_notification(value, article, perform_origin)
        next
      when 'notification.email'
        send_email_notification(value, article, perform_origin)
      end
    end

problem is… in those cases those make sense to make a separate method as they are “send” items.

unless we rewrite it so that we can render the subject and body once! and then reuse them throughout?

thorsteneckel · October 5, 2020, 2:18pm

Yes! Like these. What I meant was just a new add_trigger_note(...) method.

rosscdh · October 5, 2020, 2:26pm

Ok have updated the PR with the requested extraction; need to grok the tests.

I completely agree re the need for refactoring btw… lots of cleanup necessary!

rosscdh · October 5, 2020, 3:38pm

I found some existing tests for article.note that I’ve modified slightly to add inline template variables

pending build https://github.com/zammad/zammad/pull/3225/checks?check_run_id=1210060968

rosscdh · October 5, 2020, 5:08pm

@thorsteneckel is this normal

Mail::AddressList can not parse |"Clement.Si" <Claudia.Shu@yahoo.com.>|: Only able to parse up to "\"Clement.Si\" <Claudia.Shu@yahoo.com."
WARNING: Ignoring unparsable header "by server-9.tower-169.example.com with SMTP; 5 Apr 2017 07:45:30 -0000": invalid header name syntax: "by server-9.tower-169.example.com with SMTP; 5 Apr 2017 07"
WARNING: Ignoring unparsable header "15.0.1263.5; Wed, 5 Apr 2017 09:45:23 +0200": invalid header name syntax: "15.0.1263.5; Wed, 5 Apr 2017 09"
WARNING: Ignoring unparsable header "(TLS) id 15.0.1263.5; Wed, 5 Apr 2017 09:45:23 +0200": invalid header name syntax: "(TLS) id 15.0.1263.5; Wed, 5 Apr 2017 09"

rosscdh · October 5, 2020, 6:38pm

yep seems to be… forgot how strange specs can be

rosscdh · October 5, 2020, 6:48pm

@thorsteneckel is there a reason in the “run” component for zammad-docker-compose nginx is installed?

github.com

zammad/zammad-docker-compose/blob/master/containers/zammad/setup.sh#L8


#!/usr/bin/env bash
set -e

# install dependencies
if [ "$1" = 'install' ]; then
  PACKAGES="build-essential curl git libimlib2-dev libpq-dev"
elif [ "$1" = 'run' ]; then
  PACKAGES="curl libimlib2 libpq5 nginx rsync"
fi

apt-get update
apt-get upgrade -y
# shellcheck disable=SC2086
apt-get install -y --no-install-recommends ${PACKAGES}
rm -rf /var/lib/apt/lists/*

# install gosu
if [ "$1" = 'install' ]; then

seems a bit of a waste of space? as nginx runs as a separate docker container?

thorsteneckel · October 6, 2020, 7:56am

@monotek is the master of the zammad-docker-compose setup. Can you help out here?

monotek · October 6, 2020, 9:38am

One docker image with the zammad sources is build, using several entrypoints. Nginx is one of them.

See: https://github.com/zammad/zammad-docker-compose/blob/master/containers/zammad/docker-entrypoint.sh#L121

rosscdh · October 6, 2020, 10:31am

Hey @monotek, yes am aware have been reviewing it in detail recently.

Please look at https://github.com/zammad/zammad-docker-compose/blob/master/containers/zammad/setup.sh#L8

on “run” nginx gets installed. i cant really see the point of this as you can just refer to the official nginx image and save on image size.

I have several bits of feedback on the docker image build process but its a topic for another time

My main painpoint atm is id like a developer image with the right ruby etc installed that i can just mount the /app from my local source into and run the bundle exec db:migrate etc… on demand as a developer (i dont really want to install all the ruby stuff on my local machine)

I invested a bit of time on the topic last night and it would def work; but was wondering if there was somethign “official” before i go off on a tangent

monotek · October 6, 2020, 11:18am

If you use standard nginx image the zammad sources are missing, so there are no assets to deliver to the browser.

See: https://github.com/zammad/zammad/blob/develop/contrib/nginx/zammad.conf#L22

rosscdh · October 6, 2020, 3:03pm

yeah the workflow becomes multi-step
but usually i deal with it as a shared volume mounted by nginx and the app server and the precompile:assets step runs on the railsserver and outputs to the shared volume.

implies that more work is dont with docker-compose exec zammad-railsserver bundle exec precompile:assets - which when the application needs to move to the cloud (kube, eks) and PVCs is a topic that really needs to be addressed.

or you prepopulate custom nginx images with version tagged assets but i prefer the previous option as you can then on demand deploy a new container and as an initContainer step run the assets precompile which then populates the volume/s3 bucket/pvc…

lots of options… all fine

rosscdh · October 6, 2020, 3:05pm

basically im going to spend a bit more time trying to get a docker image running in the process described so that i can run at least the tests on the image… and hopefully finish off https://github.com/zammad/zammad/pull/3225 which is close now

monotek · October 6, 2020, 5:39pm

Not sure if separate nginx alpine image would save space compared to the nginx package inside the container. Do you like to test and compare?

But yes, using the separate nginx image would work, as long as you run it in the same pod in kubernetes so you can mount the same volume. In docker-compose it should work too.

If you consider image space, don’t waste time trying to run zamad in alpine as zammad is currently incompatible to alpine because of https://github.com/docker-library/ruby/issues/113

If you want a all in one docker image for development and testing you can use:

rosscdh · October 6, 2020, 8:53pm

Yep completely agree; I like you guys tend to use slim as we need access to all the goodies there and its small enough and saves the fumbling around with apk and package versions which is always painful.

Atm its fine the way it is i think; One thing i had success with was using caddy over nginx tls etc a little easier but only for simple static assets.

thanks for the tip re zammad docker! will take a look!

puma and unicorn… never did discover which one won the race but am glad passenger is outta the show!

rosscdh · October 7, 2020, 7:49am

@thorsteneckel tests in the PR are looking green (the specific test tested locally)

root@e4b54dfaa8f6:/opt/zammad# bundle exec rails test test/unit/ticket_trigger_test.rb:4499
Run options: --seed 51762

# Running:

.

Finished in 1.618186s, 0.6180 runs/s, 24.1011 assertions/s.
1 runs, 39 assertions, 0 failures, 0 errors, 0 skips

    assert_match("some subject! #{ticket1.id}", article_note1.subject)
    assert_match("I can integrate with 3rd party services at <a href=\"https://my.saas/foo/#{ticket1.id}\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">https://my.saas/foo/#{ticket1.id}</a>", article_note1.body)

tests that the trigger gets objects substituted

I believe this PR is now ready for [REVIEW] please