Fixing bug #1706 edit profile from drawer resetting username to default #1733

yenda · 2017-09-01T23:28:16Z

Summary:

Username is changed to default if Profile screen was opened and user taps on status and username fields

The pull request solves the bug by reworking the way state is stored for profile editing.
Before the drawer profile was in local state and the profile was in app-db. This PR puts everything into the app-db and simplifies future UX changes by defining small events for each user action.

Tests are also adapted to reflect the changes (only simple cases of status changes, maybe in the future some generative testing could be used to check more complex events, or at least more cases could be added). I also removed the phone change test because it was not a phone change test

Behaviors tested on Android emulator

status field is unfocused and saved in drawer and unfocused in edit screen when pressing linebreak
when name is invalid the save button is not available in edit profile screen and reverted to placeholder if saved
when name is invalid the name is shown in red in the drawer

Additional UX improvements tested:

when name is invalid in drawer and saved it is reverted to placeholder (instead of previous name)
when exiting the edit-profile screen without saving the state is kept unless user edits from drawer (instead of lost as soon as user goes back to profile screen)
pressing the vertical dots icon in the drawer while editing name or status saves it and goes to profile screen (before it was not saving)

Steps to test:

Open Status
Open drawer, go to profile screen
Go back and tap status then username screen

status: ready

jeluard · 2017-09-02T08:26:16Z

src/status_im/components/drawer/view.cljs

+  (close-drawer)
+  (when-let [save-event @(rf/subscribe [:my-profile.drawer/save-event])]
+    (rf/dispatch [save-event]))
+  (rf/dispatch [:navigate-to :accounts]))


Both block of code are very similar. Maybe introduce a function?

jeluard · 2017-09-02T08:26:56Z

src/status_im/components/drawer/view.cljs

