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

fix ImportCommunityPopup issues, remove private key importing #12554

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

igor-sirotin
Copy link
Contributor

@igor-sirotin igor-sirotin commented Oct 26, 2023

Fixes #12465
Fixes #12542
Drops the usage of parseCommunitySharedUrl (#12356)

What does the PR do

  1. Correctly show result of requestCommunityInfo call
  2. Remove QML timeout, trust status-go timeout instead
  3. Remove support for private key importing
  4. Remove automatic call for reuestCommunityInfo from getCommunityDetails.
    Because the former returns nothing and requires binding to nim signals.
  5. Update status-go to include fix: set pubsubTopic in request community info from mailserver status-go#4194
  6. storybook page for ImportCommunityPopup

Affected areas

ImportCommunityPopup

Screenshot of functionality (including design for comparison)

Screen.Recording.2023-10-26.at.11.02.21.mov
Screen.Recording.2023-10-26.at.11.05.15.mov

@status-im-auto
Copy link
Member

status-im-auto commented Oct 26, 2023

Jenkins Builds

Click to see older builds (7)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 76f6596 #1 2023-10-26 10:23:27 ~5 min tests/nim 📄log
✔️ 76f6596 #1 2023-10-26 10:26:47 ~8 min macos/aarch64 🍎dmg
✔️ 76f6596 #1 2023-10-26 10:27:53 ~9 min macos/x86_64 🍎dmg
✔️ 76f6596 #1 2023-10-26 10:28:43 ~10 min tests/ui 📄log
✔️ 76f6596 #1 2023-10-26 10:33:26 ~15 min linux/x86_64 📦tgz
✔️ 76f6596 #1 2023-10-26 10:49:23 ~31 min windows/x86_64 💿exe
✔️ 76f6596 #1 2023-10-26 10:50:31 ~32 min tests/e2e 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c47dc02 #2 2023-10-26 14:29:46 ~6 min macos/aarch64 🍎dmg
✔️ c47dc02 #2 2023-10-26 14:30:07 ~6 min tests/nim 📄log
✔️ c47dc02 #2 2023-10-26 14:31:39 ~7 min macos/x86_64 🍎dmg
✔️ c47dc02 #2 2023-10-26 14:34:22 ~10 min tests/ui 📄log
✔️ c47dc02 #2 2023-10-26 14:39:42 ~16 min linux/x86_64 📦tgz
✔️ c47dc02 #2 2023-10-26 14:48:03 ~24 min windows/x86_64 💿exe
✔️ c47dc02 #2 2023-10-26 14:56:03 ~32 min tests/e2e 📄log
✔️ 376163f #3 2023-10-26 17:18:32 ~5 min tests/nim 📄log
✔️ 376163f #3 2023-10-26 17:19:20 ~6 min macos/aarch64 🍎dmg
✔️ 376163f #3 2023-10-26 17:21:08 ~7 min macos/x86_64 🍎dmg
✔️ 376163f #3 2023-10-26 17:24:33 ~11 min tests/ui 📄log
✔️ 376163f #3 2023-10-26 17:28:29 ~15 min linux/x86_64 📦tgz
✖️ 376163f #3 2023-10-26 17:35:17 ~22 min tests/e2e 📄log
✔️ 376163f #3 2023-10-26 17:40:11 ~26 min windows/x86_64 💿exe
✔️ 376163f #4 2023-10-26 18:08:30 ~28 min tests/e2e 📄log

@igor-sirotin igor-sirotin force-pushed the fix/issue-12542-import-community-popup branch from 76f6596 to c47dc02 Compare October 26, 2023 14:23
Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just some minor points

storybook/main.qml Show resolved Hide resolved
ui/imports/shared/popups/ImportCommunityPopup.qml Outdated Show resolved Hide resolved
return Utils.getCommunityDataFromSharedLink(inputKey).communityId;
if (Utils.isStatusDeepLink(inputKey)) {
const linkData = Utils.getCommunityDataFromSharedLink(inputKey)
return !linkData ? "" : linkData.communityId
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return !linkData ? "" : linkData.communityId
return !!linkData ? linkData.communityId : ""

I always find this double negative idiom ugly :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get it, you find it ugly, but still suggest? 😄

ui/imports/shared/popups/ImportCommunityPopup.qml Outdated Show resolved Hide resolved
ui/imports/shared/popups/ImportCommunityPopup.qml Outdated Show resolved Hide resolved
Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

LGTM

@igor-sirotin igor-sirotin merged commit a3239d9 into master Oct 27, 2023
9 checks passed
@igor-sirotin igor-sirotin deleted the fix/issue-12542-import-community-popup branch October 27, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants