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

CLI update user #1602

Merged
merged 11 commits into from
Aug 19, 2017
Merged

CLI update user #1602

merged 11 commits into from
Aug 19, 2017

Conversation

Alkarex
Copy link
Member

@Alkarex Alkarex commented Jul 22, 2017

#1600
Not tested

@Alkarex Alkarex added this to the 1.8.0 milestone Jul 22, 2017
@Alkarex
Copy link
Member Author

Alkarex commented Jul 22, 2017

@oupala Here is a patch, but not tested at all. Would you like to give it a spin?

@Alkarex Alkarex added the Work in progress 🚧 Wait before merging label Jul 22, 2017
@oupala
Copy link
Contributor

oupala commented Jul 22, 2017

I tested your patch (congrats for the reactivity !) and I got a few things to mention.

All tests have been performed on Alkarex/FreshRSS:cli_update_user.

  • some parameters are not consistent:
>sudo ./do-install.php --default-user myuser --language fr --api-enabled --db-type sqlite 
Usage: do-install.php --default_user admin ( --auth_type form --environment production --base_url https://rss.example.net/ --language en --title FreshRSS --allow_anonymous --api_enabled --db-type mysql --db-host localhost:3306 --db-user freshrss --db-password dbPassword123 --db-base freshrss --db-prefix freshrss_ --disable_update )

Parameters are sometime using _ and sometime - where - is mainly expected.

It should be --default-user instead of --default_user and same thing each time an _ is used. Tell my if you need me to open a new issue for this item as it is about do-install.php.

  • inline help is not consistent with README:

inline help:

>sudo ./update-user.php --usser myuser --password 'apassword'
Usage: update-user.php --user username ( --password 'password' --api-password 'api_password' --language en --email user@example.net --token 'longRandomString' )

README:

./cli/update-user.php --user username ( --password 'password' --api-password 'api_password' --language en --email user@example.net --token 'longRandomString' --purge_after_months 3 --feed_min_articles_default 50 --feed_ttl_default 3600 --since_hours_posts_per_rss 168 --min_posts_per_rss 2 --max_posts_per_rss 400 )
  • maybe the most important thing is that I tried to update the password of the user, and I can't login any more, neither with the old password nor the new one:
>sudo ./update-user.php --user myuser --password 'anotherpassword'
FreshRSS updating user “myuser”…
Result: success

Any clue, trying to debug this ?

  • and last but not least, when the login fails, I'd like to go back to the login page, with a login error notification. Currently, I need to read the error message, to leave my keyboard, to click the ← Go back to your RSS feeds link, the to enter my credentials again.

Error 403 - Forbidden

You don’t have permission to access this page
← Go back to your RSS feeds

I'd like to have just a notification that the login failed, and to still be able to login without reloading the login page. Could it be possible? Once again, tell my if you need me to open a new issue for this item as it is about the login page.

@Alkarex
Copy link
Member Author

Alkarex commented Jul 22, 2017

Thanks for the feedback. No need for dedicated issues yet, as long as it is related to this pull request.
Partial answer:

  • I have updated the usage to be the same as the readme.
  • Could you please try again your password issue? I could not reproduce the problem. Have you remembered to re-apply the access rights if your changes are not made with the same user than your Web server?

@oupala
Copy link
Contributor

oupala commented Jul 26, 2017

Now that the usage consistency problem is fixed, there is another consistency problem: update-user.php permits to set much more parameters than create-user.php where both should allow to set the same parameters.

>sudo ./update-user.php --usser myuser --password 'apassword'
Usage: update-user.php --user username ( --password 'password' --api-password 'api_password' --language en --email user@example.net --token 'longRandomString' --purge_after_months 3  --feed_min_articles_default 50 --feed_ttl_default 3600 --since_hours_posts_per_rss 168 --min_posts_per_rss 2 --max_posts_per_rss 400 )
>sudo ./create-user.php --usser myuser --password 'apassword'
Usage: create-user.php --user username ( --password 'password' --api-password 'api_password' --language en --email user@example.net --token 'longRandomString --no-default-feeds' )

The following parameters are only available while updating a user, but not when creating it.

--purge_after_months 3 --feed_min_articles_default 50 --feed_ttl_default 3600 --since_hours_posts_per_rss 168 --min_posts_per_rss 2 --max_posts_per_rss 400

And there's a typo in create-user.php usage:

--token 'longRandomString --no-default-feeds'

Quotes are misplaced. They should be like that!

--token 'longRandomString' --no-default-feeds

@oupala
Copy link
Contributor

oupala commented Jul 26, 2017

I succeeded to use the update-user.php script. I think the issue was related to permissions as I was using sudo (then root) to execute the script.

I only tried to update the password. But I suppose if one parameter works, the others should work either.

So, the script seems to be pretty much validated!

@Alkarex
Copy link
Member Author

Alkarex commented Jul 27, 2017

Thanks @oupala
Typo fixed. Regarding the parameters, I am thinking that if the additional parameters of update-user are available, then this script can just be called after the creation. I would rather not duplicate far too many options if that can be avoided.

@oupala
Copy link
Contributor

oupala commented Jul 27, 2017

I understand your point of view. The famous DRY principle.

But following this principle, why not merge both script and create a create-or-update-user.php script which would create the user if it does not exist, and simply update it if it already exists?

@Alkarex
Copy link
Member Author

Alkarex commented Jul 27, 2017

Yes indeed, it would be possible to merge both scripts. However, the logic is not exactly the same in both (create user should fail if the username is taken, while update-user should fail if the username does not exist), and not all options make sense in both (e.g. --no-default-feeds which is only for the first creation).
At the cost of a slightly higher complexity, it would be possible to share a good part of the code between the two scripts, while keeping them distinct.

@oupala
Copy link
Contributor

oupala commented Jul 27, 2017

You're right on all points. I let you decide what is the best solution to get as less repeated code as possible.

Thanks anyway for adding the update user feature so quickly.

@Alkarex
Copy link
Member Author

Alkarex commented Aug 5, 2017

@oupala I have made an update to mutualise create-user and update-user, to get the same options. Not tested. Could you please give it a try?

@Alkarex Alkarex removed the Work in progress 🚧 Wait before merging label Aug 19, 2017
@Alkarex Alkarex merged commit f09039b into FreshRSS:dev Aug 19, 2017
@Alkarex Alkarex deleted the cli_update_user branch August 19, 2017 14:26
@Alkarex
Copy link
Member Author

Alkarex commented Aug 19, 2017

@oupala Merged in /dev. Tests welcome :-)

@Alkarex Alkarex modified the milestones: 1.8.0, 1.7.1 Aug 19, 2017
@Alkarex Alkarex modified the milestones: 1.7.1, 1.8.0 Aug 19, 2017
@oupala
Copy link
Contributor

oupala commented Sep 9, 2017

Seems to be ok. I juste tested dev branch and I succeeded to change my password via the CLI.

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
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

2 participants