-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Added email confirmation page #3573
Conversation
Added one component which will show email confirmed message and then redirect to the login page. Edited ACCOUNT_EMAIL_CONFIRMATION_ANONYMOUS_REDIRECT_URL from '/auth/login' to '/auth/email-confirmation' for redirection after successful email verification.
@Spectre-ak thanks for the contribution! Could you please fix linter warnings? |
@azhavoro sure I'll fix these Thanks |
Generally, these warnings are related to coding style, you can download the linter report as an artifact on the failed job page. |
okay, I'll look into these, is there a way to check for linter warnings locally? |
Yes, if you are using VS Code you need to install ESLint plugin https://openvinotoolkit.github.io/cvat/docs/contributing/development-environment/ |
okay @azhavoro , I'll fix these and let you know. |
@azhavoro I've updated the email-confirmed page |
@azhavoro linter warnings fixed and all checks passed. |
I'm not sure that all works as expected: git log -1
commit 4c32b7edfc0ee0ae261562bbd7638be61bc6a8fd (HEAD -> email_conf, pr/develop)
Author: Spectre-ak <62694340+Spectre-ak@users.noreply.github.com>
Date: Mon Aug 30 14:45:46 2021 +0530
updated email-confirmed page Steps to reproduce:
I don't see |
@azhavoro its working, I tested (on browser) just now, Actually since it is a react app, so js is needed for running the application, that's why curl response is plain html and js is not getting executed. |
In development environment it doesn't work in browser either, I don't see any notifications in case of email confirmation. Please make sure it works as expected in prod and dev modes. If this works for you, please provide the exact steps to check your PR. Thanks. |
How to test:
Attached a working sample email-confirm-page.mp4Thanks |
@Spectre-ak |
Quite minimalistic |
@azhavoro, Started cvat-ui using these
|
okay @bsekachev I can improve the design |
@Spectre-ak I cannot approve this PR until it doesn't work in development mode, please fix it. |
alright, I'll fix this |
After changing Can you please verify it? Thanks |
@Spectre-ak Hi, any updates on this? |
Hi @azhavoro, yes I'm working on it |
@azhavoro |
render() { | ||
return ( | ||
|
||
<section className='ant-layout'> |
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.
Please don't specify any classes here and use Antd components, you can find a lot of examples in the Antd documentation https://ant.design/components/layout/
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.
@azhavoro
I've removed class names and used antd components
@bsekachev Hi, could you please review? |
@azhavoro Sure |
cvat-ui/src/components/cvat-app.tsx
Outdated
<Redirect | ||
to={location.pathname.length > 1 ? `/auth/login/?next=${location.pathname}` : '/auth/login'} | ||
/> | ||
|
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.
cvat-ui/src/components/cvat-app.tsx
Outdated
@@ -39,6 +39,7 @@ import showPlatformNotification, { | |||
showUnsupportedNotification, | |||
} from 'utils/platform-checker'; | |||
import '../styles.scss'; | |||
import EmailConfirmationMessage from './email-confirmation-page/email-confirmed'; |
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.
Would rename it to EmailConfirmationPage
, what do you think?
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.
yes sure,
done
|
||
import React from 'react'; | ||
import { Link } from 'react-router-dom'; | ||
import { Layout, Row, Col } from 'antd'; |
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.
import { Layout, Row, Col } from 'antd'; | |
import { Col, Row } from 'antd/lib/grid'; | |
import Layout from 'antd/lib/layout'; |
const { Content } = Layout; | ||
|
||
/** | ||
* Component for displaying email confirmation message and then redirecting to the loginpage |
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.
* Component for displaying email confirmation message and then redirecting to the loginpage | |
* Component for displaying email confirmation message and then redirecting to the login page |
s | ||
</p> | ||
<Link to='/auth/login' id='link'>Or click this link</Link> | ||
|
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.
</Col> | ||
</Row> | ||
</Content> | ||
|
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.
</Content> | ||
|
||
</Layout> | ||
|
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.
/** | ||
* Component for displaying email confirmation message and then redirecting to the loginpage | ||
*/ | ||
class EmailConfirmationMessage extends React.Component { |
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.
Please look at antd Countdown component, if you use it, you don't need this lifecircle method and you don't need React ES6 components here.
Example:
function onFinish() {
document.getElementById('link').click();
}
const deadline = Date.now() + 1000 * 5;
ReactDOM.render(
<Row gutter={16}>
<Col span={12}>
<Countdown format='ss' title="Redirecting to login page after.." value={deadline} onFinish={onFinish} />
</Col>
</Row>,
mountNode,
);
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.
yes, i removed the class component. And with the help of Countdown component it was more easy.
return ( | ||
<Layout> | ||
<Content> | ||
<Row justify='center' align='middle' style={{ height: '100%', textAlign: 'center' }}> |
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.
Static css styless better to be declared in scss files.
|
||
if (counter === 0) { | ||
try { | ||
document.getElementById('link').click(); |
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.
better to use refs if possible. Draft code is:
const linkRef = useRef<Link>();
function onFinish() {
ref.current?.click();
}
return (
....
<Link ref={linkRef} ... />
)
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.
sure, I've changed to useRef
1. removed react class component and used functional component 2. removed setInterval and user antd countdown component 3. used useRef for redirecting to login page 4. added static style sheet for centering the whole message 5. changed EmailConfirmationMessage to EmailConfirmationPage
Hi @bsekachev , |
@@ -0,0 +1,8 @@ | |||
// Copyright (C) 2020 Intel Corporation |
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.
// Copyright (C) 2020 Intel Corporation | |
// Copyright (C) 2021 Intel Corporation |
* Component for displaying email confirmation message and then redirecting to the login page | ||
*/ | ||
|
||
function EmailConfirmationPage() { |
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.
function EmailConfirmationPage() { | |
function EmailConfirmationPage(): JSX.Element { |
const onFinish = () => { | ||
linkRef.current.click(); | ||
}; | ||
return ( |
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.
Just my thoughts how to do the component more reliable. Right now deadline is computed only once when the file is imported. In theory in case this page opened several times, it will not work as expected. In spite of the fact that this scenario is not expected to appear in real, I suggest followings:
const [deadline, setDeadline] = useState<null | number>(null);
useEffect(() => {
setDeadline(6);
}, [])
return (
deadline === null ? null : (
... current return code ...
)
)
What do you think?
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.
Or even simpler option probably will work:
<Countdown format='ss' title='Redirecting to login page after...' value={Date.now() + 1000 * 6} onFinish={onFinish} />
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.
Yes, this is needed. I actually did faced this case during development where the component was not reloading unless I refresh the browser.
Thanks
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.
Or even simpler option probably will work:
<Countdown format='ss' title='Redirecting to login page after...' value={Date.now() + 1000 * 6} onFinish={onFinish} />
yes, better and simpler
Almost brilliant ;) |
1. updated styles.scss 2. updated component
yes did and made the changes |
Great, thanks for the contribution! |
Happy to be a contributor😊 |
@dvkruchinin Could you please prepare a test for the PR? Just open the page and be sure that it redirects to login page after several seconds. |
Sure. A test will be prepared. |
Motivation and context
Fixes #2135
The issue
After email verification done by a user there is no email confirmation message displayed only redirection to the login page.
The solution
Added a page (
EmailConfirmationMessage
component) which will show email confirmation message and then redirect to the login page.Edited
ACCOUNT_EMAIL_CONFIRMATION_ANONYMOUS_REDIRECT_URL
in thecvat/setting/base.py
from
'/auth/login'
to'/auth/email-confirmation'
so that the user will be first redirected to the email-confirmation page (on successful email verification) and then to the login page.How has this been tested?
Tested locally by running the cvat app, the component is working fine, since it is running locally there is no email confirmation message sent to the user upon registration.
Checklist
develop
branchcvat-core, cvat-data and cvat-ui)
License
Feel free to contact the maintainers if that's a concern.