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

Bring spree_backend base controller to OFN #4512

Merged
merged 8 commits into from
Dec 24, 2019

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Nov 27, 2019

What? Why?

Related to #4050

Here we bring spree/admin/base_controller to OFN and merge its decorator.

One of the reasons we are doing this, we bring some code we need but end up deleting a good part of it or related useless stuff (like the alerts in this case) leaving us with basically the same number of lines of code:
Screenshot 2019-12-04 at 00 00 47

What should we test?

We remove a dead feature in spree (security alerts from spree itself). In general settings there was this checkbox, it's now gone:
Screenshot 2019-12-02 at 21 34 10

This controller is the parent of all controllers in spree.
This is what I think we should validate:

  • navigation - menus and submenus work (we can test a few of them)
  • changing language works in both frontoffice and backoffice
  • authorization - try to access a URL a manager is not allowed with a manager logged in, like going to admin/general_settings/edit with a user who is just a manager
  • api key - make sure that a page like orders list or products list basic functions work (this requires an api key to be in place)
  • flash_message_for - make sure you see the flash message when successfully updating an enterprise
  • banner - make sure that if you have an enterprise with an OC but without payment methods you see a flash message on top of the menu mentioned that problem

Release notes

Changelog Category: Changed
Moved basic admin controller code to OFN to remove dependency from Spree.

@luisramos0 luisramos0 self-assigned this Nov 27, 2019
@luisramos0 luisramos0 changed the title WIP Bring spree_backend base controller to OFN Bring spree_backend base controller to OFN Dec 3, 2019
@luisramos0 luisramos0 removed the pr-wip label Dec 3, 2019
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Yay! Such a good cleanup.

There are two Code Climate complaints. I don't think you need to reduce the lines of code here because that's out of scope. But it would be nice to simply change URI.unescape to CGI.unescape. Otherwise we have to change that when upgrading. URI.unescape has been deprecated in Ruby 2.5.5.

app/controllers/spree/admin/base_controller.rb Outdated Show resolved Hide resolved
app/controllers/spree/admin/base_controller.rb Outdated Show resolved Hide resolved
Copy link
Contributor Author

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

Thanks Maikel, I changed to CGI.unescaped 👍

@luisramos0
Copy link
Contributor Author

I removed a line of code I now realize is not needed. the helper method is never used in our code base.

@RachL RachL self-assigned this Dec 23, 2019
@RachL
Copy link
Contributor

RachL commented Dec 23, 2019

navigation - menus and submenus work (we can test a few of them)

All good.

changing language works in both frontoffice and backoffice

All good.

authorization - try to access a URL a manager is not allowed with a manager logged in, like going to admin/general_settings/edit with a user who is just a manager

All good

image

api key - make sure that a page like orders list or products list basic functions work (this requires an api key to be in place)

All good. Tested CRUD and bulk update.

flash_message_for - make sure you see the flash message when successfully updating an enterprise

All good.

image

banner - make sure that if you have an enterprise with an OC but without payment methods you see a flash message on top of the menu mentioned that problem

@luisramos0 to my knowledge this banner only show up as super admin. I tried to create an OC without payment methods and now the new layout tell me I cannot select my hub if I don't have a payment method. I guess you added a test for this? Which is I think the perfect way to go 👍

Anyway as this is impacting super admin only, I'm moving it to ready to go.

@luisramos0
Copy link
Contributor Author

luisramos0 commented Dec 23, 2019

just to play on the safe side here, lets confirm we are on the same page.
Yes, the hubs cant be added to the OC without payment methods, that is good.
BUT for example if you remove the last payment method from a hub already in an OC, you should see the banner as a manager...
If you dont see this banner in this case, it could be a bug, and it could have been introduced by this PR.

@RachL
Copy link
Contributor

RachL commented Dec 23, 2019

@luisramos0 yes but I can't delete a payment method once it is in the OC... Or you mean a fresh OC with no orders yet?

@luisramos0
Copy link
Contributor Author

I didnt say delete, I said "remove the last payment method from a hub". I should have said "remove the hub from the list of enterprises associated with the payment method" :-) do you see what I mean?

@RachL
Copy link
Contributor

RachL commented Dec 24, 2019

Ok yes sorry I missunderstood. This is working:

image

Moving to ready to go!

@luisramos0
Copy link
Contributor Author

ah, nice, thanks @RachL

@luisramos0 luisramos0 merged commit 9f1eaf0 into openfoodfoundation:master Dec 24, 2019
@luisramos0 luisramos0 deleted the backend_ctrl_base branch December 24, 2019 12:01
@luisramos0 luisramos0 mentioned this pull request Feb 6, 2020
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants