fix: description in release step #3726

adam-strzelec · 2024-11-18T12:22:56Z

Description

Add Permissions for Payer and Custom - arbitration role and Payment creator for Owner

Testing

Create advanced payment
Go thru entire flow
Go back to release step

Diffs

Changes 🏗

Resolves 3334

CLAassistant · 2024-11-18T12:23:03Z

All committers have signed the CLA.

bassgeta

Well it does in fact do the job, but if we always want to display this, I think we should remove FinalizeByPaymentCreator info so we get rid of some extra lines 😎

bassgeta · 2024-11-19T09:41:55Z

...mpletedAction/partials/PaymentBuilder/partials/PaymentBuilderWidget/PaymentBuilderWidget.tsx

-                    />
-                  )}
-                </>
+                <ActionWithPermissionsInfo


Hmmm? :D Alright so we always display that the payment was released via permissions, if that's what we always need to do (even if the user used their temporary payment creator permissions), we can safely remove FinalizeByPaymentCreatorInfo

mmioana

Hey @adam-strzelec thanks for picking this issue up 🙌

However, I believe this is not working as intended.

If I release the advanced payment using Permissions the right information is displayed

However, releasing the advanced payment using Payment creator the same details as for the previously used decision method (Permissions) is shown, which is not correct

adam-strzelec · 2024-11-28T10:58:09Z

@mmioana Where can i get decision method? From what I see, the decisionMethod in the ReleasePaymentModal is not even sent to the backend anywhere.

jakubcolony · 2024-12-03T00:25:29Z

The permissions vs payment creator selection never worked properly - the saga always uses payment creator if user is such:

While we can tell if permissions or reputation was used, we lack details to tell if permissions were specifically chosen over payment creator.

@arrenv How should we proceed with this one?

