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

Migrate auth and settings to graphQL (was meteor) #19507

Merged

Conversation

Scroody
Copy link
Collaborator

@Scroody Scroody commented Jan 24, 2024

What does this PR do?

This PR migrates the client settings to graphQL and also most parts of authtentication

Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Copy link
Member

@antobinary antobinary left a comment

Choose a reason for hiding this comment

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

Impressive work @Tainan404 and @Scroody !!!

const { children, Fallback } = this.props;

return (error ? (<Fallback {...this.state} />) : children);
const fallbackElement = Fallback && error
? <Fallback error={error || {}} errorInfo={errorInfo} /> : <div>Something went wrong</div>;
Copy link
Member

Choose a reason for hiding this comment

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

i18n needed for Something went wrong

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is, but this is being used in the fallback of error boundary and the intl loader is bellow the boundary, so the provider it's not accessible before it

callback(endedReason, () => Meteor.disconnect());
logger[log]({ logCode: 'startup_client_usercouldnotlogin_error' }, `User could not log in HTML5, hit ${code}`);
console.error({ logCode: 'startup_client_usercouldnotlogin_error' }, `User could not log in HTML5, hit ${code}`);
Copy link
Member

Choose a reason for hiding this comment

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

logger could be POSTed to the server. console.error is client only. Any chance we can keep logger?

Copy link
Member

Choose a reason for hiding this comment

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

The logger is heavely dependent of the settings that are loaded after this component be loaded

if (errorMessageDescription in intlMessages) {
errorMessageDescription = intl.formatMessage(intlMessages[errorMessageDescription]);
if (error) {
errorMessageDescription = `Error: ${JSON.stringify(error)}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this log may actually include newline characters, making it difficult to parse/consume. Please double check or just use spaces instead of newline.

permission:
columns:
- clientSettingsJson
- meetingId
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you add this prop if it is not being used?

@@ -15,10 +15,11 @@ select_permissions:
meetingId:
_eq: X-Hasura-MeetingId
comment: ""
- role: not_joined_bbb_client
- role: pre_join_bbb_client
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change should break the client!

Comment on lines 12 to 19
const client = new Client({
host: 'localhost',
port: 5432,
database: 'bbb_graphql',
user: 'bbb_frontend',
password: 'bbb_frontend',
query_timeout: 30000,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This values should come from an env config.
Just like Redis process.env.REDIS_HOST.

Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@antonbsa
Copy link
Member

antonbsa commented Mar 1, 2024

I've tried many times to reproduce the failure on Chat › Private chat disabled when the user leaves meeting test, but it hasn't happened even once locally. I ran the other tests and tested manually; nothing odd was found. So I added the flaky flag to avoid running it in CI until we figure out what's happening. Besides that, I've pushed some other related changes to the testing code

Copy link

github-actions bot commented Mar 4, 2024

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Copy link

sonarcloud bot commented Mar 6, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

github-actions bot commented Mar 6, 2024

Automated tests Summary

All the CI tests have passed!

@TiagoJacobs
Copy link
Member

Hello @Tainan404 - as we discussed, this PR opens door for removing some old code, like this one:

I am merging it considering a subsequent cleanup PR is coming.

@TiagoJacobs TiagoJacobs merged commit 58a0efe into bigbluebutton:v3.0.x-release Mar 6, 2024
27 checks passed
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.

None yet

6 participants