Convert enterprises/edit page to stimulus #9510

binarygit · 2022-08-03T11:27:21Z

What? Why?

Replaces the angular code that handled rendering of admin/enterprises/<enterprise_name>/edit
More context here: https://openfoodnetwork.slack.com/archives/C03QBDHDCSY/p1658325112405699 and #9478

Closes #

What should we test?

Visit page admin/enterprises/<enterprise_name>/edit
Ascertain only the selected side-bar tab's form is displayed
Ascertain that the displayed form changes accordingly when you select another tab

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

jibees · 2022-08-05T09:05:31Z

Some specs are failing. Back into In Dev

binarygit · 2022-08-08T12:21:48Z

Hello @jibees I know that here it's saying that two tests in system/admin/enterprises_spec.rb are failing but they are passing locally. I don't know what to do about this 🤷

jibees · 2022-08-08T12:33:22Z

@binarygit

The spec fails:

     Failure/Error: is_shop = enterprise.sells != "none"
     
     ActionView::Template::Error:
       undefined method `sells' for nil:NilClass


# ./app/helpers/admin/enterprises_helper.rb:18:in `side_menu_items`

undefined method 'sells' for nil:NilClass means that enterprise is nil then.

It use throw when user click on New Enterprise Group ie. /admin/enterprise_groups/new and @enterprise is nil here (it's logical, because I guess there is no enterprise here, right?)

Does this help?

(FYI, the spec fail locally, and when I go to /admin/enterprise_groups/new I have the error)

jibees · 2022-08-08T12:34:00Z

Ok, now I see, you don't run the right spec ; the one failing is spec/system/admin/enterprise_groups_spec.rb

binarygit · 2022-08-08T13:11:43Z

arghhh sorry, Should've looked at the error more carefully!
I got confused because I had previous failures in enterprises_spec and when I fixed them and pushed I thought these were also in the same file.
Thanks for the help 🙇

binarygit · 2022-08-09T10:28:19Z

@jibees Finally got the tests to pass. Could you have a look when you have the time, and let me know if any changes are required? Thanks!

jibees · 2022-08-09T11:57:41Z

app/webpacker/controllers/side_menu_controller.js

+  }
+
+  get currentActiveTab() {
+    return this.element.querySelector('.selected')


As .selected is a very common class (selector), I'd vote to specify elements with target (let's say tab target)

hmm yes, if we do that we can use this.tabTargets.querySelector('.${this.className}) to get the active tab which is much more specific and intentional.

This gave me the idea to use

const currentActiveForm = this.formTargets.find( (form) => form.id == `${currentActiveTab.id}_form` ); const newActiveForm = this.formTargets.find( (form) => form.id == `${newActiveTab.id}_form` );

in enteprise_form_controller 😄

And I've made the other changes you requested. Thanks jean!

jibees · 2022-08-09T12:00:06Z

app/webpacker/controllers/side_menu_controller.js

+        newActiveTab: event.currentTarget
+      }})
+
+    this.currentActiveTab.classList.remove('selected')


Maybe we can add selected as a value, to be as generic as possible?

jibees · 2022-08-09T12:01:25Z

app/webpacker/controllers/enterprise_form_controller.js

+  }
+
+  show({detail: { currentActiveTab, newActiveTab }}) {
+    let currentActiveForm = this.element.querySelector(`#${currentActiveTab.id}_form`)


I think it's better to use const as often as possible.

jibees

Nice one! I still need to look at those cableresponses things, and understand what's happening ;)

I have few comments, that should be addressed/discussed.

During this time, could you please rebase onto master? I've added a precommit hook, and prettier in the stack, in order to have all the js formatted the same way.

binarygit · 2022-08-09T14:21:12Z

After fetching upstream and running yarn install, the yarn.lock file changes. I don't think this should be happening right?
I rebased and pushed btw

Also getting this when I try to commit after changing any controller files, does this mean I need to run prettier manually? I thought it ran automatically when I tried to commit?

 $ git ci --amend
