Skip to content

csv export for accounting posts (64017) #251

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

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

Conversation

svenwey
Copy link
Collaborator

@svenwey svenwey commented Apr 4, 2025

No description provided.

@svenwey svenwey requested a review from kronn April 4, 2025 11:20
@svenwey svenwey self-assigned this Apr 4, 2025
@svenwey svenwey changed the base branch from master to feature/63950-make-accountingposts-filterable April 4, 2025 11:21
@svenwey svenwey changed the title Feature/64017 csv export for positions csv export for accounting posts (64017) Apr 4, 2025
@svenwey svenwey marked this pull request as draft April 4, 2025 11:24
@svenwey svenwey marked this pull request as ready for review April 4, 2025 11:55
@svenwey svenwey changed the base branch from feature/63950-make-accountingposts-filterable to master April 4, 2025 11:58
@svenwey svenwey marked this pull request as draft April 4, 2025 11:58
@svenwey svenwey marked this pull request as ready for review April 4, 2025 12:17
@svenwey svenwey changed the base branch from master to feature/63950-make-accountingposts-filterable April 4, 2025 12:17
@Kagemaru Kagemaru force-pushed the feature/63950-make-accountingposts-filterable branch from 298de57 to a3964d8 Compare April 25, 2025 14:07
Base automatically changed from feature/63950-make-accountingposts-filterable to master April 25, 2025 14:30
@Kagemaru Kagemaru force-pushed the feature/64017-csv_export_for_positions branch from 4431b58 to f4fa78b Compare April 25, 2025 16:29
private

def cockpit_csv_filename
basename = Order::Services::CsvFilenameGenerator.new(order, params).filename
"#{@period.start_date}_#{@period.end_date}_".parameterize + basename
Copy link
Member

Choose a reason for hiding this comment

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

2 little nitpicks:

  1. It'd be nice to have the name of the model in the filename.
  2. If there is no start_date, this will lead to a filename like _2025-04-25_puzzletime-BLS-DIS.csv

You can orient yourself on this method:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have adapted the file naming. However, I also don't really like this naming schema, since the periods egal - 01.01.2025 and 01.01.2025 - 01.01.2025 both receive the same filename (although the contents might/will differ greatly).

Copy link
Member

Choose a reason for hiding this comment

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

Right, didn't know that detail. Can you do a special handling for one day, and we open a PTime Ticket for a centralized naming Helper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that sounds great. There is a similar issue, where the periods egal - 01.01.2025 and 01.01.2025 - egal also receive the same name. So thinking about this and implementing this cleanly would be nice.

Temporarily, I would've just repeated the date in the case of one day (i.e. accounting_posts_2025-01-01_2025_01_01.csv). And also, I would just put egal there if there is no start/end date. This should lead to no collisions.

Copy link
Member

@Kagemaru Kagemaru May 20, 2025

Choose a reason for hiding this comment

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

Alright, we'll use this for now and then track the centralized helper in Ticket #64226.

Do you need to change something, for the 'egal' naming rules, or can I merge this?

@svenwey svenwey requested a review from Kagemaru April 29, 2025 06:35
Copy link
Member

@Kagemaru Kagemaru left a comment

Choose a reason for hiding this comment

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

Approved after we solve the filename discussion

@svenwey
Copy link
Collaborator Author

svenwey commented May 8, 2025

I now made a change to disambiguate the filename (se answer above)

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