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

[14.0][IMP] account_statement_import_camt54 add the parameter to the view #607

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

Conversation

ecino
Copy link

@ecino ecino commented Jun 29, 2023

The transfer_line parameter wasn't added to the view although it was defined in the model.

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 29, 2023
@ecino
Copy link
Author

ecino commented Oct 30, 2023

@alexis-via Willing to review this small PR?

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Nov 5, 2023
@ecino
Copy link
Author

ecino commented Feb 27, 2024

@OCA/banking-maintainers Anyone willing to review?

@StefanRijnhart
Copy link
Member

I'm really disappointed that this feature was added on the camt54 level rather than generically. This aspect is missing from the field's help text. It suspiciously uses amount == 0 rather than res.currency's is_zero and the feature was snuck in in a module migration without any mention in the PR description rather than in its own PR.

Are you using this feature? Otherwise I'd say maybe keep it hidden until it's improved.

@ecino ecino force-pushed the 14.0-imp-view-camt branch from bc444f4 to fcb637c Compare March 7, 2024 14:35
@ecino
Copy link
Author

ecino commented Mar 7, 2024

Yes we think this feature is useful and are using it. I made a few changes based on your comments. I'm not convinced the feature would be useful at the generic level though.

@StefanRijnhart
Copy link
Member

Thanks for addressing my rant so gracefully! Please use the prefix [IMP] account_statement_import_camt54: in all commits (I would squash these together myself).

simgonzalez and others added 2 commits March 7, 2024 15:54
The parameter wasn't added to the view although it was defined in the model.
- Use currency.iz_zero() instead of direct comparison
- Add information about camt54 usage in the help of the field
@ecino ecino force-pushed the 14.0-imp-view-camt branch from fcb637c to 0301ca7 Compare March 7, 2024 14:55
@ecino
Copy link
Author

ecino commented Mar 7, 2024

You're welcome 😉
I reworded the commits.

Copy link

github-actions bot commented Jul 7, 2024

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 7, 2024
@StefanRijnhart StefanRijnhart added no stale Use this label to prevent the automated stale action from closing this PR/Issue. and removed stale PR/Issue without recent activity, it'll be soon closed automatically. labels Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no stale Use this label to prevent the automated stale action from closing this PR/Issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants