search

ioos / ckanext-ioos-theme

IOOS Catalog as a CKAN extension
GNU Affero General Public License v3.0
7 stars 14 forks source link

Adds feedback #150

Closed lukecampbell closed 7 years ago

lukecampbell commented 7 years ago

This commit adds two new schema items: smtp.port (not sure why we didn't have it before). As well as feedback.recipients which is a space delimited list of email addresses that will receive feedback.

This commit adds a new controller called FeedbackController which simply renders the "Submit Feedback" form and passes the template the data.

This commit adds a new library for mailing based on Flask-Mail. In my opinion, Flask-Mail deals with far more edge cases for email than the existing CKAN mailer.

To disable feedback leave the recipients entry blank.

There's also a template for the body of the feedback emails.

This PR will address https://github.com/ioos/catalog-ckan/issues/144

screen shot 2017-04-19 at 5 33 26 pm

lukecampbell commented 7 years ago

I still need to do some code cleanup and some docstrings, but there it is... It was about as hard as I thought it was going to be. This diagram really helped me make sense of CKAN:

CKAN

lukecampbell commented 7 years ago

The really good news, is now I think I get how CKAN works...

lukecampbell commented 7 years ago

Oh also, in addition to the commit message, I replaced the "Groups" navlink with "Feedback".

mwengren commented 7 years ago

Can we make the feedback button tied to a dataset, instead of generic across the site?

I'd rather the feedback button go in the dataset tab area, just before the 'Groups' tab, and auto-submit a hidden field(s) with the dataset URL, and possibly Title as well, so when the email gets generated, we can bundle that info along with whatever the feedback comment is. Otherwise, we're going to be having a lot of back and forth: 'Which _____ dataset are you referring to', etc.

But, we can probably remove the site-wide 'Groups' tab where you've replaced it with the current Feedback tab anyway, I'm not sure we'll be using that in CKAN and it just confuses the navigation as is.

lukecampbell commented 7 years ago

Issue left open