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

Fixes #29792 - Add current taxonomies and user in react context #7655

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

amirfefer
Copy link
Member

current loc / org and user-id should be a single source of truth across core and plugins and react context may be the place.
The need for this PR has been raised here:
https://github.com/theforeman/foreman_plugin_template/pull/32/files#diff-2ee0f3275ce16ab586fcec80d0b2124cR11

@amirfefer amirfefer changed the title Add current taxonomies and user in react context Fixes #29792 - Add current taxonomies and user in react context May 13, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 75.168% when pulling 0397e79 on amirfefer:29792 into 368d9ed on theforeman:develop.

UISettings: ui_settings, version: SETTINGS[:version].short, docUrl: documentation_url,
location: Location.current&.id,
organization: Organization.current&.id,
user: User.current&.id
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: How about locationId, organizationId, userId for the keys?

Copy link
Member

Choose a reason for hiding this comment

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

Should we make these into objects that also contain the taxonomy label or the user full name so they can be used for display purposes?

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense

@xprazak2
Copy link
Contributor

Looks good, I just added one comment.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

How will we handle changing the current taxonomy in the UI (right now it won't be a problem due to triggering full page reload, but as pages change to react it might), as well as impersonation?

@tbrisker
Copy link
Member

what's the status here @amirfefer ?

@ofedoren
Copy link
Member

@amirfefer needs rebase :(

@amirfefer
Copy link
Member Author

@tbrisker, @xprazak2, rebased, plus I added a few changes.

How will we handle changing the current taxonomy in the UI (right now it won't be a problem due to triggering full page reload, but as pages change to react it might), as well as impersonation?

Changing a taxonomy triggers a full-page reload, including impersonation, so the context is updated.
React context is mutable via the client-side, so in the future, we can still use this pattern.

{
UISettings: ui_settings, version: SETTINGS[:version].short, docUrl: documentation_url,
location: Location.current.attributes.slice('id', 'title'),
organization: Organization.current.attributes.slice('id', 'title'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a check if current loc/org is set, otherwise fails on undefined method when in 'Any Context'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @xprazak2, should work now

@xprazak2 xprazak2 merged commit b0d5a6c into theforeman:develop Jan 8, 2021
@xprazak2
Copy link
Contributor

xprazak2 commented Jan 8, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants