Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

feat: add confirm password field and show passwords feature to change password popUp #92

Merged
merged 15 commits into from
Jul 28, 2020

Conversation

bharat-1809
Copy link
Contributor

@bharat-1809 bharat-1809 commented Jun 6, 2020

Description

Add confirm password field with validation and toggle password visibility button on the change password popUp

Fixes #76

Flutter Channel:

  • I have used the Flutter Beta channel on my local machine

Type of Change:

  • Code

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Physical Device
Steps:

  • Go to the Settings page by clicking on the icon at the top right corner of the homepage.
  • Click on the Change Password button to see whether the feature works properly or not
    flutter_01

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented on my code or provided relevant documentation, particularly in hard-to-understand areas

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • Any dependent changes have been published in downstream modules

@bharat-1809 bharat-1809 closed this Jun 6, 2020
@bharat-1809 bharat-1809 changed the title feat: add confirm password field and show passwords feature to change password pop-up feat: add confirm password field and show passwords feature to change password popUp Jun 6, 2020
@bharat-1809 bharat-1809 reopened this Jun 6, 2020
@techno-disaster
Copy link
Contributor

@bharat-1809 can you attach a video showing how the new change password dialog works now?

@bharat-1809
Copy link
Contributor Author

@bharat-1809 can you attach a video showing how the new change password dialog works now?

Sure

@bharat-1809
Copy link
Contributor Author

Link to the video

@techno-disaster
Copy link
Contributor

Link to the video

Please try to attach a gif next time onwards :)

@bharat-1809
Copy link
Contributor Author

Link to the video

Please try to attach a gif next time onwards :)

Sure😅

Copy link
Contributor

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

looks good, thank you🎉

@techno-disaster
Copy link
Contributor

The confirm password and show password features work as expected, but due to a prior bad merge, the change password dialog box dosen't respond correctly and the snackbar isn't showed. (BuildContext error). #83 fixes this. So lets merge this PR after that to confirm if the whole change password function works. Thanks for your work @bharat-1809

@bharat-1809
Copy link
Contributor Author

Okay cool, thanks😃

Copy link
Contributor

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

Nice!

lib/screens/settings/settings_screen.dart Outdated Show resolved Hide resolved
bartekpacia
bartekpacia previously approved these changes Jun 10, 2020
Copy link
Contributor

@techno-disaster techno-disaster left a comment

Choose a reason for hiding this comment

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

@bharat-1809 we follow a theme in our app, where if you click to show one of the show passsword eye icons all the fields show the password, can you do the same in the change password dialog window fields? Thank you

@bharat-1809
Copy link
Contributor Author

Yeah sure, I'll definitely do it. I have a little doubt, whether the eye icon will be displayed on all the fields or only one on the topmost field?
Thanks :)

@techno-disaster
Copy link
Contributor

Yeah sure, I'll definitely do it. I have a little doubt, whether the eye icon will be displayed on all the fields or only one on the topmost field?
Thanks :)

on all fields :)

@rpattath rpattath added the Status: Changes Requested Changes are required to be done by the PR author. label Jun 16, 2020
@bharat-1809
Copy link
Contributor Author

@techno-disaster I have made the required changes please have a look.
Thanks

Here's a gif of the same:

20200616_190800

bartekpacia
bartekpacia previously approved these changes Jun 16, 2020
@bharat-1809
Copy link
Contributor Author

@bartekpacia Is that review dismissal automatic? because I didn't do it. 😅

HaripriyaB
HaripriyaB previously approved these changes Jul 1, 2020
@HaripriyaB HaripriyaB added Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Jul 1, 2020
robotjellyzone
robotjellyzone previously approved these changes Jul 3, 2020
Copy link

@robotjellyzone robotjellyzone left a comment

Choose a reason for hiding this comment

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

I have already tested this PR here --> #92 (review)

so, LGTM 🎉 thanks for your contribution @bharat-1809 👍

ALSO, please check/mark that "I have used the Flutter Beta channel on my local machine" section in the description of this PR .

@techno-disaster techno-disaster self-requested a review July 3, 2020 16:39
Copy link
Contributor

@techno-disaster techno-disaster left a comment

Choose a reason for hiding this comment

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

Fix merge conflict

@robotjellyzone robotjellyzone added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. labels Jul 3, 2020
@bharat-1809
Copy link
Contributor Author

Fix merge conflict

Its done. @techno-disaster please have a look :)
Thanks

@techno-disaster
Copy link
Contributor

Can someone from @anitab-org/qa-team give this a test? it was tested before but to confirm nothing broke after the merging the conflict can someone give this another check? Thank You

Copy link

@robotjellyzone robotjellyzone left a comment

Choose a reason for hiding this comment

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

The changes made in this PR were tested locally. Following are the results:

Code review - Done By @techno-disaster

All possible responses (positive and negative tests) were tested as below:

Tested confirm password field , its validation with show passwords feature to change password popUp:

Screenshot/gif:

  1. Allow updating the password even when new password is not matching with confirm password :

tested-pr

  1. Not Showing missing confirm password field message even if its empty :

password-missing

Expected Result :

  • When we write a password in confirm password field then it should match with the new password field.
  • if left empty, it should reflect it as a missing field and should not allow to update the password until it gets all the field values in the required form.

Actual Result :

  • It is not correctly validating as it still allows the change in password even when both new & confirm password fields are not matching.
  • Also, not showing any missing field error/popup even when confirm password field is empty

Additional testcases covered: N/A

Additional Comments: @bharat-1809 You need to make changes in your code and see , where the validation condition is breaking as both of the failed tests were only due to breaking validation case.

Status of PR : Changes Requested
Android Version: 10.0, Device: Android Emulator Pixel 3 XL API 29

@bharat-1809
Copy link
Contributor Author

@anitab-org/maintainers Please have a look. The validation errors have been fixed

@robotjellyzone robotjellyzone added Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Jul 14, 2020
Copy link

@robotjellyzone robotjellyzone left a comment

Choose a reason for hiding this comment

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

Now its working as --->

  1. Showing missing password field -->

Screenshot_1594744902

  1. Also, shows appropriate validation message when passwords don't match -->

Screenshot_1594744935

Great job 👍 @bharat-1809

@robotjellyzone robotjellyzone added Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer. and removed Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. labels Jul 14, 2020
@isabelcosta
Copy link
Member

@techno-disaster @bartekpacia as code owners can you review this please?

Copy link
Contributor

@techno-disaster techno-disaster left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @bharat-1809 👏 🎉

@isabelcosta isabelcosta merged commit 77fc5b3 into anitab-org:develop Jul 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Category: Coding Changes to code base or refactored code that doesn't fix a bug. Category: User Interface Improvements or additions to design. Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add confirm password field and see password feature to change password popUp.
7 participants