-
Notifications
You must be signed in to change notification settings - Fork 2
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
Check if establishing relationship is possible #451
base: main
Are you sure you want to change the base?
Conversation
…-establishing-relationship-is-possible
…-establishing-relationship-is-possible
…ng-relationship-is-possible' into check-if-establishing-relationship-is-possible
…-establishing-relationship-is-possible # Conflicts: # apps/enmeshed/lib/core/app_ui_bridge.dart # apps/enmeshed/lib/core/modals/error_dialog.dart
…-establishing-relationship-is-possible
@@ -8,6 +8,7 @@ class DismissibleContactItem extends StatefulWidget { | |||
final IdentityDVO contact; | |||
final VoidCallback onTap; | |||
final void Function(BuildContext) onDeletePressed; | |||
final bool isRequestExpired; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should pass the request instead of this boolean, for this widget the variable name and type is very meaningless
if (!widget.validateCreateRelationship) return; | ||
|
||
final session = GetIt.I.get<EnmeshedRuntime>().getSession(widget.accountId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we discussed a very important part that was just ignored here. This function now runs every time a request is rendered that originates from a template. But I miss the check for the existing relationship.
Also, is this really the correct place to run this check? Can't we move this to the request screen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In RequestScreen i do "validateCreateRelationship: requestDVO?.source?.type == LocalRequestSourceType.RelationshipTemplate" and pass that to the param validateCreateRelationship
, which returns on line 317
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But i guess i could do this whole check in RequestScreen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, as you see it's not a good idea to split this information
…ip-is-possible' into check-if-establishing-relationship-is-possible
onDeletePressed: _onDeletePressed, | ||
subtitle: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that subtitle should be set inside of the DismissibleContactItem (you already inject the request in there)
|
||
if (!context.mounted) return; | ||
|
||
if (result != null && result) await _onDeletePressed(context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (result != null && result) await _onDeletePressed(context); | |
if (result == true) await _onDeletePressed(context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result can be null, but i use now if (result ?? false)
instead
@@ -147,3 +151,19 @@ Future<void> deleteContact({ | |||
|
|||
onContactDeleted(); | |||
} | |||
|
|||
Future<({bool success, String? errorCode})> validateRelationshipCreation({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again - this is not enough. type RelationshipTemplate only means that it originates from a RelationshipTemplate, but it could also originate from onExistingRelationship, then we wouldn't create a relationship and instead answer using a message.
Please talk to the runtime team how to handle this properly, I suggest @Milena-Czierlinski as a sparring partner. Maybe we can make the requestDVO event smarter so that we KNOW if it would be answered using a Relationship or a Message ;)
If you discussed this please tell me the results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We think it could be enough to check for localRequestDVO.peer.hasRelationship and then do the validation if its false.
Future<void> _onAcceptButtonPressed() async { | ||
final canCreateRelationship = await widget.validateCreateRelationship?.call(); | ||
|
||
if (canCreateRelationship != null && !canCreateRelationship) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you just return if it's not possible, what do I miss?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inside the widget.validateCreateRelationship?.call() there the error dialog is called when it is not possible
…s://github.com/nmshd/app into check-if-establishing-relationship-is-possible
…s://github.com/nmshd/app into check-if-establishing-relationship-is-possible
Readiness checklist