EnviDat / ckanext-restricted

CKAN extension for restricting access to resources
GNU Affero General Public License v3.0
7 stars 26 forks source link

Code cleaned up, pep8 and i18n! #7

Closed m431m closed 5 years ago

m431m commented 5 years ago

Hi,

I propose this pull-request:

Of course, I recommend to test it before merging first.

m431m

espona commented 5 years ago

Thanks for your contribution. There are so many changes I could hardly review everything, I have barely time for this project. I resolved conflicting changes and merged, I guess it works for you. I will test it as soon as I can. Best, L

m431m commented 5 years ago

You're welcome, I have time for review the code. If I find bugs, I would correct them. m431m

espona commented 5 years ago

I just fixed one issue at i18n/ckanext-restricted.pot. I have a problem with access request mails, they no longer display as before. All the mail is in one line without line breaks, can you review this? Many thanks

screen shot 2018-07-31 at 17 29 40
m431m commented 5 years ago

I just fixed one issue at i18n/ckanext-restricted.pot

Bad copy-paste...

I have a problem with access request mails, they no longer display as before. All the mail is in one line without line breaks, can you review this? Many thanks

Of course, I will fix it.

espona commented 5 years ago

Also, no mail is sent after dataset owner adds a new user to the "allowed users". Does this work for you?

m431m commented 5 years ago

Ok, I'll check this tomorrow morning, correct mistakes and make a patch.

espona commented 5 years ago

I just fixed that with the mails sent when a new user is allowed, it was just a missing import. This mails also display without line breaks. Hopefully that is easy to fix too :)

m431m commented 5 years ago

Hi, I found what's wrong with e-mail. The Jinja i18n {% trans %} tag destroy all line break.... I will fix it today.

espona commented 5 years ago

Ok, I guess you need a {% trans %} per line. You can change the mail contents to make it more compact, so that less line breaks are needed, maybe that makes it simpler.

m431m commented 5 years ago

Yes absolutely. I will keep the content without changing it.

m431m commented 5 years ago

Patch here: #8