New Order Cycles edit page with steps

[luisramos0]

What? Why?

Part of #4379 this is not a POC, it works and is ready for production :-)

Quite a few lines of code moved around so the review is better done commit by commit.
I am quite happy with the size of this PR, not too big for such a big change.

This PR introduced a 3 steps process to edit Order Cycles by basically moving incoming and outgoing settings into separate pages.

What should we test?

This is how the pages now look like:

Create - General Settings

Edit - General Settings

Edit - Incoming Settings

same but with no changes (see how the action buttons look like):

Edit - Outgoing Settings

We need to verify both creating and editing OC with hub but also the creating and editing of single enterprise OCs.

Release notes

Changelog Category: Changed
Create and edit Order cycles pages are now split in 3 steps: general settings, incoming settings and outgoing settings.

[luisramos0]

The build is green as the only broken spec is a spec that is removed in #4408

[sauloperez]

Nice! there's some serious work you did here 😍 👏 !

It looks pretty good to me as a first step that we can already validate 💪

From an architectural standpoint, I'd just add that by the changes we introduced in app/controllers/admin/order_cycles_controller.rb it becomes clear to me that we should split that controller into nested resources: incoming, outgoing which both take the order_cycle_id as a parameter. This would remove some of the many responsibilities that the controller already has and it'll easily lead to a more decoupled design as I'm sure more abstractions will pop up in the future.

I take the chance to link again to a presentation that touches this point pretty well: https://speakerdeck.com/derekprior/in-relentless-pursuit-of-rest. In my experience, custom controller actions tend to make controllers hard to change.

Having said that, I would just ship this and iterate. I wouldn't refactor until we are sure we keep this approach and are happy with it.

[luisramos0]

thanks for your review Pau!
tbh there are quite a few things I'd like to refactor here but I dont want to grow the PR...
In a follow up PR 👍

[RachL]

@luisramos0 here are a few comments :

• The fact that the user must click on create to reach incoming products is a bit weird. I litteraly searched 3 mins if I wasn't looking at a bug not showing the next button. I'm not sure we talked about it. Is this something needed in terms of performance?
  On the other hand, this is something we can "fix" with translation. @lin-d-hop what do you think?
• Any chance we can show to the user that the field "ready for" is mandatory in outgoing products? :-)
• Yay to the alphabetical order 🎉

[luisramos0]

@RachL
"The fact that the user must click on create"
shall I rename "Create" to "Next"?

[luisramos0]

I'd refer to build those features in separate PRs. This PR is already big. Is that ok for you @RachL?
small features needed:

• "ready for" mandatory
• alpha ordering of products
• clickable step panels

[Matt-Yorkley]

There's a few build errors to iron out, but it's looking good. I'm wondering if we want to get some of those little UX things like clickable step panels in place before merging to master?

[RachL]

Yes I was wondering the same as @Matt-Yorkley. So far we are not good at our feedback loop. It often means remaining issues are left as good first issues for the community.

I would be open that those feedback are dealt with in another PR and that the PR is merge separately, but I would keep it as one last thing to do before closing the OC performance epic.

What do you reckon @lin-d-hop ?

[luisramos0]

yes @RachL I am not discussing scope, I am just trying to keep the PR small.

[sauloperez]

Might already late but I'd prefer to be able to merge this without enabling it in production (dead simple feature toggle) so that we can work on those things in separate PRs but without keeping this one open for a long time. We already know what happens with long-lasting branches: merge conflicts, rebases needed, etc. 👉 💸

[luisramos0]

Pau, a feature toggle with all the JS code changes would be a nightmare, right?

[sauloperez]

I don't really know because I haven't played with them much but possibilities seem endless. Could be a deployment flag by means of an ENV var that chooses which JS to precompile, for instance. I think it's worth considering, at least. Check out this: https://www.martinfowler.com/articles/feature-toggles.html El dc., 6 de nov. 2019, 16:55, Luis Ramos <[email protected]> va escriure:

…

Pau, a feature toggle with all the JS code changes would be a nightmare, right? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4422?email_source=notifications&email_token=AAF2B2H32FV5RZPXL3WW7KDQSLSFHA5CNFSM4JH67D5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDHAS4Q#issuecomment-550373746>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAF2B2ACOVNH3FSSXWCDZQ3QSLSFHANCNFSM4JH67D5A> .

[RachL]

@sauloperez can we keep the discussion in one place? We discussed on slack where a lot of people contributed. I agree slack is not the best place, but duplicating is not great either...

[luisramos0]

just to be clear, toggling this feature right now means more than a day of dev time and increased technical complications.
I dont see enough reasons to make that effort.

I'd say this is pending one more dev code review to be moved to test ready as a normal PR.
On the test phase we can perform extra validation steps.

[luisramos0]

rebased to resolved conflicts.

[daniellemoorhead]

@luisramos0 given you built this does it make sense for someone else to test it? I'm assuming a tester can do it and it doesn't have to be a dev?

Myself and @JenRSmith could probably do this on Friday if @RachL and @filipefurtad0 don't get to it first....

[luisramos0]

no, this is not dev-test.
I'd say we need an experienced tester to validate this big change.

[RachL]

Tested with admin account mostly (not super admin), but I went through single OC and outgoing / incoming OC.

I tested shops with inventory and shops without inventory. With multiple distributors or producers or single producer and shops.

I tested creating, duplicating and deleting.

I did NOT test tags or do an advance testing on fees. Would that be required here?

I didn't saw anything broken 👍 🎉

However clickable steps would be really good to release at the same time.

Also FYI I have a 2-3 hubs that are ready to give me close feedbacks on this. All of the hubs that I have asked so far are duplicating their OC, so they don't feel this is going to be a big change.
However clickable steps will definitely help them.

[luisramos0]

nice, thanks @RachL
yes, I think we need to do some basic tests on tags and fees. mostly verifying they stick when you add them and go away when you removee them. the application of the tags and fees on other places of the app will be working if the fees show up in the OC edit page.

[RachL]

Ok so adding / removing fees works well :)

Tags are working well BUT I can't find how to remove a tag from an OC. However this looks like a known issue and not something introduce by this PR. I'm moving this to ready to go :-)