[EC-197] Create Organizations Module

eliykat commented 2 years ago

Type of change

[ ] Bug fix
[ ] New feature development
[x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
[ ] Build/deploy pipeline (DevOps)
[ ] Other

Objective

Start to tidy up LooseComponentsModule by creating a separate OrganizationsModule. Plus related refactors.

We should probably hold off merging this during feature (maybe even regression) testing, so that we don't diverge too much from bugs we're fixing in rc. But any feedback on that question is welcome.

we-should-take-d9ccba85ff

Code changes

Main changes for OrganizationsModule:

src/app/modules/loose-components.module.ts - remove all organization components
src/app/organizations/organizations.module.ts
- move all org components to here
- remove Org prefixes in import statements
- remove organization prefixes in some component names
src/app/oss-routing.module.ts - update style to match other lazy loaded modules
src/app/oss.module.ts - remove SharedModule import, it's not used in this module or re-exported, so it doesn't need to be here.
src/app/modules/shared.module.ts - move a few components here that are shared between the OrganizationsModule and other modules. This is likely temporary and can be cleaned up as we continue to refactor the structure of this app.
src/app/modules/vault/modules/organization-vault/organization-vault.module.ts - move src/app/organizations/vault/* to this folder and to this module, because this is where these components are used.

Changes related to ProvidersModule:

bitwarden_license/src/app/providers/providers.module.ts - this module was importing the whole of OssModule, which is unnecessary. Instead, just import SharedModule and the newly created LayoutsModule for the components it actually needs.
src/app/layouts/layouts.module.ts - create this so it can be shared between OSS and bitwarden_license
src/app/modules/loose-components.module.ts - remove all layout components and instead import the new LayoutModule.

I suspect we're still exporting & importing too many things around the place, but I'll leave it here for now.

Screenshots

Before you submit

[x] I have checked for linting errors (npm run lint) (required)
[ ] This change requires a documentation update (notify the documentation team)
[ ] This change has particular deployment requirements (notify the DevOps team)

Hinton commented 2 years ago

We should look into changing vault from depending on organizations to having organizations depend on vault at a later stage. This could allow us to lazy load the organizations module if we find the need. (As vault would always need to be loaded)

addisonbeck commented 2 years ago

I'm not of the opinion that there is a place for an OrganizationModule (except maybe just for routing). Modules typically align with features, and an Organization is more of a data model that constrains a feature. I'm concerned that going down this path will lead to another slight mess like the current component structure where two pages can look almost identical and do the same things but not be near each other in the application structure or easily share properly scoped dependancies.

Not imposing here, but I would like to weigh the pros and cons of creating top level modules for everything currently being declared by the OrganizationsModule. I think they universally make sense as Domain Modules or Routed Modules, and if we are going to move them out of LooseComponentsModule it should be to dedicated top level modules. This allows for more... modularity 🤡

eliykat commented 2 years ago

@addisonbeck That's good feedback, so you're thinking of something like this?

OssRoutingModule
├─ OrganizationRoutingModule          <- routing only
   ├─ ManageModule           <- domain/routed modules per the current folder structure
   ├─ PoliciesModule
   ├─ ServicesModule
   ├─ SettingsModule
   ├─ ToolsModule
   ├─ ...etc

Do you see these being moved under src/app/modules according to functionality, like you did for the VaultModule? I can see that you had a restructure in mind there, but I'm not sure how it fits in with what we do currently.

@Hinton I don't follow you, the changes in this PR do have the OrganizationsModule lazy loaded (it's not imported anywhere else than in OssRoutingModule). Except for the OrganizationVaultModule, but that doesn't reference OrganizationsModule.

addisonbeck commented 2 years ago

so you're thinking of something like this?

More or less. I have a few other ideas that I think might help us isolate functionality better. I'm making a branch off of this and will try and get something concrete and functional written to share for discussion.

Update 05/26/22: there is a sentiment developing that we need to fix this sooner than later, and I haven't been able to give it enough time. It's okay with me if we move forward with this as is or with the tree structure mentioned above.

eliykat commented 2 years ago

Update: after some further discussions, I'm going to adopt the structure in my comment above and I'll put this back up for review.

eliykat commented 2 years ago

Closing as I will reopen in the clients repo.

bitwarden / web