-
Notifications
You must be signed in to change notification settings - Fork 19
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
Merge SSO changes into dev #2342
Conversation
* Overhaul of most configuration files * No longer using semver; update CHANGELOG.MD
* feature/deseng415: Added recording of date with feedback submission and displaying the data on admin side. * feature/deseng415: Fixed feedback schema, removed yup import, fixed change log date.
* bugfix/deseng429: Removed outdated service class. * bugfix/deseng429: Changed version and changelog to match deployments to gdx-main.
#2338) Co-authored-by: Alex <awintschel@gmail.com>
Co-authored-by: Alex <awintschel@gmail.com>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
'testing': TestConfig, | ||
} | ||
try: | ||
print(f'Loading configuration: {environment}...') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is print the logging mechanism for this application, no logging plugin or code
I'm guessing you don't have access to the logger at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print was the logging mechanism previously used in this file. I can upgrade to current_app.logger.debug
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind; I cannot. current_app
is only defined after the configuration is set up, and that is done in this file. I'll write a ticket to look into uses of print() in the app and see if there are better alternatives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just reviewing previous commits to be merged into main branch.
Just a final review or look of code, no in depth look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some great changes in here. I appreciate how everything is properly labelled and organized. Gender-neutral comments are always an improvement as well. Maybe resolve Shawn's point about print if applicable. I won't block you, approved :)
@@ -99,7 +97,7 @@ def _get_scope_options(user_roles, has_team_access): | |||
return EngagementScopeOptions(restricted=False) | |||
if has_team_access: | |||
# return those engagements where user has access for edit members.. | |||
# either he has edit_member role or check if he is a team member | |||
# either they have edit_member role or check if they are a team member |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well done
Issue #: 🎟️DESENG-408
Description of changes:
Most of the configuration changes required to make MET function correctly when using the BC Government's hosted Single Sign On Keycloak servers.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).