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

Bugfix 641: Weekday is ignored in scheduled exports #680

Merged

Conversation

rivaldi8
Copy link
Collaborator

Am I late for 2.2?

Please, check I haven't missed any corner case :)

The actions were run just once per week without taking into account the
weekdays set by the user.

Fixes codinguser#641
@codinguser
Copy link
Owner

codinguser commented Apr 25, 2017 via email

Copy link
Owner

@codinguser codinguser left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, thanks.
Could you please check the failing tests on Travis though?
Do the tests need updating, or does the code break something?

@rivaldi8
Copy link
Collaborator Author

Sorry, I forgot to switch back to the full unit test suite before sending the pull request. It seems the tests have to be updated to set byDays for weekly actions.

Today I have a busy day. I'll fix it tomorrow.

@rivaldi8
Copy link
Collaborator Author

It should be fixed now.

@codinguser
Copy link
Owner

Cool, thanks. So byday is always set by the recurrence dialog, right?

@codinguser codinguser merged commit 893132a into codinguser:develop Apr 27, 2017
@rivaldi8
Copy link
Collaborator Author

Yes, the dialog forces you to set at least on weekday. However, after a more thorough inspection, I've found GncXmlHandler never sets the recurrence's byDays. It doesn't parse the weekdays.

Until we don't implement the parsing for imported weekly transaction, I'd add at least one weekday to byDays. We could just use the weekday of the start date. That way it would be executed at least once per week. In fact I'd say that's how it worked until now.

Also, although with a warning, GnuCash desktop allows you to define a weekly transaction without setting any weekday. In that case, it's never executed. So, I'll have to add a check for empty byDays too.

@codinguser
Copy link
Owner

@rivaldi8 sounds like a plan 👍

@rivaldi8 rivaldi8 deleted the bugfix-641-exports-weekday-ignored branch April 29, 2017 15:26
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.

2 participants