distributev / the-app

The App
MIT License
0 stars 2 forks source link

mail-merge-backend questions #82

Open luismanuel001 opened 8 years ago

luismanuel001 commented 8 years ago

On the requirements it says "Move the existing config/mail-merge/*. configuration files to flows/005 - mail-merges/templates", but on the actual file structure the "flows/005 - mail-merges/templates" is under "/theapp-template/". Should it be: "/theapp-template/TheApp/flows/005 - mail-merges/templates" instead?

luismanuel001 commented 8 years ago

Also, is there any specific reason behind the "005 - mail-merges" folder name? I think it might be better to call it simply "mail-merge" so the full path becomes "/theapp-template/TheApp/flows/mail-merges/templates".

This way we can have dynamic api endpoints like

/api/flows/mail-merge/invoices/config
/api/flows/mail-merge/invoices/form
/api/flows/mail-merge/invoices/email

Where mail-merge and invoices are dynamic:

/api/flows/{{flowName}}/{{template}}/config
/api/flows/{{flowName}}/{{template}}/form
/api/flows/{{flowName}}/{{template}}/email

PS: It is also best practce not having spaces on filepaths for cross-platform compatibility

luismanuel001 commented 8 years ago

The template config.json files from ng-mail-merge-directive are different than the provided on the requirements /005 - mail-merge/ (some properties have slightly different property names and missing properties). Which ones should I use?

distributev commented 8 years ago

The new one from requirements /005 - mail-merge/ - you have it commented so you understand what each item is doing. Table structure is also different.

If you find any item from old config which looks important to you and does not have equivalent in the new config structure just let me know.

The GUI should be more or less constant (not sure if 100% but most part yes).

luismanuel001 commented 8 years ago

On the project requirements you proposed this structure for the mailMergeData object that the createMailMerge API expects as input:

var mailMergeData = {
  recipientCode: "C0001", //save to flows_data.code1
  templatePath: "invoices/config.json", //save to flows_data.type2
  //this form information is dynamic - each different template (i.e. invoices, payslips, etc.) will have it's own form structure 
  form_vars: {
        email.to: "to@to.com",
        email.cc: "cc@cc.com",
        email.bcc: "bcc@bcc.com",
        email.subject: "Email Subject Test",
        code: "C0001",
        name: "Customer Name",
        date: "20 July 2016",
        number: "1",
        product: "Product Name",
        fees: "123"
    }
};

But on the main-merge directive code, the current mergeService.create(data) function sends the following data to the API:

{
  "merges": [
    {
      "template": "Invoices",
      "document": {
        "initialstatus": "unpaid",
        "generatepdf": true,
        "filename": "valid{{ng}file-name-template.pdf",
        "output_folder": "output/valid{{ng}}folder-name-template",
        "html": "<<COMPILED_HTML>>",
        "html_permalink": "/documents/invoices/{{merge.date| date:'yyyy-MM' }}/{{merge.recipientcode}}",
        "pdf_permalink": "/documents/invoices/{{merge.date| date:'yyyy-MM' }}/{{merge.recipientcode}}.pdf",
        "zip_permalink": "/documents/invoices/{{merge.date| date:'yyyy-MM' }}/{{merge.recipientcode}}.pdf",
        "authrequired": false
      },
      "email": {
        "sendemail": true,
        "sendhtmlemail": true,
        "to": "to10@email.com",
        "cc": "cc10@email.com",
        "bcc": "bcc10@email.com",
        "subject": "Subject10",
        "text": "Hello,how are you Name10, did you receive my invoice on 2016-12-18",
        "html": "<<COMPILED_HTML>>",
        "attachments": [
          "{{pdf_file_path}}",
          "C:/test/hardcoded-file.txt"
        ]
      },
      "smtp": {
        "host": "Email Server Host",
        "port": "25",
        "userid": "From Email User ID",
        "userpassword": "From Email Password",
        "usessl": false,
        "usetls": false,
        "debug": false,
        "fromaddress": "from@emailaddress.com",
        "name": "From Name"
      },
      "form": "form.json",
      "recipientcode": "code10",
      "status": "kue"
    }
  ]
}

Should I modify the mail-merge directive to send a mailMergeData object like you proposed, by just mapping the required fields from the object currently sent, and remove all other data since most of that data is already present on the server on the respective {{template}}/config.json?

distributev commented 8 years ago

Yes you are right - modify the directive to send the new (slimmed) data structure.

Initially the configuration was available only on /client and not on /server ==> the client had to compile and send everything to the server.

But now the /server has access to full configuration. Which means that the /client should send only the minimum required data which is

  1. the code (even this I think can be left out since, by convention, always the first element of the parsed form is the code) and
  2. the template path and
  3. the parsed form
luismanuel001 commented 8 years ago

On requirements/05 - database-structure.txt the flows_data table schema is specified, but I think we are missing fields for saving the compiled html_permalink, pdf_permalink and zip_permalink. Please indicate how should I add this.

