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

Feat/15873666 edit #3320

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feat/15873666 edit #3320

wants to merge 2 commits into from

Conversation

CzarekDryl
Copy link
Contributor

Description

This PR adds the ability to edit actions after the locked state during the Funding, Release steps. Editing in Payment step is not possible for now.

  • Edit Mode:

    • Displays yellow border and Edit Pill.
    • Disables stepper buttons and hides Kebab menu.
    • Makes Recipients, Amounts, and Claim Delays editable.
    • Change payment button opens modal, disabled until changes are made.
  • Change Payment Modal:

    • Dropdown for execution options (Reputation and Permissions).
  • Changes Step in Stepper:

    • Changes pill appears after edits in Funding, Release step.
    • Shows details on edits, permissions, and a change counter.
  • If there is funding needed after editing, new pill should show after changes one

Testing

  • Install Lazy Consensus extension to test funding with motion
  • Create advanced payment action and lock it
  • Play around with funding and editing

Closed PR with comments - #3075

Design

Resolves #2238

@CzarekDryl CzarekDryl self-assigned this Oct 14, 2024
@CzarekDryl CzarekDryl requested review from a team as code owners October 14, 2024 11:00
@CzarekDryl CzarekDryl changed the base branch from master to feat/advanced-payments October 14, 2024 11:00
Base automatically changed from feat/advanced-payments to master October 14, 2024 22:14
@jakubcolony
Copy link
Collaborator

This one also needs a rebase @CzarekDryl

@CzarekDryl
Copy link
Contributor Author

@jakubcolony @arrenv Branch rebased and ready to review

Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

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

Nice work overall @CzarekDryl, this was not a simple feature to handle 👍

Some notes from my testing:

  • Missing translations:
image image
  • Should the Make changes button be disabled if there are no changes? Otherwise it is just sending empty transactions.

  • After making changes, the values only update after a few seconds. I can't see this mentioned in the spec, but we may want to update them manually in cache so that they're visible to the user straightaway.

  • If amount is 0, the row should be hidden, that is how they get deleted on the contracts level:

image
  • When I select an edit action, should it be the slots after the changes that are shown? That's what seems to be the case, but they change order when clicked on (could be also related to the issue below):
image image
  • The payment table should be sorted by slot ID, after editing the slots returned from the backend are not necessarily in order (see slot with ID 1 is displayed under slot with ID 2):
image image
  • I feel like this could be related to the above, I edited only the claim delay but the UI mentions 6 changes:
"expenditureSlotChanges": {
  "oldSlots": [
    {
      "id": 1,
      "recipientAddress": "0xb77D57F4959eAfA0339424b83FcFaf9c15407461",
      "claimDelay": "0",
      "payouts": [
        {
          "amount": "1000000000000000000000",
          "tokenAddress": "0xeF841fe1611ce41bFCf0265097EFaf50486F5111"
        }
      ]
    },
    {
      "id": 2,
      "recipientAddress": "0x9dF24e73f40b2a911Eb254A8825103723E13209C",
      "claimDelay": "3600",
      "payouts": [
        {
          "amount": "2000000000000000000000",
          "tokenAddress": "0xeF841fe1611ce41bFCf0265097EFaf50486F5111"
        }
      ]
    }
  ],
  "newSlots": [
    {
      "id": 2,
      "recipientAddress": "0x9dF24e73f40b2a911Eb254A8825103723E13209C",
      "claimDelay": "3600",
      "payouts": [
        {
          "amount": "2000000000000000000000",
          "tokenAddress": "0xeF841fe1611ce41bFCf0265097EFaf50486F5111"
        }
      ]
    },
    {
      "id": 1,
      "recipientAddress": "0xb77D57F4959eAfA0339424b83FcFaf9c15407461",
      "claimDelay": "18000",
      "payouts": [
        {
          "amount": "1000000000000000000000",
          "tokenAddress": "0xeF841fe1611ce41bFCf0265097EFaf50486F5111"
        }
      ]
    }
  ]
}
  • After making a change that required another funding step, I'm being asked to fund the entire amount instead of just the difference between payout sum and current expenditure balance for a given token

  • Adding slots seems to work fine ✅

image
  • Editing in different steps shows correctly in the list, nicely done:
image

Comment on lines +169 to +176
const getEditContent = () => {
switch (actionType) {
case ColonyActionType.CreateExpenditure:
return <PaymentBuilderEdit action={action} />;
default:
return <div>Not implemented yet</div>;
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be handled at the level of PaymentBuilder component? It would keep the generic CompletedAction free of feature-specific logic

@arrenv
Copy link
Member

arrenv commented Oct 30, 2024

Should the Make changes button be disabled if there are no changes? Otherwise it is just sending empty transactions.

Preferably not disabled, but, we should validate on click if no changes have been made. With a validation message: "No changes have been made yet."

After making changes, the values only update after a few seconds. I can't see this mentioned in the spec, but we may want to update them manually in cache so that they're visible to the user straightaway.

My concern with showing cached values is if there is an issue with the transaction, you are going to get a flashing of the new then back to old values. My thinking here is that it would probably be better to show a skeleton loading state on the whole table. This could be done in a separate issue.

If amount is 0, the row should be hidden, that is how they get deleted on the contracts level:

Yes, I agree, the ones with 0 values should be hidden.

The payment table should be sorted by slot ID, after editing the slots returned from the backend are not necessarily in order (see slot with ID 1 is displayed under slot with ID 2):

Good pickup Jakub, yes, it would be good to have a consistent order.

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.

Payment builder - Edit action - After Locked state - Action Creator path with Permissions
3 participants