fix(CommunityPermissions): Fix token criteria updates in the permissions model #14457
Conversation
Jenkins Builds
|
| let criteriaMet = criteriaResult.criteria[index] | ||
|
|
||
| if tokenCriteriaItem.criteriaMet == criteriaMet: | ||
| continue |
There was a problem hiding this comment.
Ok, I see the mistake I made. If one of the criteria is updated, we update the whole permission, and with my change, we only update/add the criteria that changed.
I think we can still keep the performance improvement by doing it right this time.
We could do something like this:
var aCriteriaChanged = false
for index, tokenCriteriaItem in tokenPermissionItem.getTokenCriteria().getItems():
let criteriaMet = criteriaResult.criteria[index]
if tokenCriteriaItem.criteriaMet != criteriaMet:
aCriteriaChanged = trueThen below on line 957, instead of if updatedTokenCriteriaItems.len == 0:, we do if not aCriteriaChanged:.
This should have the right logic where if a criteria was updated, we put all the criterias in the model, but we can keep the performance improvement when none of the criterias changed.
Does it make sense?
There was a problem hiding this comment.
But then what if something else changes in the tokenCriteria? We'll still show the old data
E.g. TokenCriteria with 1SNT changes to 1ETH, but the user still meets the token criteria
There was a problem hiding this comment.
if this model update is really costly we could just compare the old token criteria item with the new one and update only what was changed in the model. I guess this could be done in the model itself because we're duplicating the same logic in 2 places now.
There was a problem hiding this comment.
But then what if something else changes in the tokenCriteria? We'll still show the old data
E.g. TokenCriteria with 1SNT changes to 1ETH, but the user still meets the token criteria
I asked myself this question too last time and it's not handled by this function. Here, we loop permissions, which is a Table containing CheckPermissionsResultDto. And here is the signature for CheckPermissionsResultDto:
type CheckPermissionsResultDto* = object
criteria*: seq[bool]As you can see, it only contains the criteria result, so it's ok to only check this property.
Also, updateTokenPermissionModel is only called when we get a response from the check functions (onCommunityCheckChannelPermissionsResponse and onCommunityCheckAllChannelsPermissionsResponse)
if this model update is really costly we could just compare the old token criteria item with the new one and update only what was changed in the model. I guess this could be done in the model itself because we're duplicating the same logic in 2 places now.
@micieslak did that change too. I think it was indeed the most costly part, but it will help too to not call the model functions for no reason.
I guess, you can try your change as is with the Status community (use the edit shared addresses popup) and see if it hangs super long when the result comes back. If it's not too bad, we can use your change.
Do note that you can also remove the rest of the changes I made, because they are no longer in use (thereWasAnUpdate for example)
There was a problem hiding this comment.
ohh, we're receiving here only the bool values for the tokenCriteriaMet. So the community tokensPermissionsModel is updated in other flows. Yeah, will work 👍
I'll test it now
There was a problem hiding this comment.
@jrainville Updated! Thanks 🍻
…ons model Re-create the token criteria and include all items whenever the model changes. The previous perf optimisation fix filtered the items where the criteriaMet doesn't change.
alexjba
force-pushed
the
14446-permissions-requirements-that-are-not-met-are-being-hidden-on-the-community-permission-overlay2
branch
from
57bfb69
to
c5a5c8e
Compare
|
@jrainville It's ready for merge |
jrainville
deleted the
14446-permissions-requirements-that-are-not-met-are-being-hidden-on-the-community-permission-overlay2
branch
|
@alexjba pls backport to master :) |
What does the PR do
Fixes #14446
The tokens criteria items were filtered out if the
criteriaMetflag doesn't change after a model update.This filtering was done due to perf reasons, but we need all the token criteria items in the model.
Affected areas
Community permissions
Screenshot of functionality (including design for comparison)
This shows that both token criteria items are shown in the UI even though one is met and one is not.
Screen.Recording.2024-04-17.at.17.01.59.mov