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(Communities): Introduce request id for checking community permissions #14403

Conversation

MishkaRogachev
Copy link
Contributor

@MishkaRogachev MishkaRogachev commented Apr 11, 2024

Close #14376

What does the PR do

Screen.Recording.2024-04-29.at.13.02.38.mov
  • Add enum request id to track permissions check role
  • Add dedicated permissions model for addresses check popup

Affected areas

Communities

@status-im-auto
Copy link
Member

status-im-auto commented Apr 11, 2024

Jenkins Builds

Click to see older builds (41)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c99e106 #1 2024-04-11 17:24:41 ~6 min tests/nim 📄log
✔️ c99e106 #1 2024-04-11 17:24:55 ~6 min macos/aarch64 🍎dmg
✔️ c99e106 #1 2024-04-11 17:29:39 ~11 min tests/ui 📄log
✔️ c99e106 #1 2024-04-11 17:30:45 ~12 min macos/x86_64 🍎dmg
✔️ c99e106 #1 2024-04-11 17:35:54 ~17 min linux/x86_64 📦tgz
✔️ c99e106 #1 2024-04-11 17:53:53 ~35 min windows/x86_64 💿exe
✔️ 09948a6 #2 2024-04-18 14:20:34 ~6 min tests/nim 📄log
✔️ 09948a6 #2 2024-04-18 14:22:41 ~8 min macos/aarch64 🍎dmg
✔️ 09948a6 #2 2024-04-18 14:22:50 ~8 min macos/x86_64 🍎dmg
✔️ 09948a6 #2 2024-04-18 14:26:25 ~12 min tests/ui 📄log
✔️ 09948a6 #2 2024-04-18 14:30:28 ~16 min linux/x86_64 📦tgz
✔️ 09948a6 #2 2024-04-18 14:47:31 ~33 min windows/x86_64 💿exe
✔️ 15ba842 #3 2024-04-18 18:12:33 ~6 min tests/nim 📄log
✔️ 15ba842 #3 2024-04-18 18:12:49 ~6 min macos/aarch64 🍎dmg
✔️ 15ba842 #3 2024-04-18 18:13:57 ~7 min macos/x86_64 🍎dmg
✔️ 15ba842 #3 2024-04-18 18:17:07 ~11 min tests/ui 📄log
✔️ 15ba842 #3 2024-04-18 18:21:51 ~15 min linux/x86_64 📦tgz
✔️ 15ba842 #3 2024-04-18 18:38:24 ~32 min windows/x86_64 💿exe
✔️ 1cc894d #4 2024-04-19 14:59:46 ~4 min macos/aarch64 🍎dmg
✔️ 1cc894d #4 2024-04-19 15:01:17 ~6 min tests/nim 📄log
✔️ 1cc894d #4 2024-04-19 15:03:21 ~8 min macos/x86_64 🍎dmg
✔️ 1cc894d #4 2024-04-19 15:07:44 ~12 min tests/ui 📄log
✔️ 1cc894d #4 2024-04-19 15:11:18 ~16 min linux/x86_64 📦tgz
✔️ c4c0f96 #5 2024-04-19 15:26:42 ~4 min macos/aarch64 🍎dmg
✔️ c4c0f96 #5 2024-04-19 15:29:08 ~6 min tests/nim 📄log
✔️ c4c0f96 #5 2024-04-19 15:29:44 ~7 min macos/x86_64 🍎dmg
✔️ c4c0f96 #5 2024-04-19 15:34:23 ~11 min tests/ui 📄log
✔️ c4c0f96 #5 2024-04-19 15:38:40 ~16 min linux/x86_64 📦tgz
✔️ c4c0f96 #5 2024-04-19 15:46:27 ~23 min windows/x86_64 💿exe
✔️ 57b21a0 #6 2024-04-19 16:13:47 ~4 min macos/aarch64 🍎dmg
✔️ 57b21a0 #6 2024-04-19 16:16:04 ~6 min tests/nim 📄log
✔️ 57b21a0 #6 2024-04-19 16:16:28 ~7 min macos/x86_64 🍎dmg
✔️ 57b21a0 #6 2024-04-19 16:20:17 ~10 min tests/ui 📄log
✔️ 57b21a0 #6 2024-04-19 16:24:47 ~15 min linux/x86_64 📦tgz
✔️ 57b21a0 #6 2024-04-19 16:39:41 ~30 min windows/x86_64 💿exe
✔️ 8ef6236 #7 2024-04-29 08:59:59 ~4 min macos/aarch64 🍎dmg
✔️ 8ef6236 #7 2024-04-29 09:03:53 ~8 min tests/nim 📄log
✔️ 8ef6236 #7 2024-04-29 09:05:25 ~10 min macos/x86_64 🍎dmg
✔️ 8ef6236 #7 2024-04-29 09:10:09 ~15 min linux/x86_64 📦tgz
✔️ 8ef6236 #7 2024-04-29 09:11:17 ~16 min tests/ui 📄log
✔️ 8ef6236 #7 2024-04-29 09:19:18 ~24 min windows/x86_64 💿exe
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 54b9fb3 #8 2024-04-29 11:09:22 ~4 min macos/aarch64 🍎dmg
✔️ 54b9fb3 #8 2024-04-29 11:13:04 ~7 min macos/x86_64 🍎dmg
✔️ 54b9fb3 #8 2024-04-29 11:13:58 ~8 min tests/nim 📄log
✔️ 54b9fb3 #8 2024-04-29 11:20:15 ~15 min linux/x86_64 📦tgz
✔️ 54b9fb3 #8 2024-04-29 11:20:32 ~15 min tests/ui 📄log
✔️ 54b9fb3 #8 2024-04-29 11:39:08 ~33 min windows/x86_64 💿exe
✔️ 12f3a60 #9 2024-05-01 13:05:30 ~4 min macos/aarch64 🍎dmg
✔️ 12f3a60 #9 2024-05-01 13:08:57 ~8 min macos/x86_64 🍎dmg
✔️ 12f3a60 #9 2024-05-01 13:09:40 ~8 min tests/nim 📄log
✔️ 12f3a60 #9 2024-05-01 13:16:03 ~15 min linux/x86_64 📦tgz
✔️ 12f3a60 #9 2024-05-01 13:17:18 ~16 min tests/ui 📄log
✔️ 12f3a60 #9 2024-05-01 13:34:07 ~33 min windows/x86_64 💿exe

