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

IBX-3966: Used user_content_type_identifier configuration key inside user service #155

Merged
merged 5 commits into from
Oct 14, 2022

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Oct 4, 2022

Question Answer
JIRA issue IBX-3966
Type improvement
Target Ibexa version v4.3
BC breaks yes/no

Not sure how we should handle BC in that case. Overriding userClassID was hard and as I am aware totally not documented. On the same way, just doing that without changing the adminUI parameter also did nothing in terms of using different CT than the default one.

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ibexa/engineering).

@ViniTou ViniTou force-pushed the configr-resolver-user-service branch from d364b2f to 9f8bedd Compare October 4, 2022 06:42
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

I wonder if this should be a Repository setting, not SiteAccess. How would system behave if for admin you would have one identifier and for site other one? Not that we practiced this till now, but maybe it's a good moment to distinguish these settings.
Perhaps something we can discuss internally later today?

@alongosz alongosz requested a review from a team October 6, 2022 09:52
@ViniTou ViniTou requested a review from alongosz October 10, 2022 08:59
@ViniTou
Copy link
Contributor Author

ViniTou commented Oct 10, 2022

@alongosz
As discussed, I had moved resolving configuration into user service itself.

@ViniTou ViniTou requested review from alongosz and a team October 10, 2022 11:08
@ViniTou ViniTou force-pushed the configr-resolver-user-service branch from 8db2615 to 3c211c6 Compare October 10, 2022 11:14
@alongosz alongosz requested a review from a team October 10, 2022 11:28
@konradoboza konradoboza requested a review from a team October 11, 2022 06:16
Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on IbexaDXP 4.3 commerce.

@alongosz alongosz merged commit a6109d5 into main Oct 14, 2022
@alongosz alongosz deleted the configr-resolver-user-service branch October 14, 2022 13:27
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.

8 participants