Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OBPIH-4277 Add option to make separate dashboard pages #3374

Merged
merged 2 commits into from Jul 22, 2022
Merged

Conversation

kchelstowski
Copy link
Collaborator

@kchelstowski kchelstowski commented Jul 21, 2022

Best to test locally.

Copy link
Collaborator

@awalkowiak awalkowiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kchelstowski Overall code looks good, I found one issue that needs to be resolved.

// If the size is different, that mean that the config has changed
if (userConfigSize != configSize) {
return config
if (userConfigSize != resultConfigSize) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the userConfig (which is based on the main dashboard) will have the same size as for example supplier dashboard stored in the resultConfig, then the user config will be returned instead of another dashboard by id.

def getDashboardConfig(User user, String id) {
def fullConfig = grailsApplication.config.openboxes.dashboardConfig
def resultConfig = [
dashboards: fullConfig.dashboards[id ?: "mainDashboard"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be now singular (dashboard).

def resultConfig = [
dashboards: fullConfig.dashboards[id ?: "mainDashboard"],
dashboardWidgets: fullConfig.dashboardWidgets
]
def userConfig = user.deserializeDashboardConfig()
Boolean configChanged = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this boolean has no sense right now. Could you remove it?

@awalkowiak awalkowiak merged commit 81e523d into develop Jul 22, 2022
@awalkowiak awalkowiak deleted the OBPIH-4277 branch July 22, 2022 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants