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

fix map staying blank if no permissions on default theme #197

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

olivierdalang
Copy link
Contributor

This works around the issue of map staying blank if the logged user has no permission on the default map. Such a situation easily happens with the stock qwc-map-viewer and qwc-config-generator, which assumes the first project is the default project, not taking into account user permissions.

See qwc-services/qwc-map-viewer#12 for more context.

@@ -94,6 +94,20 @@ class AppInitComponent extends React.Component {
if (ConfigUtils.getConfigProp("dontLoadDefaultTheme")) {
return;
}

// Determine default theme, and fallback if not available
let availableThemes = themes.items.map((t) => t.id);
Copy link
Member

Choose a reason for hiding this comment

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

Theme items might potentially be nested.

@@ -94,6 +94,20 @@ class AppInitComponent extends React.Component {
if (ConfigUtils.getConfigProp("dontLoadDefaultTheme")) {
return;
}

// Determine default theme, and fallback if not available
let availableThemes = themes.items.map((t) => t.id);
Copy link
Member

Choose a reason for hiding this comment

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

Theme items might potentially be nested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will try to come up with a commit that handles nested themes. Would you have a sample themes.json test with, or some info in how to create projects that have nested themes ? Is it with QGIS themes in the project ?

(In my themes.json, I see sublayers, but nothing looking like nested themes, not even an empty list)

@manisandro
Copy link
Member

I wonder whether a better approach would be to be able to set the default theme per user. This way it might be hard to control which theme is shown in the end.

1 similar comment
@manisandro
Copy link
Member

I wonder whether a better approach would be to be able to set the default theme per user. This way it might be hard to control which theme is shown in the end.

@olivierdalang
Copy link
Contributor Author

Currently (if I get things right), defaultTheme is populated by the config generator by just using the first theme that is found. So the proposed PR more or less matches that logic, but per user.

I agree it would be better to be able to define it per user, but where would that happen exactly ? I'm not yet too familiar with the QWC2 stack, but guess it needs to be handled by the auth backend somehow, and probably not really is attached to the user, but rather to roles. And then what happens if a user has many roles ?

IMO that's quite a big change for such a small fix, and may become quite difficult to correctly setup. By using the first one, it's deterministic so one can still use alphabetic ordering to determine which one gets loaded.

By the way, as said in qwc-services/qwc-map-viewer#12, qwc-map-viewer could fix that on the fly instead of fixing it on the JS side.

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.

2 participants