4860-deprecate-and-remove-dashboards-code #5448

lauriejefferson · 2023-10-23T16:14:18Z

Summary

Fixes #4860
This PR adds deprecation warnings to the following views and controllers:

codecov · 2023-10-23T16:24:37Z

Codecov Report

Merging #5448 (18d6c65) into main (c64e9fd) will decrease coverage by 0.04%.
Report is 112 commits behind head on main.
The diff coverage is 14.28%.

❗ Current head 18d6c65 differs from pull request most recent head 5e67650. Consider uploading reports for the commit 5e67650 to get more accurate results

@@            Coverage Diff             @@
##             main    #5448      +/-   ##
==========================================
- Coverage   88.86%   88.82%   -0.04%     
==========================================
  Files         607      607              
  Lines       14750    14757       +7     
==========================================
+ Hits        13107    13108       +1     
- Misses       1643     1649       +6

Files	Coverage Δ
...ore/lib/spree/permission_sets/dashboard_display.rb	`100.00% <100.00%> (ø)`
...p/controllers/spree/admin/dashboards_controller.rb	`0.00% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jarednorman

Thanks for taking a stab at this!

We have some infrastructure for handling deprecations and don't call ActiveSupportDeprecation.warn directly like you have. Instead we use Spree.deprecator. You'll need to switch your code to use that.

Additionally, the DashboardsController is deprecated in a way that simply loading it will trigger the deprecation which is undesirable and the specs themselves don't need to be deprecated. I recommend looking through some past PRs that deprecated things to find some examples of how we've handled similar deprecations in the past to get a feel for how this should be handled.

jarednorman · 2023-10-31T18:06:56Z

Thanks for using the deprecator. This still hasn't addressed my other concerns:

Additionally, the DashboardsController is deprecated in a way that simply loading it will trigger the deprecation which is undesirable and the specs themselves don't need to be deprecated. I recommend looking through some past PRs that deprecated things to find some examples of how we've handled similar deprecations in the past to get a feel for how this should be handled.

lauriejefferson · 2023-11-01T10:32:16Z

I need a little help with the DashboardController deprecation. I searched through past PRs and I see #4102 that may relate. From the PR, It does use a deprecator in the OrdersController, but I'm not sure if that code has also been deprecated in favor of Spree.deprecator.

tvdeyen · 2023-11-01T11:28:36Z

backend/app/controllers/spree/admin/dashboards_controller.rb

@@ -3,6 +3,9 @@
 module Spree
  module Admin
    class DashboardsController < BaseController
+      class << self
+         Spree.deprecator.warn "The Dashboards controller is deprecated. Please use the new admin dashboard."
+      end


I think if we put this in a before_action the code would only run if the controller is used during runtime and not during app boot. Could you try?

before_action do Spree.deprecator.warn "The Dashboards controller is deprecated. Please use the new admin dashboard." end

Also I find the message a bit confusing. To me it is not clear which one is deprecated and which I should use.

The deprecated controller can simply be logged by using

"The #{self.class.name} controller" ...

in the message. Could you please give a more precise hint which the new controller is? Maybe by using a path or a more precise name?

Updated controller.

Is there any response to the changes I made on this pull request?

There isn't a new admin dashboard. This dashboards feature was something that was added to the project, but never actually used in core (and I've never seen it used in store either.) We're not removing this in place of something else, we're just removing it.

Yes, for now there's no replacement. We can say something like:

"The Dashboards controller is deprecated. If you still use dashboards, please copy all controllers and views from solidus_backend to your application."

Also, there's a test failure that I think is related!

@kennyadsl Is the above language the same for the DashboardDisplay module, the Home controller and view?

Maybe we can use a single message that is more generic and we can apply to all. Up to you to choose it, but let me know if I can help.

See latest commit.

kennyadsl

Thanks for the contribution @lauriejefferson! I left a couple of comments, but I think we are close. 🙂

lauriejefferson · 2024-04-01T18:54:46Z

@kennyadsl @jarednorman Can this PR be closed?

jarednorman

Yeah, I think this looks good now.

tvdeyen · 2024-04-05T09:36:23Z

@lauriejefferson would you mind to rebase and force push your branch with latest main to retrigger the pipepline? Thanks 🙏🏻

lauriejefferson · 2024-04-05T14:50:08Z

@tvdeyen Rebase completed.

lauriejefferson · 2024-04-11T13:41:08Z

@jarednorman @tvdeyen @kennyadsl Is there any feedback on this issue?

jarednorman · 2024-04-11T15:50:25Z

Nope, I have approved and am just waiting for another person on core to approve.

tvdeyen · 2024-04-11T16:15:00Z

@lauriejefferson the spec failures are related. Mind taking another look?

tvdeyen · 2024-04-11T16:15:36Z

core/spec/models/spree/permission_sets/dashboard_display_spec.rb

@@ -10,6 +10,7 @@

  context "when activated" do
    before do
+      Spree.Deprecator.warn 'The dashboard_display_spec.rb test is being deprecated.  Please update your source code.'


Ah, a typo causes the spec error

Suggested change

Spree.Deprecator.warn 'The dashboard_display_spec.rb test is being deprecated. Please update your source code.'

Spree.deprecator.warn 'The dashboard_display_spec.rb test is being deprecated. Please update your source code.'

tvdeyen · 2024-04-11T16:15:44Z

core/spec/models/spree/permission_sets/dashboard_display_spec.rb

@@ -18,6 +19,7 @@
  end

  context "when not activated" do
+    Spree.Deprecator.warn 'The dashboard_display_spec.rb test is being deprecated.  Please update your source code.'


Suggested change

Spree.Deprecator.warn 'The dashboard_display_spec.rb test is being deprecated. Please update your source code.'

Spree.deprecator.warn 'The dashboard_display_spec.rb test is being deprecated. Please update your source code.'

lauriejefferson · 2024-04-12T19:57:47Z

@tvdeyen I fixed the rspec tests. Please review.

lauriejefferson requested a review from a team as a code owner October 23, 2023 16:14

github-actions bot added changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem labels Oct 23, 2023

jarednorman requested changes Oct 23, 2023

View reviewed changes

tvdeyen reviewed Nov 1, 2023

View reviewed changes

kennyadsl reviewed Nov 15, 2023

View reviewed changes

jarednorman approved these changes Apr 2, 2024

View reviewed changes

lauriejefferson added 4 commits April 5, 2024 10:37

4860-deprecate-and-remove-dashboards-code

d8e1120

added Spree.deprecator and removed ActiveSupport::Deprecation

2159712

changed deprecation in dashboards controller

9d7315c

added before_action to controllers and rspec files

314703e

lauriejefferson force-pushed the 4860-deprecate-and-remove-dashboards-code branch from 52029c2 to 314703e Compare April 5, 2024 14:39

tvdeyen reviewed Apr 11, 2024

View reviewed changes

fixes typo in dashboard_display_spec.rb

e407686

jarednorman approved these changes Apr 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4860-deprecate-and-remove-dashboards-code #5448

4860-deprecate-and-remove-dashboards-code #5448

lauriejefferson commented Oct 23, 2023

codecov bot commented Oct 23, 2023 •

edited

jarednorman left a comment

jarednorman commented Oct 31, 2023

lauriejefferson commented Nov 1, 2023

tvdeyen Nov 1, 2023

tvdeyen Nov 1, 2023

lauriejefferson Nov 3, 2023

lauriejefferson Nov 14, 2023

jarednorman Nov 15, 2023

kennyadsl Nov 15, 2023

kennyadsl Nov 15, 2023

lauriejefferson Nov 15, 2023

kennyadsl Nov 15, 2023

lauriejefferson Dec 22, 2023

kennyadsl left a comment

lauriejefferson commented Apr 1, 2024

jarednorman left a comment

tvdeyen commented Apr 5, 2024

lauriejefferson commented Apr 5, 2024

lauriejefferson commented Apr 11, 2024

jarednorman commented Apr 11, 2024

tvdeyen commented Apr 11, 2024

tvdeyen Apr 11, 2024

tvdeyen Apr 11, 2024

lauriejefferson commented Apr 12, 2024

	Spree.Deprecator.warn 'The dashboard_display_spec.rb test is being deprecated. Please update your source code.'
	Spree.deprecator.warn 'The dashboard_display_spec.rb test is being deprecated. Please update your source code.'

4860-deprecate-and-remove-dashboards-code #5448

Are you sure you want to change the base?

4860-deprecate-and-remove-dashboards-code #5448

Conversation

lauriejefferson commented Oct 23, 2023

Summary

codecov bot commented Oct 23, 2023 • edited

Codecov Report

jarednorman left a comment

Choose a reason for hiding this comment

jarednorman commented Oct 31, 2023

lauriejefferson commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

lauriejefferson commented Apr 1, 2024

jarednorman left a comment

Choose a reason for hiding this comment

tvdeyen commented Apr 5, 2024

lauriejefferson commented Apr 5, 2024

lauriejefferson commented Apr 11, 2024

jarednorman commented Apr 11, 2024

tvdeyen commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauriejefferson commented Apr 12, 2024

codecov bot commented Oct 23, 2023 •

edited