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

feat: share image/username on separate topic on a community #2745

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

osmaczko
Copy link
Contributor

@osmaczko osmaczko commented Jul 8, 2022

  • publishContactCode now also publishes on joined communities separate topic
  • publishIdentityImage is now triggered each time user joins a new community, this is to ensure other community members got updated with user details immediately

closes: #2707

@status-github-bot
Copy link

status-github-bot bot commented Jul 8, 2022

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?
  • Have you tested changes with mobile?
  • Have you tested changes with desktop?

@status-im-auto
Copy link
Member

status-im-auto commented Jul 8, 2022

Jenkins Builds

Click to see older builds (3)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 06211d1 #1 2022-07-08 10:39:42 ~3 min linux 📦zip
✔️ 06211d1 #1 2022-07-08 10:41:16 ~4 min ios 📦zip
✔️ 06211d1 #1 2022-07-08 10:42:53 ~6 min android 📦aar
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 02d5068 #2 2022-07-13 09:56:36 ~2 min linux 📦zip
✔️ 02d5068 #2 2022-07-13 09:58:27 ~4 min android 📦aar
✔️ 02d5068 #2 2022-07-13 09:59:17 ~5 min ios 📦zip
✔️ 3ac5887 #3 2022-07-18 12:20:20 ~2 min linux 📦zip
✔️ 3ac5887 #3 2022-07-18 12:20:53 ~3 min ios 📦zip
✔️ 3ac5887 #3 2022-07-18 12:22:33 ~4 min android 📦aar

Copy link
Member

@0x-r4bbit 0x-r4bbit left a comment

Choose a reason for hiding this comment

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

Changes LGTM.
Can we have 1-2 tests for this please?

@osmaczko osmaczko force-pushed the feat/2702/community-share-user-separate-topic branch from 06211d1 to 02d5068 Compare July 13, 2022 09:53
@osmaczko
Copy link
Contributor Author

Changes LGTM. Can we have 1-2 tests for this please?

@PascalPrecht tests added, please take a look: https://github.com/status-im/status-go/pull/2745/files#diff-d4f4b1d3dcfd1f9dda7f6d7a397650c30b499042a7770a1df9a6fe80d61f04fe

Copy link
Member

@cammellos cammellos left a comment

Choose a reason for hiding this comment

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

Great PR!

s.Require().NotNil(response)
}

patryk := s.newMessenger()
Copy link
Member

Choose a reason for hiding this comment

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

Bob's going to be jealous 😎

@osmaczko
Copy link
Contributor Author

osmaczko commented Jul 13, 2022

waiting for tests: status-im/status-mobile#13661

@0x-r4bbit
Copy link
Member

Nice work @osmaczko

@pavloburykh
Copy link

pavloburykh commented Jul 14, 2022

@osmaczko Thanx for the PR! If it is ready for QA, could you please move it to Ready for client testing column, in order it be taken into test by both desktop and mobile QA teams? Thank you!

@osmaczko osmaczko moved this from Contributor to Ready for client testing in Client testing status-go Jul 14, 2022
@osmaczko
Copy link
Contributor Author

@osmaczko Thanx for the PR! If it is ready for QA, could you please move it to Ready for client testing column, in order it be taken into test by both desktop and QA team? Thank you!

Thanks. Ofc, done 👍

@pavloburykh
Copy link

@osmaczko thank you for the PR.

Tested on mobile and got a question about ENS name updating in community channel:

User's ENS name is updated for other community users only after ENS user posts some message in channel. Is it an expected behaviour?

Steps:

  1. Create a new community
  2. Create a new User 1
  3. Join community via newly created User 1
  4. Join community via User 2 who has couple of ENS names connected
  5. Try to change User's 2 ENS name (without posting new messages in channel after update) and see if User 1 can see updated ENS.

Actual result: User 1 doesn't see User's 2 updated ENS until User 2 posts some message in community channel.

Expected result: ??? User 1 sees User's 2 updated ENS name, right after it has been updated.

PS: sorry, not sure if this problem is related to current PR.

ENS_update_community.mp4

@pavloburykh pavloburykh self-assigned this Jul 15, 2022
@pavloburykh pavloburykh moved this from Ready for client testing to Contributor in Client testing status-go Jul 15, 2022
@osmaczko
Copy link
Contributor Author

osmaczko commented Jul 18, 2022

@pavloburykh thanks for testing.

It seems propagating ENS name with contact identity is not yet implemented (FYI @cammellos)

Actual result: User 1 doesn't see User's 2 updated ENS until User 2 posts some message in community channel.

I believe the same issue happens in any other kind of chat (public, group, 1on1), could you please check?

Expected result: ??? User 1 sees User's 2 updated ENS name, right after it has been updated.

Yes, I would expect so.


IMO the issue is not related to this PR and should be solved separately. Since on mobile, display names are not implemented yet (:question:), I'm attaching a video where display names are propagated to community members as expected:

community-rename-2022-07-18_12.38.43.mp4

Changing user images also works as expected:

community-avatar-2022-07-18_13.07.02.mp4

@pavloburykh pavloburykh added Tested: mobile checked for regression on mobile client and removed mobile: tested-issues labels Jul 18, 2022
@pavloburykh pavloburykh moved this from Contributor to Ready for client testing in Client testing status-go Jul 18, 2022
@pavloburykh
Copy link

@osmaczko thank you! You're right, this issue is not related to current PR. Therefore, currently there are no issues from mobile side. Waiting for desktop QA team to test the PR, after that it will be ready for merge.

@anastasiyaig
Copy link

anastasiyaig commented Jul 20, 2022

@osmaczko 1 question i want to clarify: as user2 , having custom picture, i joined community and this user's picture is not displayed in members list of community, or Online users section:

Screenshot 2022-07-20 at 10 55 58

I created status-im/status-desktop#6530 to test the commit in go, double checked - it points to correct version

@anastasiyaig
Copy link

okay , the issue is a separate one related to settings

@anastasiyaig anastasiyaig added the Tested: desktop checked for regression on desktop client label Jul 20, 2022
@anastasiyaig anastasiyaig moved this from Ready for client testing to Merge in Client testing status-go Jul 20, 2022
@osmaczko osmaczko merged commit e5e0740 into develop Jul 20, 2022
Client testing status-go automation moved this from Merge to DONE Jul 20, 2022
@osmaczko osmaczko deleted the feat/2702/community-share-user-separate-topic branch July 20, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tested: desktop checked for regression on desktop client Tested: mobile checked for regression on mobile client
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Share image/username on separate topic on a community
7 participants