search

collective / Products.PasswordStrength

Adds verification rules for user passwords in plone.
http://plone.org/products/passwordstrength
Other
5 stars 4 forks source link

4.3 #2

Closed sgeulette closed 10 years ago

sgeulette commented 10 years ago

Merged 4.3 branch with master last changes. Added buildout and robot tests. Don't skip password validation for manager. Skip password validation for generated password. Added french translation. Corrected UnicodeDecodeError.

sgeulette commented 10 years ago

@djay Hi Dylan, could you review the changes ? Thanks in advance.

djay commented 10 years ago

Hi, I will review it in the next couple of days. Would it also be possible to get it ready to run on travis?

gbastien commented 10 years ago

@djay

hi, just a little reminder ;-) as we would like this to be reviewed/merged and to have a new release as soon as possible.

Thank you!

Gauthier

gbastien commented 10 years ago

Hi @djay sorry to insist ;-) :-p

Thank you!

Gauthier

do3cc commented 10 years ago

Why does the package contain translations for the plone.po domain, couldn't all translations stay in Products.PasswordStrength?

do3cc commented 10 years ago

I had a look at the changes, I think there are two problems right now. But I am not a user of the package, I don't know if the monkey patches on their own make sense, so I don't feel confident to merge it on my own if the issues I noted are cleared. I think @pysailor should be qualified too.

djay commented 10 years ago

@gbastien I'd really like to see tests running on travis before it gets merged. Since there are monkey patches involved that should only be used on versions prior to Plone 4.3, we really need to test this.

djay commented 10 years ago

@gbastien I think what you need to do is submit a smaller pull request with just travis and tests and we'll merge that. We'll enable travis and then the branch will be tested. Can @hvelarde or someone else enable travis for this package pretty please?

sgeulette commented 10 years ago

@djay @hvelarde I already added travis in my fork. It has passed several times but is complaining today (https://travis-ci.org/IMIO/Products.PasswordStrength). I don't understand why because it's running well locally and the changes made today aren't related.

djay commented 10 years ago

You only seem to be testing on 4.3, instead of also on 4.2, 4.1 and 4.0. Also it would be good if you can submit a pull request with only travis and the tests so we can see what works before the merge.

sgeulette commented 10 years ago

@djay Isn't it easier to merge the code in the 4.3 branch, run the tests and adapt the code if necessary ? I tried to remove Password.validate patch but the inline validation doesn't work anymore in 4.3.3...

gbastien commented 10 years ago

Hi @djay @hvelarde, what do you think about going on with this? We would like to sprint on this at the next Plone Conference, but it would be nice to have a merge/new release as soon as possible and to continue work on merged code. Last release is one year old.

Thank you,

Gauthier

hvelarde commented 10 years ago

I just enabled the Travis CI hook on this repo.

gbastien commented 10 years ago

@djay, @hvelarde,

OK, ready to merge so? :-p

Gauthier

djay commented 10 years ago

It hasn't tested the branch against travis yet since travis was enabled after you made the pull request. if you make another commit to this branch it might trigger a test. We are going to need a version that works for Plone 3 - 4.3 before we do a release.

hvelarde commented 10 years ago

:+1:

sgeulette commented 10 years ago

Hi djay, I will probably work on Products.PasswordStrength during the conference sprint. I see you want a version working on Plone 3 - 4.3 but the written tests made with robotframework will only work on Plone 4. In that case, isn't it better to limit the new version only to Plone 4 ? Stephan

djay commented 10 years ago

We had a requirement to have it work on plone 3 but that recently went away so I'm all for 4+ now :) On 31 Oct 2014 15:42, "Stéphan Geulette" notifications@github.com wrote:

Hi djay, I will probably work on Products.PasswordStrength during the conference sprint. I see you want a version working on Plone 3 - 4.3 but the written tests made with robotframework will only work on Plone 4. In that case, isn't it better to limit the new version only to Plone 4 ? Stephan

— Reply to this email directly or view it on GitHub https://github.com/collective/Products.PasswordStrength/pull/2#issuecomment-61279711 .

sgeulette commented 10 years ago

Great, do you think it's necessary to test on travis each Plone 4 version ? Or only with 4.3 ?

djay commented 10 years ago

On 31 Oct 2014, at 10:52 pm, Stéphan Geulette notifications@github.com wrote:

Great, do you think it's necessary to test on travis each Plone 4 version ? Or only with 4.3 ?

yes absoluely test each version. Since pre-4.3 we need to still include the monkey patches and we needs some conditional zcml to do this. If you want to ask any questions I'm in the devon room with Martins group.

macagua commented 10 years ago

@djay just a question: the master branch will be merge later to next release ?

djay commented 10 years ago

@macagua what we need to do is reverse this deletion - https://github.com/collective/Products.PasswordStrength/commit/4005a8caa53ba2cf0c8d23dd8456aac957932355 and make it conditional on < 4.3 plone versions. It would be great to make the new features available to 4.1 and 4.2 at least, and then release a single 4.1-4.3 compatible version