@mahendra Some comments: - regarding the test email feature, while iterating through a few probable real life scenarios, I would assume it's ok to fire the email to whoever is in the recipient field. Alternatively, bringing up a modal that asks which addresses to send to that defaults to the recipients in the schedule would probably be optimal. Personally I would prefer to see this already be part of the current PR, as it is the natural first step of creating an email schedule: first test, then schedule. But if it is a lot of extra work then probably ok to wait for extra PR. - custom resolution in separate PR for sure. - One final comment is regarding the lack of transparency regarding when the schedule is updated. I think the optimal user experience would be for the schedule to be updated every time a schedule is updated (perhaps even a `config.py` setting for choosing to update hourly or at every update), but if this is difficult to achieve then a small note somewhere near the cron schedule is defined is probably fine.
At any rate, I think the feature is excellent and the code clean (apart from the selenium retries which aren't your fault). We will be gradually deploying this into production shortly, so if we have any problems I will be sure to let you know! [ Full content available at: https://github.com/apache/incubator-superset/pull/5294 ] This message was relayed via gitbox.apache.org for [email protected]
