macxred / cashctrl_api

Python client for the CashCtrl REST API
MIT License
0 stars 0 forks source link

Feat/create non existent account groups #27

Closed AlexTheWizardL closed 2 months ago

AlexTheWizardL commented 2 months ago

Pull Request: Update Category Handling in CashCtrlClient

Summary

This pull request introduces several updates and improvements to the CashCtrlClient class, particularly focusing on the handling of category updates. It includes new tests to validate these changes, ensuring robustness and correctness.

Changes

New Test Cases

  1. test_update_account_categories_with_list
    • Ensures that updating account categories with a list type raises a ValueError.
  2. test_update_file_categories_with_dict
    • Ensures that updating file categories with a dictionary type raises a ValueError.
  3. test_account_category_update
    • Tests the full cycle of updating account categories and restoring the initial state.

Code Updates

  1. Handling Parent ID for Account Categories

    elif resource == 'account':
        params['parentId'] = categories.get(parent_path, 1)
    if isinstance(target, dict) and target[category]:
        params['number'] = target[category]
  2. Updating Account Category Numbers

    if resource == 'account':
        dict_df = pd.DataFrame(list(target.items()), columns=['path', 'new_number'])
        merged = category_list.merge(dict_df, on='path', how='inner')
        merged.dropna(subset=['number', 'new_number'], inplace=True)
        update = merged[merged['number'] != merged['new_number']]
        for row in update.to_dict('records'):
            params = {
                'id': row['id'],
                'name': row['path'],
                'number': row['new_number'],
                'parentId': row['parentId']
            }
            self.post('account/category/update.json', params=params)
  3. Target Type Validation

    if resource == 'account' and not isinstance(target, dict):
        raise ValueError('Accounts target should be a dict of groups and associated account numbers')
    elif resource != 'account' and isinstance(target, dict):
        raise ValueError('Target categories should be a list for this resource')
  4. Update Categories Method Signature

    def update_categories(self, resource: str, target: List[str] | dict, delete: bool = False):
  5. Adding Number Column for Account Categories

    if resource == 'account':
        accounts = pd.DataFrame(self.get("account/list.json")['data'])
        accounts = accounts.groupby('categoryId', as_index=False).agg({'number': 'min'})
        df.drop(columns=['number'], inplace=True)
        df = pd.merge(df, accounts, how='left', left_on='id', right_on='categoryId')
  6. Enforcing Data Types

    df = enforce_dtypes(df, CATEGORY_COLUMNS, optional={'number': 'Int64'})
  7. Path Handling

    path = f"{parent_path}/{node['text'].replace('/', '\\/')}"

Conclusion

@lasuk Please review the changes and provide feedback or approval for merging. There are not so many changes, so you can point to any part of code that you see we can improve

lasuk commented 2 months ago

Thanks for the modifications. I've added further modifications as fdabbe8. @AlexTheWizardL: Please shout if you disagree.

AlexTheWizardL commented 2 months ago

@lasuk Your changes look good to me

lasuk commented 2 months ago

The code looks good to me and passes all tests.

However, the result does not correspond to what I would expect. After running the unit tests up to L94, I see below category tree in the gui, where new nodes are attached under 'Assets'. However, the 'Assets' node was not part of our specification:

account_categories = {
    '/Anlagevermögen/hello': 1000,
    '/Anlagevermögen/world/how/are/you/?': 1010,
    '/Anlagevermögen/feeling/kind/of/warm': 1020,
    '/Anlagevermögen/are': 1020,
    '/Anlagevermögen/you?': 1020,
    '/Anlagevermögen/Finanzanlagen': 6000,
}
image