Copy and reveal credentials #124

mihainisipeanusv · 2018-10-03T12:42:29Z

Fixes #13
Fixes #12

Adding the username and password copy in ItemDetailFragment, also reveal / hide password

please let me know is adding a EditText in ItemDetailsView passed to presenter is the correct approach
please let me know what is the correct approach to mock a EditText in tests.

devinreams · 2018-10-03T12:47:35Z

Thanks for your first contribution, @mihainisipeanusv. I'll defer to the engineering and UX teams to help review this. A few pointers, since I failed to explain these the other day:

Adding "Fixed {issue number}" to the PR description will link this up to the corresponding issues and help us keep our issue/PRs organized
bitrise runs linting so you may see failures on the build if not explicitly running them locally (we don't have any shared idea files yet or pre-commit hooks or anything). in this case you'll see :app:ktlint FAILED on your commits here which probably can all be resolved with ./gradlew ktlintFormat and committing the results 🤞

Thanks again! 🏅

jhugman

This PR seems to work well, but I think it needs some work before it lands.

The uni-directional data flow would demand that we set up an ItemDetailStore and perhaps a ClipboardStore; this would get rid of a bunch view layer code in your presenter, and make it more testable. You can see how the swift version works in the ItemDetailStore and CopyDisplayStore.

Please also read the rest of the comments, and ask questions if they don't make sense.

jhugman · 2018-10-03T13:10:56Z

app/src/test/java/mozilla/lockbox/presenter/ItemDetailPresenterTest.kt

+            get() = tapStub
+
+        override val editUsername: EditText
+            get() = TODO("not implemented") //To change initializer of created properties use File | Settings | File Templates.


Nit: unwanted TODO, and generated comments.

jhugman · 2018-10-03T13:11:40Z

app/src/test/java/mozilla/lockbox/presenter/ItemDetailPresenterTest.kt


        override fun updateItem(item: ItemDetailViewModel) {
            this.item = item
        }
+
+        override fun copyNotification(strId: Int) {
+


Nit: have you forgotten to git add something, or should this be a // NOOP comment?

jhugman · 2018-10-03T13:11:50Z

app/src/test/java/mozilla/lockbox/presenter/ItemDetailPresenterTest.kt

+        }
+
+        override fun updatePasswordField(visible: Boolean){
+


jhugman · 2018-10-03T13:11:59Z

app/src/test/java/mozilla/lockbox/presenter/ItemDetailPresenterTest.kt

+        override val editUsername: EditText
+            get() = TODO("not implemented") //To change initializer of created properties use File | Settings | File Templates.
+        override val editPassword: EditText
+            get() = TODO("not implemented") //To change initializer of created properties use File | Settings | File Templates.


jhugman · 2018-10-03T13:18:17Z

app/src/main/java/mozilla/lockbox/presenter/ItemDetailPresenter.kt

@@ -6,23 +6,69 @@

 package mozilla.lockbox.presenter

+import android.util.Log


You should use log, which is declared in LockboxApplication.

jhugman · 2018-10-03T15:24:13Z

app/src/main/java/mozilla/lockbox/presenter/ItemDetailPresenter.kt

+
+        this.view.btnPasswordCopyClicks
+                .subscribe {
+                    clipboardCopy("password", view.editPassword.text.toString())


I think we should be using the ServerPassword or the ItemDetailViewModel version of the username/password, rather than the UI version.

jhugman · 2018-10-03T15:24:56Z

app/src/main/java/mozilla/lockbox/presenter/ItemDetailPresenter.kt

    private val dispatcher: Dispatcher = Dispatcher.shared,
    private val dataStore: DataStore = DataStore.shared
+


Nit: whitespace.

jhugman · 2018-10-03T15:25:33Z

app/src/main/java/mozilla/lockbox/presenter/ItemDetailPresenter.kt


 interface ItemDetailView {
    var itemId: String?
    fun updateItem(item: ItemDetailViewModel)
+    fun copyNotification(strId: Int)
+    fun updatePasswordField(visible: Boolean)


For a boolean flag like this, consider using a field with a getter/setter.

jhugman · 2018-10-03T15:26:15Z

app/src/main/java/mozilla/lockbox/view/ItemDetailFragment.kt

+            inputPassword.transformationMethod = null
+            btnPasswordToggle.setImageResource(R.drawable.ic_hide)
+        } else {
+            inputPassword.transformationMethod = PasswordTransformationMethod()


Consider using PasswordTransformationMethod.getInstance()

jhugman · 2018-10-03T15:29:04Z

app/src/test/java/mozilla/lockbox/presenter/ItemDetailPresenterTest.kt

-    val subject = ItemDetailPresenter(view, dataStore = dataStore)
+    val clipboardManager = Mockito.mock(ClipboardManager::class.java)
+
+    val subject = ItemDetailPresenter(view, clipboardManager, dataStore = dataStore)


Tests needed.

nickbrandt · 2018-10-03T16:44:31Z

@mihainisipeanusv this looks great and functions as expected.

I did notice an issue with the icon sizes & color. After digging into Zeplin, I realized that although the icons were exportable, they were retaining the original size & color of our icon symbol within Sketch. Meaning although we had resized this icon to 18px by 18px and changed the color in the design, it was exporting at 16px as well as not including the transparent bounding box. I apologize for this and have fixed this in Zeplin now, as well as am including the icons below.

My assumption is that we will take care of styling the snackbar for the copy notifications in a later issue, is that correct?

icon_hide_2018-10-03.zip
icon_copy_2018-10-03.zip
icon_show_2018-10-03.zip

mihainisipeanusv · 2018-10-04T15:21:18Z

@mihainisipeanusv this looks great and functions as expected.

I did notice an issue with the icon sizes & color. After digging into Zeplin, I realized that although the icons were exportable, they were retaining the original size & color of our icon symbol within Sketch. Meaning although we had resized this icon to 18px by 18px and changed the color in the design, it was exporting at 16px as well as not including the transparent bounding box. I apologize for this and have fixed this in Zeplin now, as well as am including the icons below.

My assumption is that we will take care of styling the snackbar for the copy notifications in a later issue, is that correct?

icon_hide_2018-10-03.zip
icon_copy_2018-10-03.zip
icon_show_2018-10-03.zip

nick I had noticed that the new version of the hide and show icon are not ok. In the hide one the eye is at the image center and in the show is not, and when you change image it just jump up and down

jhugman

Thanks for your hard work on this. This is starting to look like the right shape. :)

I've added some more comments. Ask in #Lockbox slack or in the PR itself if you have any questions.

app/src/main/java/mozilla/lockbox/view/ItemDetailFragment.kt

app/src/main/java/mozilla/lockbox/presenter/ItemDetailPresenter.kt

jhugman · 2018-10-05T14:12:26Z

app/src/main/java/mozilla/lockbox/view/ItemDetailFragment.kt

+                inputPassword.transformationMethod = PasswordTransformationMethod.getInstance()
+                btnPasswordToggle.setImageResource(R.drawable.ic_icon_show)
+            }
+        }


app/src/main/java/mozilla/lockbox/store/ClipboardStore.kt

app/src/main/java/mozilla/lockbox/presenter/ItemDetailPresenter.kt

jhugman · 2018-10-05T14:50:39Z

app/src/main/java/mozilla/lockbox/presenter/ItemDetailPresenter.kt

 ) : Presenter() {
+
+    override fun onViewReady() {
+


Nit: whitespace

jhugman

This is getting really close.

Please make the final changes and land this.

Lovely work, thank you.

jhugman · 2018-10-08T11:50:29Z

app/src/main/java/mozilla/lockbox/presenter/ItemDetailPresenter.kt

+                    view.itemId?.let {
+                        dataStore.get(it)
+                                .subscribe {
+                                    dispatcher.dispatch(ClipboardAction.Clip("username", it!!.username!!))


What happens when there is no username?

jhugman · 2018-10-08T12:04:12Z

app/src/main/java/mozilla/lockbox/presenter/ItemDetailPresenter.kt

+
+        this.view.passwordCopyClicks
+                .subscribe {
+                    view.itemId?.let {


Extract this statement into a method, parameterized for username and password.

At some point, it would be good to not hit the database each time, and stash the itemViewModel, but I don't think we need to worry about that now.

jhugman · 2018-10-08T12:05:05Z

app/src/main/java/mozilla/lockbox/store/ClipboardStore.kt

+                    when (it) {
+                        is ClipboardAction.Clip -> {
+                            addToClipboard(it.label, it.str)
+                        }


Good. Consider making a CopyUsername / CopyPassword objects, but this shouldn't stop you landing this at this point.

jhugman · 2018-10-08T12:05:15Z

app/src/main/java/mozilla/lockbox/store/ClipboardStore.kt

+    }
+
+    fun addToClipboard(label: String, str: String) {
+


Whitespace.

jhugman · 2018-10-08T12:07:23Z

app/src/test/java/mozilla/lockbox/store/ClipboardStoreTest.kt

+
+        val clipboardManager: ClipboardManager = RuntimeEnvironment.application.getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager
+        assertTrue(clipboardManager.primaryClip.getItemAt(0).text.equals(testString))
+    }


Can this be made into a property?

RuntimeEnvironment.application.getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager

jhugman · 2018-10-08T12:13:07Z

app/src/main/java/mozilla/lockbox/view/ItemDetailFragment.kt

+
+    override fun showToastNotification(@StringRes strId: Int) {
+        Toast.makeText(activity, getString(strId), Toast.LENGTH_SHORT).show()
+    }


jhugman · 2018-10-08T12:14:35Z

app/src/main/java/mozilla/lockbox/action/ClipboardAction.kt

+
+sealed class ClipboardAction : Action {
+    data class Clip(val label: String, val str: String) : ClipboardAction()
+}


Consider CopyUsername, CopyPassword.

devinreams · 2018-10-08T19:30:10Z

The unit tests were failing on the last two commits and this merged branch now has master failing:
https://app.bitrise.io/build/b0f43db64a0b5751

:app:compileDebugUnitTestKotline: /bitrise/src/app/src/test/java/mozilla/lockbox/store/ClipboardStoreTest.kt: (41, 45): Unresolved reference: Clip

linuxwolf · 2018-10-08T19:34:33Z

There are also 6 new linting errors (full list isn't enumerated on bitrise)

mihainisipeanusv added 3 commits October 2, 2018 16:03

adding copy username to clipboard option

f2a2beb

adding copy password to clipboard

19ad225

adding reveal password, update icons

acb1a05

mihainisipeanusv requested a review from a team as a code owner October 3, 2018 12:42

ghost assigned mihainisipeanusv Oct 3, 2018

ghost added the in progress label Oct 3, 2018

devinreams changed the title ~~Copy credentials~~ Copy and reveal credentials Oct 3, 2018

code update after ktlintFormat

d335f17

jhugman suggested changes Oct 3, 2018

View reviewed changes

mihainisipeanusv added 2 commits October 4, 2018 16:50

partial code review fixes

0af60e5

password visibility method implemented as a variable set override

3fdc0e3

mihainisipeanusv added 2 commits October 5, 2018 11:36

update icons, add ClipboardStore

60aeada

adding clipboardstore test

3f2de55

jhugman suggested changes Oct 5, 2018

View reviewed changes

sashei mentioned this pull request Oct 5, 2018

TelemetryStore implementation #128

Merged

mihainisipeanusv added 2 commits October 8, 2018 10:08

refactor code for code review

d472c3f

access ClipboardStore with actions

49b3d66

jhugman approved these changes Oct 8, 2018

View reviewed changes

mihainisipeanusv and others added 2 commits October 8, 2018 16:29

split clipboard actions in two, add null empty checks for clipboard data

ce5a1e3

Merge branch 'master' into copy-credentials

6d6156a

mihainisipeanusv merged commit 1ee365b into master Oct 8, 2018

ghost removed the in progress label Oct 8, 2018

devinreams deleted the copy-credentials branch October 8, 2018 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy and reveal credentials #124

Copy and reveal credentials #124

mihainisipeanusv commented Oct 3, 2018 •

edited by devinreams

devinreams commented Oct 3, 2018

jhugman left a comment

jhugman Oct 3, 2018

jhugman Oct 3, 2018

jhugman Oct 3, 2018

jhugman Oct 3, 2018

jhugman Oct 3, 2018

jhugman Oct 3, 2018

jhugman Oct 3, 2018

jhugman Oct 3, 2018

jhugman Oct 3, 2018

jhugman Oct 3, 2018

nickbrandt commented Oct 3, 2018

mihainisipeanusv commented Oct 4, 2018

jhugman left a comment

jhugman Oct 5, 2018

jhugman Oct 5, 2018

jhugman left a comment

jhugman Oct 8, 2018

jhugman Oct 8, 2018

jhugman Oct 8, 2018

jhugman Oct 8, 2018

jhugman Oct 8, 2018

jhugman Oct 8, 2018

jhugman Oct 8, 2018

devinreams commented Oct 8, 2018

linuxwolf commented Oct 8, 2018

		@@ -6,23 +6,69 @@

		package mozilla.lockbox.presenter

		import android.util.Log

		private val dispatcher: Dispatcher = Dispatcher.shared,
		private val dataStore: DataStore = DataStore.shared

Copy and reveal credentials #124

Copy and reveal credentials #124

Conversation

mihainisipeanusv commented Oct 3, 2018 • edited by devinreams

devinreams commented Oct 3, 2018

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickbrandt commented Oct 3, 2018

mihainisipeanusv commented Oct 4, 2018

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinreams commented Oct 8, 2018

linuxwolf commented Oct 8, 2018

mihainisipeanusv commented Oct 3, 2018 •

edited by devinreams