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 logic for motionDomainId in Payment & Permissions dialogs #3077

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

chinins
Copy link
Contributor

@chinins chinins commented Dec 27, 2021

Description

This PR fixes the motion domain ID that is sent by the dialog to the saga.

To test it you need to create a payment & permissions motion in different domains (at least Root & another domain) by changing From domain (and not directly motion domain ID). This should change the motion domain id. You then need to log the values sent to the saga (on master there is another bug that doesn't display correct domain ids in the motions, this is being fixed by a separate PR #3073

@rdig, I would like a review from you at some point as it touches the code that you were implementing (if I recall correctly). I would like to make sure that I am not misunderstanding the logic and the implementation.

Resolves #3074

@chinins chinins added the bug label Dec 27, 2021
@chinins chinins self-assigned this Dec 27, 2021
@chinins chinins marked this pull request as draft December 27, 2021 21:40
@chinins chinins requested a review from a team December 27, 2021 21:44
@chinins chinins marked this pull request as ready for review December 27, 2021 21:44
Copy link
Member

@arrenv arrenv left a comment

Choose a reason for hiding this comment

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

Fix is working well for me, very nice!

Before

wrong-team-for-motion-before

After

wrong-team-for-motion-after

Copy link
Contributor

@ArmandoGraterol ArmandoGraterol left a comment

Choose a reason for hiding this comment

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

Working as expected now! Good job

@chinins chinins force-pushed the fix/3074-correct-dialogs-motion-domain-id branch from b23e6a8 to c1faee2 Compare January 17, 2022 21:38
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

This make sense, but I haven't had time to review all cases.

Initial tests shows everything is working fine, but give it a final test before merging make sure all cases (when creating a motion in a sub-domain) work as expected

@chinins chinins force-pushed the fix/3074-correct-dialogs-motion-domain-id branch from c1faee2 to 67dd589 Compare January 19, 2022 17:07
@chinins
Copy link
Contributor Author

chinins commented Jan 19, 2022

@rdig, thank you! I've tested again and it's all working fine.

@chinins chinins merged commit d6cd2e4 into master Jan 19, 2022
@chinins chinins deleted the fix/3074-correct-dialogs-motion-domain-id branch January 19, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants