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

[Feature] Implementing "Bulk Updates" Actions For Expenses && Recurring Invoices #2127

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

Civolilah
Copy link
Collaborator

@Civolilah Civolilah commented Oct 14, 2024

@beganovich @turbo124 The PR includes the implementation of the "bulk_updates" bulk action for Expenses and Recurring Invoices. The only available field is the tax selector for tax columns. Screenshot:

Screenshot 2024-10-15 at 07 53 09

Note: @turbo124 Before testing, please make sure that the bulk_updates property in the statics query is correctly updated for those two entities.

Currently, I have implemented a solution calling those columns "tax_1", "tax_2", and "tax_3".

Let me know your thoughts.

@Civolilah Civolilah marked this pull request as ready for review October 15, 2024 06:12
@turbo124
Copy link
Member

turbo124 commented Oct 16, 2024

@Civolilah could you use translations tax1,tax2,tax3 for the labels please?
We can then use the same keys for the logic as well.

@turbo124
Copy link
Member

image

when selecting a tax, i do not get the second option for the tax to be selected.

@Civolilah
Copy link
Collaborator Author

image

when selecting a tax, i do not get the second option for the tax to be selected.

@turbo124 Okay, so the solution has been adjusted using the old data. Now, as I can see in the bulk_updates, it returns tax1, tax2, and tax3. According to that change, the solution has been adjusted and it should display selectors properly. Screenshot:

Screenshot 2024-10-20 at 16 25 52

Let me know your thoughts.

@turbo124
Copy link
Member

instead of newValue, can you please pass snake case new_value

@Civolilah
Copy link
Collaborator Author

instead of newValue, can you please pass snake case new_value

@turbo124 Ah okay, so this was my mistake. Right now it's updated to be "new_value" instead of "newValue", as it was for the clients endpoint already. It looks like it's working great on my end. Let me know your thoughts.

export function useBulk() {
const queryClient = useQueryClient();
const invalidateQueryValue = useAtomValue(invalidationQueryAtom);

return (
return async (
Copy link
Member

Choose a reason for hiding this comment

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

Am I missing something or is there a reason these fns are marked as async?
No awaits inside?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@beganovich Actually, I made this function async because TypeScript notified me that it could be made async (since we return a Promise from the 'request' function). However, I've just removed the async keyword and it works as expected. Screenshot:

Screenshot 2024-10-22 at 18 11 09

Copy link
Member

@beganovich beganovich left a comment

Choose a reason for hiding this comment

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

Looks good, tiny info needed.

@Civolilah
Copy link
Collaborator Author

@beganovich @turbo124 Dave, could this PR please go through one more QA iteration? When we merged the PR related to tax selector value uniqueness (name + rate), it wasn't updated here, which caused an issue. I've fixed it now, and it should work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants