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][Users]Add new admin store_credits show page #5978

Merged

Conversation

MadelineCollier
Copy link
Contributor

@MadelineCollier MadelineCollier commented Dec 2, 2024

Summary

This PR is for issue: #5824

The store credits flow is in the process of being significantly refactored so this will eventually be a page which both displays the store credit info and allows editing (when appropriate). It will match the flow of the new user addresses page. For now this just replaces the legacy show page and migrates the users/:user_id/store_credits/:id page from the legacy soldius_backend to the new solidus_admin. Again there were no designs so I just tried my best to match to the existing components and layouts.

Screenshots

New page
Screenshot 2024-12-02 at 5 40 30 PM

Old page
Screenshot 2024-12-02 at 4 37 54 PM

Bonus

New badge scheme
Screenshot 2024-12-02 at 4 46 49 PM

Old badge scheme
Screenshot 2024-12-02 at 4 47 05 PM

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.

@MadelineCollier MadelineCollier requested a review from a team as a code owner December 2, 2024 16:41
@MadelineCollier MadelineCollier force-pushed the admin-user-store-credit-page branch 3 times, most recently from 84f5e6f to 7269173 Compare December 3, 2024 18:04
@MadelineCollier
Copy link
Contributor Author

This PR is waiting for #5998 which should fix the two failing flaky specs. Thanks @tvdeyen for your work on making the other specs less flaky!

The admin store credit flow is complicated enough and has enough of its
own actions that it should really be in its own controller rather than
squished into the users controller.
If a store credit has been invalidated it will show a red badge, if it
has not, it will show a green badge. This is more in line with
expectations than the previous colour scheme.
The store credits flow is in the process of being significantly
refactored so this will eventually be a page which both displays the
store credit info and allows editing (when appropriate). It will match
the flow of the new user addresses page.
@MadelineCollier MadelineCollier force-pushed the admin-user-store-credit-page branch from 7269173 to 97919a8 Compare December 4, 2024 13:31
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 95.55556% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.47%. Comparing base (81c605c) to head (97919a8).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...olidus_admin/users/store_credits/show/component.rb 92.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5978      +/-   ##
==========================================
+ Coverage   87.81%   89.47%   +1.65%     
==========================================
  Files         476      784     +308     
  Lines       11656    18038    +6382     
==========================================
+ Hits        10236    16139    +5903     
- Misses       1420     1899     +479     

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

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.

Thanks. Can we please continue the work on semantics we started in #5950 (6bd7f5e) and use links instead of divs for the store credits components. I would like us to establish as much semantics as possible going forward with the new admin. Reducing the amount of work we have to do in the future.

@MadelineCollier
Copy link
Contributor Author

MadelineCollier commented Dec 5, 2024

Thanks. Can we please continue the work on semantics we started in #5950 (6bd7f5e) and use links instead of divs for the store credits components. I would like us to establish as much semantics as possible going forward with the new admin. Reducing the amount of work we have to do in the future.

The links don't work with the specs as they blow away the query params, something which the specs expect to be present after clicking through. Since we still haven't solved that (without javascript) I think we'll need to keep it as is until we have a solution to preserve those params.

@MadelineCollier MadelineCollier requested review from tvdeyen and a team December 5, 2024 11:40
@MadelineCollier
Copy link
Contributor Author

@tvdeyen more context here.

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.

I tabled #6014 (comment)

so this is ready to be merged. Thanks for the patience 🙏🏻

@MadelineCollier
Copy link
Contributor Author

I tabled #6014 (comment)

so this is ready to be merged. Thanks for the patience 🙏🏻

No worries, thank you so much for investing the time to look into it!

@MadelineCollier MadelineCollier merged commit b773f5e into solidusio:main Dec 6, 2024
18 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