-
-
Notifications
You must be signed in to change notification settings - Fork 4.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
Combine sessions and app passwords view into one single view #5166
Conversation
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst, thanks for your PR! By analyzing the history of the files in this pull request, we identified @icewind1991, @Faldon and @eppfel to be potential reviewers. |
Looks good and does what it should. Any reason why it is still |
|
||
<div id="apppasswords" class="section"> | ||
<h2><?php p($l->t('App passwords'));?></h2> | ||
<p class="settings-hint"><?php p($l->t('Here you can generate individual passwords for apps so you don’t have to give out your password. You can revoke them individually too.'));?></p> |
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.
The App passwords title should still be there though, no? For federated sharing we have it as a h3.
At least the settings-hint should stay since otherwise people don’t have a clue what to use the »App name« and »Create new app password« controls for, right?
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.
@jancborchardt can you fix it directly please?
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 be good to have my questions answered beforehand. :) Do we want to abolish the heading to just make it one consistent list, or use it as a h3?
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.
For activity we also have it as an <h2>.
Maybe the section should be called something like "Security", "Web, desktop mobile clients and app specific passwords that currently have access to your account."
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.
Tested and works 👍
Ignore failures, because they are only due to timeouts. |
@ChristophWurst @nickvergessen Could you create the backport for stable12? |
@MorrisJobke what about the open comments from jan? 😛 |
* was removed with #5166 so there is no more apppasswords-section Signed-off-by: Marius Blüm <marius@lineone.io>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Fixes #4295
@oparoz @MorrisJobke is that what you described in the ticket? I presume we have to adjust the description of the sessions section accordingly. Suggestions are very welcome!
@rullzer this potentially breaks the app password link from our clients because the link is hard-coded IIRC. Should we redirect it to the sessions section somehow?