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

#10414: Remove the list of associated groups of logged in user from the User Details modal window #10415

Merged
merged 4 commits into from
Jun 27, 2024

Conversation

mahmoudadel54
Copy link
Contributor

@mahmoudadel54 mahmoudadel54 commented Jun 7, 2024

Description

In this PR, handling hiding the group info in the user details modal by adding a cfg in Login plugin 'hideGroupUserInfo' is implemented. If 'hideGroupUserInfo' is configured into Login plugin cfg,
{ "name": "Login", "cfg": { "toolsCfg": [{"hideGroupUserInfo": true}] } }
the group info into user details modal will not be shown.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

#10414

What is the current behavior?
#10414

What is the new behavior?
If 'hideGroupUserInfo' is configured into Login plugin cfg, the group info into user details modal will not be shown.

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

For Login plugin (cfg.toolsCfg[0].hideGroupUserInfo) ---> the first index into toolsCfg is for userDetails tool based on this order as I think:

  • To enable this, it should be added to all Login plugin
  • For the old contexts, it may not work as expected because of the previous cfg into Login plugin

… in user from the User Details modal window

Description:
- handle hide group info in user details modal by adding a cfg in Login plugin 'hideGroupUserInfo'
- add unit test
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

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

your solutions works only because the user details is the first in the list of tools, but if a project has a custom login plugin this might not work, so I think you have to specify this information as well. Probably it should be mentioned in the migration guidelines because it could be a breaking change in a sense that this configuration may not work there if there are refactors to the login plugin. the index part is import to highlight.

  • please add this entry in the migration guidelnes
  • improve tests

… in user from the User Details modal window

Description:
- add migration guide notes related to the PR changes
- edit in unit tests
@mahmoudadel54 mahmoudadel54 requested a review from MV88 June 14, 2024 06:14
… in user from the User Details modal window

Description:
- Merge branch 'master' into enhance_10414 with fix conflicts
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

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

make sure to run locally documentation to check possible syntax errror

docs/developer-guide/mapstore-migration-guide.md Outdated Show resolved Hide resolved
docs/developer-guide/mapstore-migration-guide.md Outdated Show resolved Hide resolved
docs/developer-guide/mapstore-migration-guide.md Outdated Show resolved Hide resolved
docs/developer-guide/mapstore-migration-guide.md Outdated Show resolved Hide resolved
docs/developer-guide/mapstore-migration-guide.md Outdated Show resolved Hide resolved
… in user from the User Details modal window

Description:
- resolve review comments related to edits in migration file
@mahmoudadel54 mahmoudadel54 requested a review from MV88 June 25, 2024 20:32
@MV88 MV88 merged commit 032cad4 into geosolutions-it:master Jun 27, 2024
6 checks passed
@tdipisa
Copy link
Member

tdipisa commented Jun 27, 2024

@ElenaGallo please test in DEV and let us know if we can backport.

@ElenaGallo
Copy link
Contributor

@tdipisa even the admin can't see the GroupUserInfo section, is this correct?

admin

@tdipisa
Copy link
Member

tdipisa commented Jun 27, 2024

@tdipisa even the admin can't see the GroupUserInfo section, is this correct?

I've commented here:
#10414 (comment)

Something that has not been reported in the ACs. @mahmoudadel54 can you please take a look? Can we do that simply by config or dev is necessary for it?

@mahmoudadel54
Copy link
Contributor Author

mahmoudadel54 commented Jun 27, 2024

@tdipisa even the admin can't see the GroupUserInfo section, is this correct?

I've commented here: #10414 (comment)

Something that has not been reported in the ACs. @mahmoudadel54 can you please take a look?

Ok I will take a look for it
Dev is necessary for it even we put a config for theis requirement

Is it required to make it optional based on config to show or hide group for admin ? Or group should be shown by default for admin without using config? @tdipisa

@tdipisa
Copy link
Member

tdipisa commented Jun 27, 2024

@mahmoudadel54 thank you for this check. Let's leave this as it is now since it is not so fundamental and the config you have provided is not the default one. @ElenaGallo if there is nothing else we can we can ask @mahmoudadel54 to backport.

@ElenaGallo
Copy link
Contributor

Test passed, @mahmoudadel54 please backport to 2024.01.xx

@ElenaGallo ElenaGallo added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Jun 27, 2024
@mahmoudadel54
Copy link
Contributor Author

Test passed, @mahmoudadel54 please backport to 2024.01.xx

backport is done ---> #10447

@tdipisa tdipisa removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Jun 28, 2024
tdipisa pushed a commit that referenced this pull request Jun 28, 2024
…f logged in user from the User Details modal window (#10415) (#10447)

* #10418: Share tool - the 'Add place mark and zoom to sharing link' option is not applied correctly (#10419)

* backport #10414 with fix conflict in migration file

* fix migration conflict during merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the list of associated groups of logged in user from the User Details modal window
4 participants