@cammellos
Copy link
Member

@MishkaRogachev JSON rpc should already have a request id you can use:

curl -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","method":"web3_clientVersion","params":[],"id":67}'

can we leverage that instead?

@MishkaRogachev
Copy link
Contributor Author

@MishkaRogachev JSON rpc should already have a request id you can use:

curl -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","method":"web3_clientVersion","params":[],"id":67}'

can we leverage that instead?

you are right in case we need to identify requests between status-go and desktop, should use id from json, here the problem is a bit deeper on ui, i think when i come back to this PR i need to create a second permissions model instance to check permissions independently from the main community permissions model

@MishkaRogachev MishkaRogachev force-pushed the fix/issue-14376-chat-content-updates-in-the-background-while-playing-with-the-edit-shared-popup-addresses branch 4 times, most recently from c4c0f96 to 57b21a0 Compare April 19, 2024 16:09
@MishkaRogachev MishkaRogachev force-pushed the fix/issue-14376-chat-content-updates-in-the-background-while-playing-with-the-edit-shared-popup-addresses branch 2 times, most recently from 8ef6236 to 54b9fb3 Compare April 29, 2024 11:04
@MishkaRogachev
Copy link
Contributor Author

@caybro @mprakhov
I am putting this PR for review, the problem described in the task it fixes, but there are a few buts:

  • The components are very interconnected, but I will not risk to do some serious refactoring myself, there are a lot of components whose purpose I don't understand very well.
  • There is a problem with the state of the button, the state as if lags by one step. (UPD: the same problem exists in the master):
Screenshot 2024-04-29 at 13 08 20 Screenshot 2024-04-29 at 13 06 33

@MishkaRogachev MishkaRogachev marked this pull request as ready for review April 29, 2024 12:53
@MishkaRogachev
Copy link
Contributor Author

@caybro i can't reproduce this issue on master. Is not it already fixed via #14518?

@caybro
Copy link
Member

caybro commented Apr 29, 2024

@caybro i can't reproduce this issue on master. Is not it already fixed via #14518?

Not sure really... I saw something similar at the offsite but others are telling me they can't reproduce

@@ -378,7 +378,7 @@ method onAcceptRequestToJoinFailedNoPermission*(self: AccessInterface, community
method onDeactivateChatLoader*(self: AccessInterface, chatId: string) {.base.} =
raise newException(ValueError, "No implementation available")

method onCommunityCheckPermissionsToJoinResponse*(self: AccessInterface, checkPermissionsToJoinResponse: CheckPermissionsToJoinResponseDto) {.base.} =
method onCommunityCheckPermissionsToJoinResponse*(self: AccessInterface, checkPermissionsToJoinResponse: CheckPermissionsToJoinResponseDto, ) {.base.} =
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary , at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@mprakhov mprakhov left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@MishkaRogachev MishkaRogachev force-pushed the fix/issue-14376-chat-content-updates-in-the-background-while-playing-with-the-edit-shared-popup-addresses branch from 54b9fb3 to 12f3a60 Compare May 1, 2024 13:00
@MishkaRogachev
Copy link
Contributor Author

@jrainville @mprakhov @caybro
I have some doubts that this PR is worth it:

  • it doesn't make the code cleaner, the separation should be made deeper
  • the problem mentioned in the ticket has already been fixed in the master, or rather I can't reproduce it anymore

If anyone can confirm that the problem is not reproducible in the current master, I suggest closing this PR

@jrainville
Copy link
Member

If anyone can confirm that the problem is not reproducible in the current master, I suggest closing this PR

If indeed the issue isn't reproducible anymore, it's not worth adding code for no reason. There might be a refactor on the permissions module later anyway

@MishkaRogachev
Copy link
Contributor Author

MishkaRogachev commented May 1, 2024

If indeed the issue isn't reproducible anymore, it's not worth adding code for no reason. There might be a refactor on the permissions module later anyway

Yeah, I'd prefer a more consistent refactoring here. Most likely it was fixed here: #13825

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

Successfully merging this pull request may close these issues.

Chat content updates in the background while playing with the Edit Shared Popup addresses
6 participants