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

Allow admin user to reset passwords #1765

Merged
merged 1 commit into from
Feb 14, 2018
Merged

Conversation

aledeg
Copy link
Member

@aledeg aledeg commented Jan 12, 2018

See #960

Comments are welcomed.

@aledeg aledeg force-pushed the password-update branch 2 times, most recently from 0f8f3fb to 81df712 Compare January 12, 2018 20:59
@@ -101,6 +101,10 @@
'_' => 'User %s has been deleted',
'error' => 'User %s cannot be deleted',
),
'updated' => array(
'_' => 'The user %s has been updated',
Copy link
Member

@Frenzie Frenzie Jan 12, 2018

Choose a reason for hiding this comment

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

For consistency with the other strings (see a couple of lines above) that should be "User %s…"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@aledeg aledeg added this to the 1.10.0 milestone Jan 12, 2018
<label class="group-name" for="current_user"><?php echo _t('admin.user.selected'); ?></label>
<div class="group-controls">
<select id="current_user" class="select-change" name="username">
<option selected="selected">&nbsp;</option>
Copy link
Member

@Alkarex Alkarex Jan 16, 2018

Choose a reason for hiding this comment

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

Good idea, and could you please do the same thing (i.e. empty first option) for the next list of users a bit further down?
Detail: I would prefer a simple space, or Unicode non-breaking space, as I am not so found of entities.

Copy link
Member Author

Choose a reason for hiding this comment

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

No pbs for the space.
On the other hand, I've tried to replicate the same thing for the other list but there is JS involved in that one. It's not that easy and I didn't take the time to dig into it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've found a way :)

@Alkarex
Copy link
Member

Alkarex commented Jan 16, 2018

At a first glance, it looks very good to me 👍
I will try to test very soon.
I am wondering whether it is necessary to have two times the list of users, but there may be a point in having distinct areas for updating vs. deleting.

@aledeg
Copy link
Member Author

aledeg commented Jan 16, 2018

My initial idea was to split things between updating and deleting to avoid confusion.

@aledeg aledeg force-pushed the password-update branch 2 times, most recently from 86d7c99 to f3d3479 Compare January 16, 2018 19:13
@Alkarex Alkarex modified the milestones: 1.11.0, 1.10.0 Feb 5, 2018
$username = Minz_Session::param('currentUser');
}
$this->view->current_user = $username;
$this->view->current_user = Minz_Request::param('u');
Copy link
Member

Choose a reason for hiding this comment

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

When simply browsing to ?c=user&a=manage, $this->view->current_user seems to be null, and I get the following error:

<br />
<b>Fatal error</b>:  Uncaught Error: Call to a member function execute() on boolean in ./FreshRSS/app/Models/EntryDAO.php:879
Stack trace:
#0 ./FreshRSS/app/Controllers/userController.php(142): FreshRSS_EntryDAO-&gt;count()
#1 ./FreshRSS/lib/Minz/Dispatcher.php(118): FreshRSS_user_Controller-&gt;manageAction()
#2 ./FreshRSS/lib/Minz/Dispatcher.php(47): Minz_Dispatcher-&gt;launchAction('manageAction')
#3 ./FreshRSS/lib/Minz/FrontController.php(84): Minz_Dispatcher-&gt;run()
#4 ./FreshRSS/p/i/index.php(46): Minz_FrontController-&gt;run()
#5 {main}
  thrown in <b>./FreshRSS/app/Models/EntryDAO.php</b> on line <b>879</b><br />

Copy link
Member

Choose a reason for hiding this comment

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

Things seem to work fine with the previous code, for this little section

Copy link
Member Author

Choose a reason for hiding this comment

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

@Alkarex That's weird. I cannot reproduce.
My error_reporting is set to E_ALL. My user doesn't have authentication. I cannot think of something else that could break that.
Any ideas ?

Copy link
Member

Choose a reason for hiding this comment

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

When landing on the page the first time, Minz_Request::param('u') is not defined, is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not defined but set automatically to false.
By the way, I am using mysql. Maybe you're using something different.

Copy link
Member

Choose a reason for hiding this comment

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

Tested with SQLite, not tested yet with MySQL - but it would be an error if it does not fail :-P
Anyway, $entryDAO->count() (as well as $databaseDAO->size() probably) should not be called with an undefined (or false) username.
So I suggest to either keep the previous code, which loaded the default user (seems like the easiest), or move the count elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Alkarex I've kept what I was doing previously but I've added a validation on the user. This way, we don't have an error.
I've tried on Sqlite but I couldn't make frss work :'(

@Alkarex Alkarex merged commit ac3e383 into FreshRSS:dev Feb 14, 2018
@aledeg aledeg deleted the password-update branch February 14, 2018 21:22
@Alkarex
Copy link
Member

Alkarex commented Feb 14, 2018

Damn, a bit too fast:

Notice: Undefined property: Minz_View::$nb_articles in ./FreshRSS/app/views/user/manage.phtml on line 100

Notice: Undefined property: Minz_View::$size_user in ./FreshRSS/app/views/user/manage.phtml on line 101

@aledeg
Copy link
Member Author

aledeg commented Feb 14, 2018

Oups, should I make a new PR or update the previous one?

@Alkarex
Copy link
Member

Alkarex commented Feb 14, 2018

New one please :-)

@aledeg
Copy link
Member Author

aledeg commented Feb 14, 2018

👍

aledeg added a commit to aledeg/FreshRSS that referenced this pull request Feb 14, 2018
Alkarex pushed a commit that referenced this pull request Feb 14, 2018
Alkarex added a commit that referenced this pull request Feb 14, 2018
javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020
javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020
javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020
mdemoss pushed a commit to mdemoss/FreshRSS that referenced this pull request Mar 25, 2021
mdemoss pushed a commit to mdemoss/FreshRSS that referenced this pull request Mar 25, 2021
mdemoss pushed a commit to mdemoss/FreshRSS that referenced this pull request Mar 25, 2021
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

3 participants