distributev commented 8 years ago

05 - database-structure.txt says

additional_data2: DataTypes.STRING, // store mailMergeData / templates (document and email) compiled using ng-node-compile on server using https://www.npmjs.com/package/ng-node-compile -

and 15 - API - compiledMailMergeInfo getMailMergeHistory (recipientCode) shows full compiledMailMergeInfo which should be stored in additional_data2 (including the additional_vars section)

P.S - Don't forget about Date related variables should also be available to be used in templates (email, document) which are explained in the same file

luismanuel001 commented 8 years ago

yes thats all fine, and I know the variables are going to be available on the additional_data2 field, but I think it will help to have the the html_permalink as a direct column on the flows_data for when we need to query it in order to serve the document from that url using express.

The /frontend dynamic router needs to query the database to check if the given url is in a flows_data record in order to serve it, so wouldn't it make more sense to have it as a column field?

So we can run something like Flow.where({ html_permalink: url }), instead of having to run a Flow.findAll() and then parse the additional_data2 of each entry.

distributev commented 8 years ago

For this specific reason it will make more sense to put separate columns for html_permalink, etc which will make your query simpler.

But flows_data table is supposed to store data for any kind of flows not just mail-merge (i.e. mail-merge which we are doing now it's only a type of flow - other will come) ==> the existing structure of flows_data is generic to be reused across flows. The aim is not to have in this table fields / columns hard-coded specifically for mail-merge (i.e. html_permalink, pdf_permalink, zip_permalink) but generic ones applicable cross flows. Specific flows data (i.e. mail-merge data) is stored as json in additional_data1, additional_data2 and other fields documented in 05 - database-structure. You can still query the permalinks from additional_data2 with an extra step to parse the returned JSON structure.

distributev commented 8 years ago

It's possible to register dynamically when the server starts all the routes (i.e. html_permalink, pdf_permalink, zip_permalink) found in flows_data.additional_data2 for (flows_data.type1 === "mail-merge")

luismanuel001 commented 8 years ago

For the mailMergeCsvFile(csvFilePath) feature, should the API accept the csvData instead of the csvFilePath? Since the user is going to be referencing a file on their system and not on the server, I think we should put a file input on the front end, read the file's content data, and send the content data (not the file path) to the mailMergeCsvFile API to start the mail-merge flow process.

Re:

This is a very good question.

There are 2 possible scenarios

  1. User will upload CSV files from the browser
  2. CSV files will be feeded directly to the server (by a human or by an automated process which will place the CSV files into a pre-defined folder on the server)

In both situations the expectation is that the CSV files will be (really) huge. The implemented solution should cover both of the above scenarios with a single code base (the code to handle CSV files from browser or from server should be the same for both situations).

The complete solution will come with the yet to be implemented scriptable-node-js project and will be based on a file system watcher (based on https://github.com/paulmillr/chokidar) - the file system watcher will be configured to "watch" a pre-defined folder on the server for new *.csv files. When a new CSV file is dropped into the watched folder it will then start to "stream" the CSV file and mail merge all the data rows which will be found - papa parse (for browser) and baby parse (for server) both have the feature of streaming CSV files in order to support huge volumes. Your API should use the baby parse into the "streaming" mode.

To answer your question the existing API should be mailMergeCsvFile(csvFilePath) - the csvFilePath is indeed on server and you should use baby parse into the "streaming" mode.

In a subsequent scriptable-node-js project a chockidar watcher will be configured to watch a server folder for new CSV files and will execute your code when a new CSV file is found. In the browser it will indeed be a file input where the user will upload CSV files. The upload will be configured into the previous "watched" folder on the server. Probably the upload process should also be "streamed" otherwise the browser will hang when the users will try to upload really huge CSV files.

luismanuel001 commented 8 years ago

For the email job, it wasn't specified on the requirements that I needed to implement the compress into a zip of the attachments. Should I implement this? should I default to zip all files if email.attachments.length > 1? How should I handle the case when some files specified at the attachments: [ ] are not found (don't exist on the server), should I zip/send only the found files or fail the entire job flow?

Re:

Indeed if email.attachments.length > 1 then zip all attachments and upload the resulted zip file to blob2 column and make that zip file available for download through zip_permalink

In the 04 - commented config.json it says "zip_permalink": /* frontend URL from where to download the generated ZIP (if count(attachments)>1 */

In 05 - database-structure.txt it says blob2:DataTypes.BLOB, // if the email has count(.attachments)>1 save in this column the .zip BLOB with all files attached to this specific mail-merge (document.zip_permalink should start to download this)

How should I handle the case when some files specified at the attachments: [ ]?

If some files from attachments: [ ] are not found then fail the corresponding merge data row (only that row of data) and put the relevant kue job(s) on failed status - put the status in the database as failed - save the stack trace error into the corresponding job(s) and database columns. Fail only the corresponding row of data - continue to process next rows of data (from the CSV or from the GUI back - next buttons).