We could add a separate payment creator finalization action (but this might be overkill just for history tracking).
Simply prevent payment creators from selecting permissions (assuming there's no real need for them to use permissions). This lets us keep using the existing expenditure owner address check.

arrenv · 2024-12-06T10:14:54Z

@adam-strzelec @jakubcolony

Thank you for the input on this. I think it is useful to be able to communicate the difference of how the payment was released. So, out of the two options provided, I would opt for the second one.

Simply prevent payment creators from selecting permissions (assuming there's no real need for them to use permissions). This lets us keep using the existing expenditure owner address check.

To add to that, I feel it would also be good to have the select field to auto select the option if there is only one. I do think a new issue should be created for this update.

jakubcolony

It looks like there was a copy change that was not specified in the issue. I think this should still read "Payment creator released this payment."

The info is also incorrectly showing payment creator decision method when the payment was finalized by a different user via permissions.

jakubcolony · 2024-12-13T00:52:48Z

...mpletedAction/partials/PaymentBuilder/partials/PaymentBuilderWidget/PaymentBuilderWidget.tsx

-                </>
+                <ActionWithPermissionsInfo
+                  action={finalizingActions?.items[0]}
+                  statusText="action.executed.creator.description"


This needs to show payment creator/permissions depending on the decision method used

mmioana

Thank you for your continuous effort on this one @adam-strzelec 🙌

Now the copy is the correct one for the Release step as the Payment creator

Created another Advanced payment

And tried to finalise it as amy, but it seems I can no longer do that. Is this as intended?

adam-strzelec · 2025-01-02T08:40:15Z

@mmioana I have no idea. @arrenv If I am a person who did not create the action, should I receive such information?

arrenv · 2025-01-02T09:17:35Z

@mmioana I have no idea. @arrenv If I am a person who did not create the action, should I receive such information?

Yes, this is intended, there has to be an available decision method for a non-creator of the Payment to be able to release it.

So, amy would need either the right permissions ("Payer" bundled or Arbitration), or, the Lazy consensus extension would need to be installed and there would need to be reputation in the domain.

mmioana

Hey @adam-strzelec @arrenv thanks for the clarification 🙌

The description correctly updates when releasing as a payment creator

Created another Advanced payment

Then after installing the Reputation extension

And tried to release it as amy, the Reputation decision method was available in the dropdown

However @arrenv @adam-strzelec, if I assign amy the Payer bundled permissions, the permissions is not available to Release the advanced payment ❌

arrenv · 2025-01-14T10:35:55Z

@adam-strzelec It is correct that with "Payer" bundled permissions or "Custom - Arbitration" permissions. You should be able to "Release" the payment.

adam-strzelec · 2025-01-14T16:48:42Z

@mmioana done 🙂

jakubcolony

Thanks for another round of changes @adam-strzelec

As leela (who created the payment), I can correctly see Payment creator as the only available decision method:

As a user without arbitration permission, I get an error as expected:

However, after being assigned Payer role, the newly available method reads "Payment creator" instead of "Permissions":

mmioana

Thanks for your continuous efforts on this issue @adam-strzelec 🙌

Released a payment as Payment creator ✅

Gave amy payer permissions

Released payment using Payer permissions ✅

However the widget from the Release step showed the payment was released as Payment creator, which is not correct ❌

Gave fry Custom-Arbitration permissions

Released payment using Custom-Arbitration permissions ✅

However, also here the same problem with the widget showing the payment was released as Payment creator, which is not correct ❌

mmioana · 2025-01-16T07:27:08Z

...nts/v5/common/CompletedAction/partials/PaymentBuilder/partials/FinalizePaymentModal/hooks.ts

+    userRole?.role === 'payer' ||
+    (userRole?.role === 'custom' &&


Can we please use here UserRole.Payer and UserRole.Custom instead of hard-coded values?

mmioana · 2025-01-16T08:03:12Z

...ts/v5/common/CompletedAction/partials/PaymentBuilder/partials/PaymentBuilderWidget/hooks.tsx

            {selectedFinalizeAction && !selectedFinalizeMotion && (
-              <ActionWithPermissionsInfo action={selectedFinalizeAction} />
+              <FinalizeByPaymentCreatorInfo
+                userAdddress={selectedFinalizeAction.initiatorAddress}
+              />
            )}


Here we should perform a similar check for userIsCreator to decide which description to actually show

adam-strzelec · 2025-01-24T11:09:58Z

@mmioana Do we have any designs for how it should look like when the user uses permissions? The main issue only says very generally that The history of decisions should accurately show what happened and how the decision was made. Should it be Member used permissions to create this action.?

jakubcolony · 2025-01-27T23:36:15Z

@adam-strzelec I believe that's the copy that should be used in case of permissions, it would be consistent with the cancel step:

adam-strzelec · 2025-01-28T12:31:01Z

@bassgeta @jakubcolony @mmioana done 🙂

mmioana

Thanks a lot for all the changes @adam-strzelec it all works as expected now 🥇

Released the payment as Payment creator

Gave amy Payer permissions and released the payment using Permissions

Now gave fry Custom-Arbitration permissions

And released the payment using Permissions

Removed fry's permissions and installed the Reputation extension

Created another payment as fry and used the Reputation decision method for releasing the payment

@adam-strzelec Regarding the wording I think @arrenv is best to provide an input to when releasing a payment using Permissions we should use Member used permissions to create this action.. Personally, I believe Member used permissions to release this payment. would be a better fit. Either way, I'm happy to approve your PR 💯

Really nice work and thanks again for all the changes! 🎉

bassgeta

Thanks for all the hard work on this, looking real good right now 💪
As payment creator ✔️

As member with permissions ✔️

With reputation ✔️

arrenv · 2025-01-29T11:47:54Z

@adam-strzelec Regarding wording as @mmioana has raised, this is definitely the desired behavior, we want these descriptions to be as accurate as possible so, if it can be Member used permissions to release this payment. in this case, that would be preferred.

adam-strzelec · 2025-01-30T09:56:01Z

@arrenv @mmioana @bassgeta I already changed the copy 🙂

mmioana

Thanks a lot @adam-strzelec for your latest copy change as well as for all your efforts on this issue 🙌

jakubcolony

Nice work on this PR, looks good to be merged. Thank you @adam-strzelec 👍

Releasing as payment creator:

Releasing as another user with permissions:

Trying to release as a user without permissions:

Releasing using reputation:

AdrianDudko · 2025-02-04T09:15:24Z

@bassgeta Can you please take a look at this PR? We have 2 approvals already :)

adam-strzelec self-assigned this Nov 18, 2024

adam-strzelec requested a review from a team as a code owner November 18, 2024 12:22

bassgeta requested changes Nov 19, 2024

View reviewed changes

adam-strzelec requested a review from bassgeta November 20, 2024 15:06

mmioana requested changes Nov 21, 2024

View reviewed changes

adam-strzelec requested a review from mmioana December 9, 2024 09:11

jakubcolony requested changes Dec 13, 2024

View reviewed changes

strzelec and others added 2 commits December 19, 2024 10:32

fix: remove finalizeByPaymentCreator component

6e55e5c

fix: undo previous overview and remove permissions from modal

4592627

adam-strzelec force-pushed the fix/16120160-release-step-description branch from d808b86 to 4592627 Compare December 19, 2024 10:29

adam-strzelec requested a review from jakubcolony December 19, 2024 10:31

mmioana reviewed Dec 20, 2024

View reviewed changes

adam-strzelec requested a review from mmioana January 2, 2025 09:31

mmioana requested changes Jan 13, 2025

View reviewed changes

adam-strzelec added 2 commits January 14, 2025 17:32

fix: add payment creator for user who is payer or custom

808fc66

fix: add creator permissions for specific roles

5679d5a

adam-strzelec requested a review from mmioana January 14, 2025 16:48

jakubcolony requested changes Jan 15, 2025

View reviewed changes

fix: add permissions for payer and for custom arbitration

e74da52

adam-strzelec requested a review from jakubcolony January 15, 2025 09:44

mmioana mentioned this pull request Jan 15, 2025

[Permission] Permission decision method not visible on the release step for users with Arbitration permission #3779

Closed

mmioana requested changes Jan 16, 2025

View reviewed changes

fix: change title when user used permissions

02e9eab

adam-strzelec requested a review from mmioana January 28, 2025 12:30

mmioana previously approved these changes Jan 28, 2025

View reviewed changes

bassgeta previously approved these changes Jan 29, 2025

View reviewed changes

fix: change header copy for user who used permissions

d6d1b7d

adam-strzelec dismissed stale reviews from bassgeta and mmioana via d6d1b7d January 30, 2025 09:54

adam-strzelec requested review from mmioana and bassgeta January 30, 2025 09:56

mmioana approved these changes Jan 30, 2025

View reviewed changes

jakubcolony approved these changes Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: description in release step #3726

fix: description in release step #3726

adam-strzelec commented Nov 18, 2024 •

edited

Loading

CLAassistant commented Nov 18, 2024 •

edited

Loading

bassgeta left a comment

bassgeta Nov 19, 2024

mmioana left a comment

adam-strzelec commented Nov 28, 2024 •

edited

Loading

jakubcolony commented Dec 3, 2024

arrenv commented Dec 6, 2024

jakubcolony left a comment

jakubcolony Dec 13, 2024

mmioana left a comment

adam-strzelec commented Jan 2, 2025

arrenv commented Jan 2, 2025 •

edited

Loading

mmioana left a comment

arrenv commented Jan 14, 2025

adam-strzelec commented Jan 14, 2025

jakubcolony left a comment

mmioana left a comment

mmioana Jan 16, 2025

mmioana Jan 16, 2025

adam-strzelec commented Jan 24, 2025

jakubcolony commented Jan 27, 2025

adam-strzelec commented Jan 28, 2025

mmioana left a comment

bassgeta left a comment

arrenv commented Jan 29, 2025

adam-strzelec commented Jan 30, 2025 •

edited

Loading

mmioana left a comment

jakubcolony left a comment

AdrianDudko commented Feb 4, 2025

		userRole?.role === 'payer' \|\|
		(userRole?.role === 'custom' &&

fix: description in release step #3726

Are you sure you want to change the base?

fix: description in release step #3726

Conversation

adam-strzelec commented Nov 18, 2024 • edited Loading

Description

Testing

Diffs

CLAassistant commented Nov 18, 2024 • edited Loading

bassgeta left a comment

Choose a reason for hiding this comment

bassgeta Nov 19, 2024

Choose a reason for hiding this comment

mmioana left a comment

Choose a reason for hiding this comment

adam-strzelec commented Nov 28, 2024 • edited Loading

jakubcolony commented Dec 3, 2024

arrenv commented Dec 6, 2024

jakubcolony left a comment

Choose a reason for hiding this comment

jakubcolony Dec 13, 2024

Choose a reason for hiding this comment

mmioana left a comment

Choose a reason for hiding this comment

adam-strzelec commented Jan 2, 2025

arrenv commented Jan 2, 2025 • edited Loading

mmioana left a comment

Choose a reason for hiding this comment

arrenv commented Jan 14, 2025

adam-strzelec commented Jan 14, 2025

jakubcolony left a comment

Choose a reason for hiding this comment

mmioana left a comment

Choose a reason for hiding this comment

mmioana Jan 16, 2025

Choose a reason for hiding this comment

mmioana Jan 16, 2025

Choose a reason for hiding this comment

adam-strzelec commented Jan 24, 2025

jakubcolony commented Jan 27, 2025

adam-strzelec commented Jan 28, 2025

mmioana left a comment

Choose a reason for hiding this comment

bassgeta left a comment

Choose a reason for hiding this comment

arrenv commented Jan 29, 2025

adam-strzelec commented Jan 30, 2025 • edited Loading

mmioana left a comment

Choose a reason for hiding this comment

jakubcolony left a comment

Choose a reason for hiding this comment

AdrianDudko commented Feb 4, 2025

adam-strzelec commented Nov 18, 2024 •

edited

Loading

CLAassistant commented Nov 18, 2024 •

edited

Loading

adam-strzelec commented Nov 28, 2024 •

edited

Loading

arrenv commented Jan 2, 2025 •

edited

Loading

adam-strzelec commented Jan 30, 2025 •

edited

Loading