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] Adjustment Reasons edit/update #5815

Merged

Conversation

MadelineCollier
Copy link
Contributor

Summary

This PR is for this issue but it doesn't have an issue ID yet as the issue is still in draft and there is a GitHub bug preventing opening the issue.

This PR migrates the editing/updating of existing adjustment reasons to the new admin interface, following the existing pattern used for tax categories.

The form is rendered via a modal dialog on the adjustment reasons list by leveraging Turbo frames. Successful updating leads to a turbo stream page refresh, which updates the existing list preserving the query params and the scroll position, for a consistent UX.

The attached video shows the functionality visually:

Screen.Recording.2024-08-08.at.5.32.56.PM.mov

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

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.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

The new adjustment reasons were always being created with `active: true`
regardless of the checkbox state. This commit fixes that issue so that
the checkbox state is respected and allows users to control the active
state.
The human-readable output is the same regardless of which model we are
referencing here, (always "Active") but since this is the
AdjustmentReason component, let's be consistent and reference that here.
@MadelineCollier MadelineCollier force-pushed the admin-update-adjustment-reason branch from cbb6253 to 09a30db Compare August 8, 2024 15:43
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 78.57143% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.78%. Comparing base (3e8be72) to head (364aec2).

Files Patch % Lines
...ers/solidus_admin/adjustment_reasons_controller.rb 70.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5815      +/-   ##
==========================================
- Coverage   88.80%   88.78%   -0.02%     
==========================================
  Files         732      733       +1     
  Lines       17088    17114      +26     
==========================================
+ Hits        15175    15195      +20     
- Misses       1913     1919       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This should consistently use either uppercase or lowercase. Since the
others use lowercase, I went with that.
@MadelineCollier MadelineCollier force-pushed the admin-update-adjustment-reason branch from 09a30db to 364aec2 Compare August 8, 2024 16:39
@MadelineCollier
Copy link
Contributor Author

MadelineCollier commented Aug 8, 2024

Looked at trying to up my coverage % for this PR, but couldn't find a worthwhile way of testing failed updates without introducing controller specs in addition to our feature specs. Since I didn't see any controller specs for our existing controllers, I hope the feature specs I provided on this PR should be good enough, but let me know if you'd like me to have a longer look at this or if you have any suggestions.

Screenshot 2024-08-08 at 6 43 09 PM

In the meantime, I'll leave this PR as is for someone else to make the call on merging it.

@MadelineCollier MadelineCollier merged commit 956ce47 into solidusio:main Aug 12, 2024
12 of 14 checks passed
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.

2 participants