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

Hint functionality for user password #18

Merged
merged 43 commits into from
Jul 29, 2021

Conversation

Bhawna-Ad
Copy link
Contributor

@Bhawna-Ad Bhawna-Ad commented Jun 23, 2021

Description

Made the following changes :

  • Added hint textfield

  • Passed the hint string to ChooseMasterPswrdViewModel

Working on #3

Type of change

New feature

Code quality checklist

Just put an x in the [] where applicable.

  • [] I ran gradlew lint and found no new error related to my code in the report.
  • [] I ran Analyze>Inspect Code and found no new error related to my code.
  • [] I have verified my changes on an emulator/ real device.

@Ni3verma
Copy link
Owner

Hi @Bhawna-Ad , as this is a UI change can you also update PR with the screenshot of your change?

and I see that your fork dont have latest master changes.. so this is what you have to do:
checkout your local master > set upstream master > pull my master(upstream) changes in your master -- now your master is same as mine
now merge your local master in this new branch > resolve conflicts (if any) and push
Let me know if you need any kind of help :)

@Bhawna-Ad
Copy link
Contributor Author

Sure, I will do the needful. Do I have to upload the screenshot here in this thread only? And also it is showing "some checks were not successful" so how do I resolve this?

@Ni3verma
Copy link
Owner

Not in the thread, you can edit your PR by clicking on this icon and paste the image
image

and for CICD failure : for now ignore it, I think it could be because you don't have latest master changes merged in this branch. once you merge and push, it will again run automatically. If it fails again I will check

@Ni3verma Ni3verma self-requested a review June 23, 2021 07:42
@Ni3verma Ni3verma added the enhancement Enhancing existing functionality label Jun 23, 2021
@Bhawna-Ad
Copy link
Contributor Author

Bhawna-Ad commented Jun 23, 2021

Alright, thanks! So after merging do I need to create another draft pull request?😅

@Ni3verma
Copy link
Owner

no you only have to commit and push. It will automatically reflect here

@Bhawna-Ad
Copy link
Contributor Author

I have updated the Pull request with the image of the changes in the UI

@Ni3verma Ni3verma marked this pull request as ready for review June 23, 2021 12:12
@Ni3verma Ni3verma changed the title Feature/add hint Hint functionality for user password Jun 23, 2021
Copy link
Owner

@Ni3verma Ni3verma left a comment

Choose a reason for hiding this comment

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

Reviewed first set of changes.
few minor changes to do.. functionality wise everything is working as it should be 💯

@Ni3verma Ni3verma marked this pull request as draft June 23, 2021 12:37
@Ni3verma
Copy link
Owner

Hey there !
I have reviewed the code, excellent work!
and you can ignore the failing check, there is some issue with github hosted runners which you can track here.

@Ni3verma Ni3verma linked an issue Jun 23, 2021 that may be closed by this pull request
@Bhawna-Ad
Copy link
Contributor Author

I am glad to hear that! I have made the required changes now should I commit and push them to this PR only?

@Ni3verma
Copy link
Owner

yes once you make a PR, you only have to push to that branch and it will automatically reflect here

@Bhawna-Ad
Copy link
Contributor Author

The PR is updated. Please suggest if there are any other changes to be made and how to proceed further.

Copy link
Owner

@Ni3verma Ni3verma left a comment

Choose a reason for hiding this comment

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

If done by mistake then you can undo it. or let me know if there is a reason behind it.

.idea/gradle.xml Outdated Show resolved Hide resolved
@Ni3verma
Copy link
Owner

Next instead of validation, you can pick up to show this hint in login screen. Here I want you to be creative on how to show hint. always show it? show it on click of button? or show it on click of a text "show hint"? whatever looks good. I am more inclined toward showing it on click of a text.
and have you worked on Room library?

@Bhawna-Ad
Copy link
Contributor Author

Well yes I like the idea of showing hint on clicking the text. I would like to implement that.
I haven't worked on Room library but I can surely learn about it and then work on it.

@Ni3verma
Copy link
Owner

yeah Room is very famous and also very easy to learn. not much will be required as part of this issue.
first work on the UI, layout > handling click > showing the text (for now you will not be getting the hint in login view model so you can hardcode it)

next I will tell you how to encrypt the hint before adding in db as you are not doing it. and then later with room how to decrypt and get that hint in login view model

@Bhawna-Ad
Copy link
Contributor Author

Okay! I will update you once I am done with that.

@Ni3verma
Copy link
Owner

@Bhawna-Ad
also fetch latest changes from my master to your master and merge in this branch... after this no code check will fail and I can run pipeline on this code.

@Bhawna-Ad
Copy link
Contributor Author

Yes I have updated my branch and now it is even with your master branch

Bhawna-Ad added 2 commits July 6, 2021 12:44
…eature/add-hint

� Conflicts:
�	app/src/main/java/com/andryoga/safebox/ui/common/Utils.kt
Copy link
Owner

@Ni3verma Ni3verma left a comment

Choose a reason for hiding this comment

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

call dao.getHint method

Copy link
Owner

@Ni3verma Ni3verma left a comment

Choose a reason for hiding this comment

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

Make Hint optional everywhere

Copy link
Owner

@Ni3verma Ni3verma left a comment

Choose a reason for hiding this comment

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

!! operator will throw Null pointer exception if hint is null

Copy link
Owner

@Ni3verma Ni3verma left a comment

Choose a reason for hiding this comment

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

Great !

Copy link
Owner

@Ni3verma Ni3verma left a comment

Choose a reason for hiding this comment

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

excellent work with implementing LCS.
but functionality wise there are many issues, see my comments

Copy link
Owner

@Ni3verma Ni3verma left a comment

Choose a reason for hiding this comment

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

Hint is overriding pswrd validation rules !

Signed-off-by: Nitin Verma <[email protected]>
@Bhawna-Ad Bhawna-Ad marked this pull request as ready for review July 25, 2021 13:05
@Ni3verma
Copy link
Owner

Awesome work again.. Thanks for your contribution :)

@Ni3verma Ni3verma merged commit 3a35079 into Ni3verma:master Jul 29, 2021
@Bhawna-Ad
Copy link
Contributor Author

It was an awesome experience working on this project.🙌

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

Successfully merging this pull request may close these issues.

Add Hint functionality for user login password
2 participants