RIP-Comm
feat(transaction): duplicate transaction
This PR add the functionality to duplicate a transaction from an existing one.
Steps to try it out:
- Go to a transaction page
- Click in the top left corner on the copy Icon
- Click "Duplicate" in the alert dialog
A new transaction with the exact same fields should be created with only a different id and a suffix (copy) in the note
This PR also added a new test for the new functionality of the .copy() method.
- Closes #222
@fres-sudo did you try to copy a recurring transaction? When I do it I don't see the account amount updating, if you can reproduce it but the fix is not straightforward maybe you could try converting the copied transaction to a normal one and not copy it as a recurring one
@theperu Thanks for the review! I don't sincerely understand why a user would ever need to duplicate a recurring transaction, please clarify to me this use case, probably I am missing smtng. I mean, if It's a recurring transaction, why do you want to duplicate it?
Here is an example that comes to my mind, I have a chess lesson every week but maybe every once in a while I might do it twice a week and I might want to open the last transaction and copy it.
Just to clarify, I'm talking about copying a transaction from the last transactions section and not on the planning page. If it's still not clear send a message on the discord and we can talk about it tomorrow in the voice chat which will make it much easier to explain π
Ah okay, okay, so you mean copying just the transaction itself, even if it's a recurring one. I got it, my bad, sorry. I misunderstood. I didn't try it yet, I'll do it and fix the problem, thanks a lot!
I want to suggest another thing: when a transaction is copied, why not open the transaction just created? Otherwise, if i copy a transaction of, let's say, last month, when the operation finish the transactions list gets me on the top, and i need to scroll to change the date, the amount, or just to remove (copy) text.
@mikev-cw the issue with that approach is that the screen of the transaction would be basically identical to the page that you are coming from (except for the name)
@mikev-cw Thanks for the suggestion, I already gave my opinion on that, and my suggestions, here #222
Ok, I see. Sorry for the mess guys, you already well pointed the issue. Back to us, the scroll behavior is very tricky... What do you think if we:
- scroll in some way to the transaction just created (or just avoid to scroll to the top when the full process has finished) OR
- at least request to the user if he wants to edit the newly created transaction, and in that case we open it
Consider also that when the copied transaction is created, is not always just ahead the original one (I think that's because have higher IDs), making even more difficult to find it
at least request to the user if he wants to edit the newly created transaction, and in that case we open it
I think this solution will make a lot of friction in the overall UX...too much steps for a simple duplication. I'll try to implement the scroll to the duplicated transaction and I'll get back to you, wdyt?
@mikev-cw, I'm back. Sorry for the delay.
I was thinking about the "scroll implementation," and I believe it's quite unfeasible for a couple of reasons:
-
If the user duplicates a transaction by navigating to the transaction page from the home page, thereβs a possibility that after the duplication, when the user is redirected back to the home page, the duplicated transaction may not be present. The home page only displays a limited number of transactions, not the entire history, so thereβs a concrete chance that the duplicated transaction won't be visible, which means scrolling wouldn't be possible.
-
Implementing an actual scroll system would require a global state (just for this feature).
-
The entire
transaction_list.dartwould need to be rethought since it currently usesSingleChildScrollView, which, as far as I know, cannot provide the same level of granular control compared toListView.builder.
One possible solution, to give a better feedback to the user, would be to show a snack bar, displaying for example:
"{transaction_title} (copy) has been created"
Or something like that.
What do you think?
@fres-sudo Got it. Thanks for the analysis. Any chance to return some data from the insert, useful for making the toast/snackbar tappable for editing?
"{transaction_title} (copy) has been created. Tap to edit"
Great Idea, I think yes, I can manage to do it quite easily, I'll try to implement this flow:
user duplicate transaction β redirect to home/transactions page β show snack bar (with CTA) β if the user click on the snack bar he gets redirected to the transaction page of the duplicated transaction.
Any ideas on that?
ElevatedButton(
style: ElevatedButton.styleFrom(
padding: const EdgeInsets.symmetric(horizontal: 16.0),
),
onPressed: () =>
ref.read(transactionsProvider.notifier).duplicateTransaction(transaction).then((t) {
if (context.mounted) {
Navigator.of(context)
..maybePop()
..maybePop();
ScaffoldMessenger.of(context).showSnackBar(
SnackBar(
content: t != null
? Row(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
children: [
Expanded(
child: Text(
"${t.note} has been created.",
overflow: TextOverflow.ellipsis,
),
),
ElevatedButton(
onPressed: () {
if (context.mounted) {
ref
.read(transactionsProvider.notifier)
.transactionUpdateState(t);
Navigator.of(context).pushNamed("/add-page",
arguments: {'recurrencyEditingPermitted': !t.recurring});
}
},
child: Text("Edit")),
],
)
: const Text("Error duplicating transaction"),
duration: const Duration(seconds: 5),
),
);
}
}),
The problem here is clear. I am calling ref too late, at the time I call the ref to update the transaction state the context is not there anymore...is already popped.
Updating the state with
ref.read(transactionsProvider.notifier).transactionUpdateState(t);even before the user clicks on "Edit" is actually an option (dirty IMO, but still an option), but the problem raise also later, as I'll point in the following lines.
The problem is certainly the coupling being made in the Provider...too many responsibilities: holding the state for the current selected transaction and performing CRUDs.
TBH I dont have any solution on that, I am not deep at all in Riverpod, with a BloCapproach my solution would have been something like the following snippet:
BlocListener<TransactionBloc, TransactionState> (
listener: (context, state) => switch (state) {
DuplicatedTransactionState(:final transaction) => showSnachBar(....,
// If the user click on the "Edit" button
() => Navigator.of(context).push...AddPage(transaction: transaction))
}
)
This is just some example code, the issue that I am trying to point are the following:
- For no reason the Provider (that is actually the same that is responsible to perform CRUDs) is holding the state of the selected transactoin, in order to show it when the user navigate to the "AddPage", I mean, why? Why we can't just pass the selected transactoin as prop, so do something like:
AppPage(transaction: transaction))
- Since the Provider has more responsabilities then it should we can't just listen at the state changes of the provider (
ref.listenthat similiar toBlocListener, for what I understand), because it's busy to hold the state of the current selected transaction.
So, even if it's out of scope for this PR, what do you think about a refactoring in the current state managing?
In addition to this also a refactoring of the current models is a must, we can't hold the query logic inside the models. It's not strictily related to this PR, it's just a general issue.
Hello @fres-sudo
Have you tried passing the result from the .pop() back to the widget that did the .push() instead of handling everything in the .then()?
Something like:
Source Widget:
Button(
onPressed: () async {
await Navigator.of(context).push(destination).then((res) {
if (res != null) // show success snackbar with edit
else // show error snackbar
});
}
)
Destination Widget:
//in the button onPressed
ref.read(transactionsProvider.notifier).duplicateTransaction(transaction).then((t) {
if (t != null) Navigator..pop()..pop(t);
// then handle the snackbar in source widget.
});
result from .pop() is a generic type so you should be able to pass the duplicated transaction id as well, so that you can use it to navigate to the transaction page once you are back to the Source Widget.
.pop() docs here
I don't know if I've explained it in a way that's understandable π Please reach out if you have questions and we can have a look together (on Discord if you want, same username π )
Is this the intended flow?
https://github.com/user-attachments/assets/4d7816f1-2b8d-4c1e-a44a-42f56e2fe3d8
Is this the intended flow?
Yes it is! How did you achieve it? What did I miss?
As explained here (not so well π )
I'm returning the duplicated transaction with the second .pop().
To be able to do so, I had to modify the duplicateTransaction method from lib\providers\transactions_provider.dart to have a return type of Future<Transaction> instead of Future<void> so that the duplicated transaction is returned when calling ref.read(transactionsProvider.notifier) .duplicateTransaction(transaction).
Like this:
Future<Transaction> duplicateTransaction(Transaction transaction) async {
final duplicatedTransaction = transaction.copy(
id: null,
note: "${transaction.note} (copy)",
);
state = const AsyncValue.loading();
state = await AsyncValue.guard(() async {
await TransactionMethods().insert(duplicatedTransaction);
return _getTransactions(update: true);
});
return duplicatedTransaction;
}
In this way you can do this from duplicate_transaction_dialog.dart:
onPressed: () => ref
.read(transactionsProvider.notifier)
.duplicateTransaction(transaction)
.then((transaction) {
if (context.mounted) {
Navigator.of(context)
..pop()
// pass the updated transaction back to the caller of this route.
// note that I'm doing it in the second pop as this is the one which brings us back to the dashboard page.
..pop(transaction);
}
}
),
Then, in lib/custom_widgets/transactions_list.dart, where the /add_page is pushed, we can modify the .pushNamed() function to use that transaction we are passing back from the previous code block, like so:
Navigator.of(context).pushNamed("/add-page", arguments: {
'recurrencyEditingPermitted': !transaction.recurring
}).then((transaction) {
if (transaction == null) return;
// probably not necessary, we can just use transaction
var _transaction = transaction as Transaction;
// use _transaction to show it in your SnackBar
// then, from the SnackBar's button you call something like:
if (context.mounted) {
ref
.read(transactionsProvider.notifier)
.transactionUpdateState(_transaction);
Navigator.of(context).pushNamed("/add-page",
arguments: {
'recurrencyEditingPermitted':
!transaction.recurring
});
}
});
This works as you can see (minus the plus button on the bottom nav that jumps up with the SnackBar π ) but, as I'm typing this, I'm realizing that this implementation has a flaw, because it doesn't work if you push the /add_page from anywhere else that's not the Last transactions section.
We could probably look into solving this issue.
Maybe we could have a lastDuplicatedTransactionProvider that stores the transaction (we just need to keep track of the last duplicated transaction anyway), or we could add the code for handling the returned transaction from everywhere the /add_page is pushed (I think I would prefer the first approach).
Edit: actually, the issue I've mentioned above it shouldn't be there because the code that receives back the transaction is in the TransactionTile which is the only Widget that displays transaction (that's good). But, when doing this flow from the Transaction tab in the bottom nav, I'm getting this error:
E/flutter ( 6600): [ERROR:flutter/runtime/dart_vm_initializer.cc(40)] Unhandled Exception: Looking up a deactivated widget's ancestor is unsafe.
E/flutter ( 6600): At this point the state of the widget's element tree is no longer stable.
E/flutter ( 6600): To safely refer to a widget's ancestor in its dispose() method, save a reference to the ancestor by calling dependOnInheritedWidgetOfExactType() in the widget's didChangeDependencies() method.
I'm trying to understand where this is coming from and why it works from one place but it doesn't from another π€
As explained https://github.com/RIP-Comm/sossoldi/pull/234#issuecomment-2734435859 (not so well π )
Damn sorry, idk why I completely miss that message, I only saw the video...forgive me π.
I had to modify the duplicateTransaction method
Yes ofc, I already did that in my implementation.
This works as you can see (minus the plus button on the bottom nav that jumps up with the SnackBar π ) but, as I'm typing this, I'm realizing that this implementation has a flaw, because it doesn't work if you push the /add_page from anywhere else that's not the Last transactions section.
I already thought about it, so passing down the created transaction to the pop() method, the issue that I encountered was actually this Λ.
So, since you can navigate to the /add-page from 2 different places (home page and transactions page), you have to add this kind of implementation:
Navigator.of(context).pushNamed("/add-page", arguments: {
'recurrencyEditingPermitted': !transaction.recurring
}).then((transaction) {
to both the places, and in my head this could never work out. But apparently I was wrong.
I saw that you had a similar thought
we could add the code for handling the returned transaction from everywhere the /add_page is pushed (I think I would prefer the first approach).
About a better implementation:
Maybe we could have a lastDuplicatedTransactionProvider that stores the transaction (we just need to keep track of the last duplicated transaction anyway)
I agree to proceed with this approach, and I also think that this new provider should also contain the logic to duplicate the transaction (it should call the repository actually, but we don't have this layer yet...).
And so we listen for its state changes, and we update the UI (in this case push the route) whenever a new transaction is duplicated. So yes, it actually stores the last duplicated transaction. In my Bloc-way to see the state managing, performing the method and updating the state must exist in the same "class" (or notifier, I guess, in a more Riverpod-way, probably I am wrong, correct me, with Sossoldi is my first time using Riverpod).
Thanks for the deep dive, and let me know what did you think about this.
Cheersπ€
I agree, probably a better structured provider would be convenient here.
I did try using a simple StateProvider that just holds an instance of the last duplicated transaction (which is assigned from the dialog) and actually made it work for both the Dashboard page and the transaction list that's showed when tapping one of your accounts.
I'm struggling with the Transactions page and I think it has to do with it being a Sliver and probably not having a Scaffold ancestor
I'm struggling with the Transactions page and I think it has to do with it being a Sliver and probably not having a Scaffold ancestor
I see, I'll check on that, and maybe I'll add a Scaffold() directly.
Made the changes!
@bongio94 I didn't find the issue you were pointing to tbh, this one:
I'm struggling with the Transactions page and I think it has to do with it being a Sliver and probably not having a Scaffold ancestor
When you have time, can you please take a look to check if the flow is working correctly?
Yeah I see, I was probably doing it wrong!
I'll go through the PR now (but I only have 10 minutes, I'll continue this evening).
Given we don't have a unified way of formatting code, it would be helpful if any change that is formatting was discarded before pushing the commit, this would make doing review much easier π
Given we don't have a unified way of formatting code, it would be helpful if any change that is formatting was discarded before pushing the commit, this would make doing review much easier π
I seeππ Indeed most of the times I work with the formatOnSave off to avoid having dirty diff, but in some situation it became a mess if I don't format. Forgive me plz.
P.S. prolly if u enable the option "Hide Whitespaces" you could have a better diff.
Very good work guys! Loved how you shared your competences ππ» Let's merge