yarn run v1.22.17
warning ../../package.json: No license field
$ pretty-quick --check --staged
🔍  Finding changed files since git revision cb6faed1d.
🎯  Found 1 changed file.
⛔️  Check failed: app/webpacker/controllers/side_menu_controller.js
✗ Code style issues found in the above file(s). Forgot to run Prettier?
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
husky - pre-commit hook exited with code 1 (error)

It lets me commit after I run prettier manually, but thought I would ask if this is intentional 😄

jibees · 2022-08-09T15:08:36Z

Please, revert your modifications on yarn.lock I don't think they are useful in the context of this PR. I'll make a PR if any errors occurs on yarn.lock. Created: Upgrade yarn.lock since it seemed to be derived #9544
No the precommit hook only check (via pretty-quick --check --staged) if any errors occurs. You should run prettier on your modified files via yarn prettier command)

jibees

Many thanks, I think it's getting better and better!

Just small comments + one general:

I think that commit 2bdb612 and 09b2130 should be squashed together since they are related to the same feature (ie. server + client side)

jibees · 2022-08-10T05:42:33Z

spec/javascripts/stimulus/enterprise_form_controller_test.js

+ */
+
+import { Application } from "stimulus";
+import side_menu_controller from "../../../app/webpacker/controllers/side_menu_controller";


I'm wondering if there is any way to test enterprise_form_controller controller without side_menu_controller ?

If yes, I think it should be done. I don't think we need to click on element, but only send event.

If no, I think enterprise_form_controller should be included to side_menu_controller.

What do you think?

I don't think it's possible to test enterprise_form in isolation because the payload for show() { detail: { currentActiveTab, newActiveTab } } is unique.
I did think of making these into a single controller, but I felt it was easier to understand what was happening when there were two of them working together. Especially because the elements that they are concerned with (side menu handles tabs and enterprise form handles form) are different.
Also, I couldn't think of an appropriate name for such a controller.
Should I try to make them into a single controller? I think this takes a hit on the readability of the code though.

I re-read your code, trying to understand better how elements are interacting.
I'm now more confident that (IMHO) those two controllers should be merged into one, for example panel_controller which handle at the same time the side menu on the left (which could be on top adctually, so I'd rename it into menu?) and the content that are displayed or not, strongly depending on what is selected/clicked on the menu.
To my opinion, it'll be easier to understand (no event between controllers will be sent, I think it's easier to understand), and will be fully generic (ie. not depending on any enterprise-something).

What do you think?

oh I see what you mean. I think you're also thinking about the menu on the top Dashboard Products ... and that makes sense (maybe we could use this controller to render those thing when we get there). Ok I'll convert it into a single menu_controller. Thanks!

Yes, It can be used somewhere else (one day?!) and it should be generic.
I though about panel because menu makes me think that we don't talk about content (but only about clickable links).
Maybe it's better to call it tabs_and_panels_controller? I don't know, I'm very bad about naming something...

Anyway, thanks a lot for your comprehension, and your patience!

Could you take a look jibbes? Sorry, it took me sometime to get this done. I always mess something up when I need to go back and do a rebase, so it takes a couple of tries to get it right! 😫

jibees · 2022-08-10T05:44:58Z

app/webpacker/controllers/primary_details_controller.js

@@ -0,0 +1,25 @@
+import { Controller } from "stimulus"


This file is not formatted as it should be. I'm wondering then how you was able to commit ...

Could you please run yarn prettier --check . and copy/paste the output?

Thanks! 🙏

I formatted it correctly and pushed jn. Also, I squashed those commits!

mkllnk

Nice work. I have to admit that I can't judge the architecture because I have no experience with Stimulus but the code is readable. 😄

app/controllers/admin/enterprises_controller.rb

app/views/admin/enterprises/_form.html.haml

filipefurtad0 · 2022-08-17T17:11:39Z

Hey @binarygit,

Thanks for this huge piece of work and your detailed notes on what to test. Going through them:

On the admin/enterprises/<enterprise_name>/edit page:

Ascertain only the selected side-bar tab's form is displayed -> OK
Ascertain that the displayed form changes accordingly when you select another tab -> OK

Uploading Screencast from 17-08-2022 18:04:06.webm…

Had a look as well on other browsers and mobile:

Latest Chrome (Ubuntu 22.04)
Android and iPhone (Browserstack)

