Remove organization_name from base URL

[jp524]

Resolves #4240

Description

Refer to #4324 for original PR that is now organized into logical and smaller commits.

Type of change

• New feature (non-breaking change which adds functionality) -> for users that are not super admins
• Breaking change (fix or feature that would cause existing functionality to not work as expected) -> super admins won’t be able to access routes outside of the /admin namespace (see Discussion section for Remove organization_name from base URL #4324 )

How Has This Been Tested?

Tests files have been updated accordingly.

[dorner]

Overall this looks good - congratulations, I don't think I've ever reviewed a PR this big before! 😂 Had a few nits.

@cielf how do we want to proceed with this? I don't think we need a full test of every possible page, but I think we'll need some sanity click-around testing?

[dorner]

Some of the system tests are failing though.

[cielf]

Overall this looks good - congratulations, I don't think I've ever reviewed a PR this big before! 😂 Had a few nits.

@cielf how do we want to proceed with this? I don't think we need a full test of every possible page, but I think we'll need some sanity click-around testing?

We definitely need some manual testing -- what we really are testing for is whether the pages come up, though, right? We don't have to be looking in detail on every page. It shouldn't take too long. But I'n going to wait until the nits are addressed.

[jp524]

Tests for spec/system/organization_system_spec.rb are still failing at this time. It's odd because I can't reproduce the behaviour when running the app in my local environment - everything works fine.
I'll continue investigating on Monday.

[elasticspoon]

Tests for spec/system/organization_system_spec.rb are still failing at this time. It's odd because I can't reproduce the behaviour when running the app in my local environment - everything works fine. I'll continue investigating on Monday.

If stuff is failing on CI and not locally and vice versa I usually try running rails assets:precompile and rails db:test:prepare. Often times the either the test javascript or the test db have gotten out of sync with what you have locally.

[dorner]

Nits are all addressed! Unfortunately there are now a bunch of conflicts with @elasticspoon 's work. I was kind of worried this would happen, but I'm pretty sure the conflicts should all be pretty straightforward to fix. Maybe merging master will also fix the failing tests?

[jp524]

Some tests are still failing - I'm looking into it.

@elasticspoon actually the tests also fail locally, but when I try to reproduce the steps that the tests use it works in the browser.

[jp524]

Almost there! The last failing test is one introduced in commit f158ba9.

@dorner Since you wrote the original test would you be able to have a look when you have the chance? It's spec/events/inventory_aggregate_spec.rb:692

[elasticspoon]

Almost there! The last failing test is one introduced in commit f158ba9.

@dorner Since you wrote the original test would you be able to have a look when you have the chance? It's spec/events/inventory_aggregate_spec.rb:692

What is happening is when the line item is getting created its also creating a storage location that ends up empty which fails the test. It on this line: https://github.com/jp524/human-essentials/blob/e67744a8d27592005931d51609cb8e1fb851fa4b/spec/events/inventory_aggregate_spec.rb#L702

If you do it like this https://github.com/jp524/human-essentials/blob/e67744a8d27592005931d51609cb8e1fb851fa4b/spec/events/inventory_aggregate_spec.rb#L648

i believe it should work.

[jp524]

Thank you @elasticspoon!

Now ready for final review.

[cielf]

Cool! I'll slot in doing some testing over the next couple of days.

[cielf]

I did some light manual testing - and didn't find anything broken. @dorner?

[dorner]

Looks good to me! 🎉 @cielf any reason to wait on the merge?

[elasticspoon]

@jp524 sorry, I ended up making a similar fix to the one you did for something else and now you have a conflict to resolve. ☹️

[cielf]

I don't know of any reason not to merge once the conflict is resolved.

[jp524]

@jp524 sorry, I ended up making a similar fix to the one you did for something else and now you have a conflict to resolve. ☹️

No worries! I just merged main.
It looks like there's a test failing at spec/system/partner_system_spec.rb:224. I can't reproduce this locally though so maybe it's just flaky.

[dorner]

This is good to go! Thanks so much!

[github-actions]

@jp524: Your PR Remove organization_name from base URL is part of today's Human Essentials production release: 2024.05.26.
Thank you very much for your contribution!