andrewdwallo / erpsaas

A Laravel and Filament-powered accounting platform, crafting a modern and automated solution for financial management.
MIT License
856 stars 241 forks source link

Live Currency Improvement + greek 3x #78

Closed konstantinosbotonakis closed 2 weeks ago

konstantinosbotonakis commented 3 weeks ago

image

andrewdwallo commented 2 weeks ago

@konstantinosbotonakis I think it would be better to just hide the navigation menu item for Live Currency if Forex isn't enabled. Make sure the abort in mount goes back as well.

shouldRegisterNavigation check is not working I think because I am using the navigation builder. You are going to have to apply this in the FilamentCompaniesServiceProvider:

NavigationItem::make(LiveCurrency::getNavigationLabel())
                                    ->group(LiveCurrency::getNavigationGroup())
                                    ->parentItem(LiveCurrency::getNavigationParentItem())
                                    ->icon(LiveCurrency::getNavigationIcon())
                                    ->activeIcon(LiveCurrency::getActiveNavigationIcon())
                                    ->isActiveWhen(fn (): bool => request()->routeIs(LiveCurrency::getNavigationItemActiveRoutePattern()))
                                    ->sort(LiveCurrency::getNavigationSort())
                                    ->url(LiveCurrency::getNavigationUrl())
                                    ->visible(LiveCurrency::shouldRegisterNavigation()),
konstantinosbotonakis commented 2 weeks ago

@andrewdwallo good suggestion but in case we hide options from main menu, we should be able to enable them via admin. AFAIK Forex cannot be enabled via admin settings, only as ENV var before spin up, right?

konstantinosbotonakis commented 2 weeks ago

Also I can see you approved the changes, but I don't have write access, so I can't merge.

andrewdwallo commented 2 weeks ago

@konstantinosbotonakis I didn't approve anything, @farhadrahmanii seemed to approve it although I'm not sure how he can do that. Might need to change some settings for this repo. And yes, that is correct, I am the only one able to merge. I'm sure you can understand why I wouldn't allow people who I don't know to merge whenever they want...

konstantinosbotonakis commented 2 weeks ago

Sure no problem. I didn't expect to get write access @andrewdwallo

andrewdwallo commented 2 weeks ago

I'll take a look at this in a little while, I'm currently busy at the moment.

andrewdwallo commented 2 weeks ago

@andrewdwallo good suggestion but in case we hide options from main menu, we should be able to enable them via admin. AFAIK Forex cannot be enabled via admin settings, only as ENV var before spin up, right?

No we shouldn't be able to enable this via an Admin panel. This should be based on whether its enabled via the config or not, at least for now.

konstantinosbotonakis commented 2 weeks ago

Ok got it. I thought because the project is named and focused on SaaS that settings might be able to be toggled via admin users.

andrewdwallo commented 2 weeks ago

Ok got it. I thought because the project is named and focused on SaaS that settings might be able to be toggled via admin users.

It depends on what the company who owns the SaaS wants to do. For example, almost all Accounting Software I know of provide Live Currency Exchange rates by default - not on a per user basis.