Good to go 🎉

binarygit self-assigned this Aug 3, 2022

binarygit force-pushed the convert-enterprises/edit-page-to-stimulus branch from f7a6b26 to 0564608 Compare August 4, 2022 01:20

binarygit force-pushed the convert-enterprises/edit-page-to-stimulus branch 4 times, most recently from 3dcca85 to 2b61b82 Compare August 8, 2022 10:52

binarygit force-pushed the convert-enterprises/edit-page-to-stimulus branch 2 times, most recently from 2f47c6c to 41430eb Compare August 9, 2022 08:34

jibees reviewed Aug 9, 2022

View reviewed changes

binarygit force-pushed the convert-enterprises/edit-page-to-stimulus branch from cc39b56 to 7a82167 Compare August 9, 2022 14:24

binarygit force-pushed the convert-enterprises/edit-page-to-stimulus branch 5 times, most recently from 9d1e5f6 to b5b80a1 Compare August 9, 2022 16:31

jibees reviewed Aug 10, 2022

View reviewed changes

binarygit force-pushed the convert-enterprises/edit-page-to-stimulus branch from b5b80a1 to dfa6a01 Compare August 10, 2022 06:19

binarygit added 2 commits August 10, 2022 15:08

Render admin/enterprises/<name>/edit page using stimulus

e18a242

Display/hide side-menu items when values in primary details change

6b56a8d

binarygit added 2 commits August 10, 2022 15:14

Render enterprise groups #edit and #new page using stimulus

27552c9

Delete angular code for side_menu

79ba223

binarygit force-pushed the convert-enterprises/edit-page-to-stimulus branch from dfa6a01 to 79ba223 Compare August 10, 2022 09:29

mkllnk approved these changes Aug 17, 2022

View reviewed changes

app/controllers/admin/enterprises_controller.rb Show resolved Hide resolved

app/views/admin/enterprises/_form.html.haml Show resolved Hide resolved

filipefurtad0 merged commit b992daa into openfoodfoundation:master Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert enterprises/edit page to stimulus #9510

Convert enterprises/edit page to stimulus #9510

binarygit commented Aug 3, 2022

jibees commented Aug 5, 2022

binarygit commented Aug 8, 2022

jibees commented Aug 8, 2022

jibees commented Aug 8, 2022

binarygit commented Aug 8, 2022

binarygit commented Aug 9, 2022 •

edited

Loading

jibees Aug 9, 2022

binarygit Aug 9, 2022 •

edited

Loading

binarygit Aug 9, 2022

jibees Aug 9, 2022

jibees Aug 9, 2022

jibees left a comment

binarygit commented Aug 9, 2022 •

edited

Loading

jibees commented Aug 9, 2022 •

edited

Loading

jibees left a comment

jibees Aug 10, 2022 •

edited

Loading

binarygit Aug 10, 2022

jibees Aug 10, 2022

binarygit Aug 10, 2022

jibees Aug 10, 2022

binarygit Aug 10, 2022

jibees Aug 10, 2022

binarygit Aug 10, 2022

mkllnk left a comment

filipefurtad0 commented Aug 17, 2022

Convert enterprises/edit page to stimulus #9510

Convert enterprises/edit page to stimulus #9510

Conversation

binarygit commented Aug 3, 2022

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

jibees commented Aug 5, 2022

binarygit commented Aug 8, 2022

jibees commented Aug 8, 2022

jibees commented Aug 8, 2022

binarygit commented Aug 8, 2022

binarygit commented Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

binarygit Aug 9, 2022 • edited Loading

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

jibees left a comment

Choose a reason for hiding this comment

binarygit commented Aug 9, 2022 • edited Loading

jibees commented Aug 9, 2022 • edited Loading

jibees left a comment

Choose a reason for hiding this comment

jibees Aug 10, 2022 • edited Loading

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

mkllnk left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Aug 17, 2022

binarygit commented Aug 9, 2022 •

edited

Loading

binarygit Aug 9, 2022 •

edited

Loading

binarygit commented Aug 9, 2022 •

edited

Loading

jibees commented Aug 9, 2022 •

edited

Loading

jibees Aug 10, 2022 •

edited

Loading