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

[Admin] Add Update Tax Category feature #5697

Merged
merged 4 commits into from Apr 12, 2024

Conversation

spaghetticode
Copy link
Member

Summary

Recently, it was introduced in the new admin the ability to create a new tax category via a modal dialog. It's now time to allow editing an existing one.

The new feature works similarly to the one for creating a new tax category, i.e. it opens a modal dialog with the edit form. When submitting the form, if the tax category has errors then they're shown to the user, otherwise the record is updated, the modal is removed, and a success message is shown.

Screen.Recording.2024-03-13.at.14.54.05.mov

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.

  • I have kept my commits small and atomic.

  • I have used clear, explanatory commit messages.

  • ✅ I have added automated tests to cover my changes.

  • 📸 I have attached screenshots to demo visual changes.
    The following are not always needed:

  • 📖 I have updated the README to account for my changes.

  • 📑 I have documented new code with YARD.

  • 🛣️ I have opened a PR to update the guides.

This new Turbo frame will be filled with the modal for editing the
tax category.
The component renders the edit form inside a modal on top of the
tax categories list, inside a Turbo frame.
This completes the feature for editing an existing tax category.
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

great work!

@loicginoux
Copy link
Contributor

Hello, subscribing to this PR, when this is merged, I'll start doing the same logic for shipping category as I started implementing the creation here #5718

@tvdeyen tvdeyen merged commit cb0ff1e into solidusio:main Apr 12, 2024
12 checks passed
@@ -14,6 +16,16 @@ def new
end
end

def edit
@tax_category = Spree::TaxCategory.find(params[:id])
Copy link
Contributor

Choose a reason for hiding this comment

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

PR has already been merged but as I am reading your code, I discover this line is not needed as you refactored it in find_tax_category. Will you do it or shall I remove it in my PR about adding edition for shipping categories (not related but quick fix) ?

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.

None yet

3 participants