-   [view
-    [ci/chat-icon (:photo-path account) {:size 52}]]])
+  (letsubs [account [:get-current-account]]
+    [touchable-opacity {:on-press #(navigate-to-profile)


Why not just navigate-to-profile ?

jeluard · 2017-09-02T08:27:37Z

src/status_im/components/drawer/view.cljs

-    {:on-press (fn []
-                 (close-drawer)
-                 (rf/dispatch [:navigate-to :my-profile]))}
+    {:on-press #(navigate-to-profile)}


Same comment

jeluard · 2017-09-02T08:27:40Z

src/status_im/components/drawer/view.cljs

-    {:on-press (fn []
-                 (close-drawer)
-                 (rf/dispatch [:navigate-to :accounts]))}
+    {:on-press #(navigate-to-accounts)}


Same comment

jeluard · 2017-09-02T08:28:57Z

src/status_im/ui/screens/profile/events.cljs

+          {:keys [name public-key]}                        (get-current-account db)]
+      {:db (cond-> db
+             (not default-name) (assoc :my-profile/default-name (gfycat/generate-gfy public-key))
+             :always            (assoc-in [:my-profile/drawer :name] name))})))


I am more used to see :else. I don't know if we have such convention in the project.

We used true in some places, but i like :always much more than true tbh. And :else isn't quite correct here, because other branches can be executed as well.

Indeed :always is more appropriate for cond->.

jeluard · 2017-09-02T08:30:24Z

src/status_im/ui/screens/profile/subs.cljs

+  :<- [:get-current-account]
+  :<- [:get :my-profile/drawer]
+  (fn [[current-account profile] [_ k]]
+    (or (get profile k)


Both blocks are similar. Introduce a function?

jeluard · 2017-09-02T08:30:41Z

src/status_im/ui/screens/profile/subs.cljs

+(reg-sub :my-profile.drawer/valid-name?
+  :<- [:get :my-profile/drawer]
+  (fn [profile]
+    (get profile :valid-name? true)))


Same comment.

I am not sure how to introduce a function there it is subscriptions, the idea is that it only recomputes when the profile or drawer changes

(or (get ..) (get ..)) is used for 2 subscriptions. This part could be abstracted?

I don't think it is worth trading 2 lines for a function and an indirection

rasom · 2017-09-04T07:03:32Z

src/status_im/ui/screens/profile/events.cljs

+                     (assoc-in [:my-profile/profile :edit-status?] edit-status?)
+                     (update-in [:my-profile/profile]
+                                #(merge (select-keys (get-current-account db) db/account-profile-keys) %)))]
+      {:db new-db


alignment in map

annadanchenko · 2017-09-04T18:17:32Z

#1746 Default username becomes empty if open Profile screen [bug/username-changed-to-default] is introduced by this PR
still testing

annadanchenko · 2017-09-05T08:17:18Z

Wrong QR code (picture - if scan then no public key is recognized), no username and no avatar is shown on Show QR opened on Profile screen. Same issue for Show QR of the Public key and Address.

Test session (from 00:50) https://app.testfairy.com/projects/4803622-status/builds/6498278/sessions/3/?accessToken=Sw2XZ7nnooCNL6a49kGzIbIeia4

Steps:

Open Status and sign in
Open drawer
Tap on 3 dots icon to open Profile screen
Tap on Show QR
Check values shown on the screen. Try to scan QR by another device with Status to add it as a contact
Return to Profile screen
Next to Public key tap on 3 dots and select Show QR.
Check values shown on the screen. Try to scan QR by another device with Status to add it as a contact

On both Android and iOS, build #6 for bug/username-changed-to-defaulthttp://185.90.37.243:8080/job/Github%20Org%20Builder/job/status-react/job/bug%252Fusername-changed-to-default/

yenda · 2017-09-05T08:34:40Z

The QR code bug was caused by a missing import:

With the refactoring the navigation code got moved from events to navigation namespace and navigation namespace has to be imported in events otherwise the code is never there. In this case there was data to be pre-loaded when opening the qr-code modal window that was not anymore.

oskarth · 2017-09-05T10:45:47Z

src/status_im/components/drawer/view.cljs

@@ -34,60 +34,70 @@
 (defn open-drawer [] (.openDrawer @drawer-atom))
 (defn close-drawer [] (.closeDrawer @drawer-atom))


What about calling this open-drawer! to signify side effect? Same with close-drawer and possibly save-profile.

oskarth · 2017-09-05T10:47:23Z

src/status_im/components/drawer/view.cljs

+            status       [:my-profile.drawer/get :status]]
+    (let [placeholder (i18n/label :t/update-status)]
+      [view st/status-container
+       (if edit-status?


I'd consider breaking these two vectors out into util fns so if statement is clear, no need for newline then

oskarth · 2017-09-05T10:48:25Z

src/status_im/ui/screens/profile/db.cljs

@@ -6,6 +6,8 @@
            [status-im.constants :as constants]
            [status-im.utils.homoglyph :as homoglyph]))

+(def account-profile-keys [:name :photo-path :status])


Is this always used as a cursor? If so I think we should indicate it as such, e.g. photo-status-path or something

it's not a cursor it is a list of keys to select from the profile when extracting profile

oskarth · 2017-09-05T10:48:39Z

src/status_im/ui/screens/profile/edit/views.cljs

-                             :on-change-text #(dispatch [:set-in [:my-profile/edit :name] %])}]]))
+     [text-input-with-label
+      {:label          (label :t/name)
+       :default-value profile-name


oskarth · 2017-09-05T10:49:00Z

src/status_im/ui/screens/profile/edit/views.cljs

    [react/view styles/edit-profile-status
     [react/scroll-view
      (if edit-status?
        [react/text-input
-         {:ref               #(reset! input-ref %)
-          :auto-focus        edit-status?
+         {:auto-focus        (if edit-status? true false)


isn't this just edit-status? would consider renaming it to auto-focus?

I don't know about this one, the focus can be on something else, auto-focus in the app-db wouldn't be really helpful to figure our what it implies. Here it means user is editing the status field.

oskarth · 2017-09-05T10:50:22Z

src/status_im/ui/screens/profile/subs.cljs

+  :<- [:get-current-account]
+  :<- [:get :my-profile/drawer]
+  (fn [[current-account profile] [_ k]]
+    (or (get profile k)


janherich · 2017-09-05T10:50:51Z

src/status_im/ui/screens/profile/events.cljs

+        {:db new-db
+         :dispatch-n [[:check-status-change status]
+                      [:account-update {:status status}]]}))))
+


Ideally we would like to have :check-status-change and :account-update refactored into pure functions and just call them there instead of the dispatch, but I understand it's not focus of this PR and it's another module (accounts), so it can be done later.

yes it's an old part of the code where there is still ui/side-effects

annadanchenko · 2017-09-05T14:25:36Z

Wrong QR code is fixed in last build
Pressing save button in edit profile screen return to profile screen - OK

New issue: #1769 picture is not saved as avatar if use Capture. After tap on camera icon Edit profile screen is shown but avatar contains old default picture

annadanchenko · 2017-09-07T11:15:19Z

#1769 is fixed.
New issue is found in this PR, can't reproduce it on develop build: #1778 Username of the first account is changed to the username of second account on same device [bug/username-changed-to-default]

asemiankevich · 2017-09-08T11:38:37Z

Looks good, #1769
#1746
#1747
#1706 are not reproducible, QR issue is also fixed

yenda requested review from angusiguess, jeluard, oskarth, janherich and flexsurfer September 1, 2017 23:28

yenda changed the title ~~Fixing bug #1706 edit profile from drawer resetting username to default~~ [WIP] Fixing bug #1706 edit profile from drawer resetting username to default Sep 1, 2017

yenda force-pushed the bug/username-changed-to-default branch 2 times, most recently from 67e5a62 to 81b407a Compare September 2, 2017 00:12

yenda self-assigned this Sep 2, 2017

yenda changed the title ~~[WIP] Fixing bug #1706 edit profile from drawer resetting username to default~~ Fixing bug #1706 edit profile from drawer resetting username to default Sep 2, 2017

angusiguess approved these changes Sep 2, 2017

View reviewed changes

yenda force-pushed the bug/username-changed-to-default branch from 81b407a to 71b0aca Compare September 2, 2017 07:51

jeluard reviewed Sep 2, 2017

View reviewed changes

jeluard suggested changes Sep 2, 2017

View reviewed changes

rasom reviewed Sep 4, 2017

View reviewed changes

yenda force-pushed the bug/username-changed-to-default branch from 71b0aca to 1951698 Compare September 4, 2017 08:46

rasom approved these changes Sep 4, 2017

View reviewed changes

yenda force-pushed the bug/username-changed-to-default branch from 1951698 to d93d300 Compare September 4, 2017 23:09

yenda force-pushed the bug/username-changed-to-default branch from d93d300 to f34c98a Compare September 5, 2017 08:30

yenda force-pushed the bug/username-changed-to-default branch from f34c98a to deb1dbb Compare September 5, 2017 09:39

jeluard approved these changes Sep 5, 2017

View reviewed changes

oskarth reviewed Sep 5, 2017

View reviewed changes

oskarth approved these changes Sep 5, 2017

View reviewed changes

janherich approved these changes Sep 5, 2017

View reviewed changes

yenda force-pushed the bug/username-changed-to-default branch 2 times, most recently from 1ad5d20 to 9044db9 Compare September 6, 2017 18:24

yenda force-pushed the bug/username-changed-to-default branch from 9044db9 to 4ced8d4 Compare September 7, 2017 12:41

asemiankevich self-assigned this Sep 8, 2017

asemiankevich added the Tested - OK label Sep 8, 2017

yenda force-pushed the bug/username-changed-to-default branch from 4ced8d4 to 3686067 Compare September 8, 2017 13:05

moving drawer local state to db and fixing bug

26db180

yenda force-pushed the bug/username-changed-to-default branch from 3686067 to 26db180 Compare September 8, 2017 14:34

rasom merged commit ab98de0 into develop Sep 9, 2017

rasom deleted the bug/username-changed-to-default branch September 9, 2017 04:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing bug #1706 edit profile from drawer resetting username to default #1733

Fixing bug #1706 edit profile from drawer resetting username to default #1733

yenda commented Sep 1, 2017 •

edited by rasom

Loading

jeluard Sep 2, 2017

jeluard Sep 2, 2017

jeluard Sep 2, 2017

jeluard Sep 2, 2017

jeluard Sep 2, 2017

rasom Sep 4, 2017

jeluard Sep 4, 2017

jeluard Sep 2, 2017

oskarth Sep 5, 2017

jeluard Sep 2, 2017

yenda Sep 4, 2017

jeluard Sep 4, 2017

yenda Sep 4, 2017

rasom Sep 4, 2017

annadanchenko commented Sep 4, 2017

annadanchenko commented Sep 5, 2017 •

edited

Loading

yenda commented Sep 5, 2017

oskarth Sep 5, 2017

oskarth Sep 5, 2017

oskarth Sep 5, 2017

yenda Sep 6, 2017

oskarth Sep 5, 2017

oskarth Sep 5, 2017

yenda Sep 6, 2017

oskarth Sep 5, 2017

janherich Sep 5, 2017

yenda Sep 6, 2017

annadanchenko commented Sep 5, 2017 •

edited

Loading

annadanchenko commented Sep 7, 2017 •

edited

Loading

asemiankevich commented Sep 8, 2017

		@@ -34,60 +34,70 @@
		(defn open-drawer [] (.openDrawer @drawer-atom))
		(defn close-drawer [] (.closeDrawer @drawer-atom))

Fixing bug #1706 edit profile from drawer resetting username to default #1733

Fixing bug #1706 edit profile from drawer resetting username to default #1733

Conversation

yenda commented Sep 1, 2017 • edited by rasom Loading

Summary:

Behaviors tested on Android emulator

Additional UX improvements tested:

Steps to test:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annadanchenko commented Sep 4, 2017

annadanchenko commented Sep 5, 2017 • edited Loading

yenda commented Sep 5, 2017

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

annadanchenko commented Sep 5, 2017 • edited Loading

annadanchenko commented Sep 7, 2017 • edited Loading

asemiankevich commented Sep 8, 2017

yenda commented Sep 1, 2017 •

edited by rasom

Loading

annadanchenko commented Sep 5, 2017 •

edited

Loading

annadanchenko commented Sep 5, 2017 •

edited

Loading

annadanchenko commented Sep 7, 2017 •

edited

Loading