Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test coverages for hashed password #893

Merged
merged 1 commit into from
Apr 15, 2016
Merged

Test coverages for hashed password #893

merged 1 commit into from
Apr 15, 2016

Conversation

Amir-61
Copy link
Contributor

@Amir-61 Amir-61 commented Apr 6, 2016

  • Test coverages for hashed password for replaceAttributes
  • Test coverages for hashed password for updateAttribute

Connect to strongloop/loopback#2029

Notes:

Connect to #896

#896 needs to be landed before this patch (PR#893)

@bajtos: Please review... Thanks!

@Amir-61 Amir-61 added the #review label Apr 6, 2016
@Amir-61 Amir-61 force-pushed the hashed_password_tests branch 3 times, most recently from e5aafe0 to 75b71d0 Compare April 6, 2016 19:46
// Check that it has not been already hashed
if (pos === -1 ||
(plain.substr(0, pos).toUpperCase() !== plain.substr(pos + 1, plain.length)))
this.$password = plain + '-' + plain.toUpperCase();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uff, this is rather difficult to understand. Could you please split the code into smaller steps and save intermediate results into named variables? Something like this:

var hashed = false;
var pos = plain.indexOf('-');
if (pos !== -1) {
  var head = plain.substr(0, pos);
  var tail = plain.substr(pos + 1, plain.length);
  hashed = head.toUpperCase() === tail;
}

if (hashed) return;

this.$password = plain + '-' + plain.toUpperCase();

@bajtos bajtos assigned Amir-61 and unassigned bajtos Apr 7, 2016
@bajtos
Copy link
Member

bajtos commented Apr 7, 2016

Hi @Amir-61, the tests look good in general, I left two minor comments to address. Have you seen the tests fail? I'd like you to check that these new tests would catch the regression which was fixed by dbdf915

@Amir-61
Copy link
Contributor Author

Amir-61 commented Apr 7, 2016

Thanks @bajtos ,

In case if my comment gets lost: #893 (comment)

I believe #896 needs to be landed first.

Have you seen the tests fail?

Yes without the fix the added test was failing. 👍

Thanks!

@Amir-61 Amir-61 assigned bajtos and unassigned Amir-61 Apr 8, 2016
@bajtos
Copy link
Member

bajtos commented Apr 8, 2016

LGTM. Let's get #896 landed first and the re-run the CI builds before landing this one, to make sure they pass.

@bajtos bajtos assigned Amir-61 and unassigned bajtos Apr 8, 2016
@Amir-61 Amir-61 added #wip and removed #review labels Apr 12, 2016
@Amir-61
Copy link
Contributor Author

Amir-61 commented Apr 14, 2016

@slnode test please

* Test coverages for hashed password for replaceAttributes
* Test coverages for hashed password for updateAttribute
@Amir-61
Copy link
Contributor Author

Amir-61 commented Apr 15, 2016

Also back-ported the commit to 2.x; I'm so sorry I forgot to add the PR# for this back-porting; I knew I could do git commit --amend and do git push -f, but I preferred not to do it since it rewrites the git history... I'll make sure I'll add the PR# in the upcoming PRs when back-porting...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants