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

Add dotfiles to user directory #213

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add dotfiles to user directory #213

wants to merge 2 commits into from

Conversation

hectron
Copy link

@hectron hectron commented Jul 8, 2019

Problem

Whenever a user has to wipe out their .kryptonite or .settings, they have to navigate to wherever fb-messenger-cli is installed. In addition, the file names .kryptonite and .settings do not make much sense if they are found elsewhere.

Proposal

This pull request:

  • changes .kyptonite to .fbmessenger.enc and .settings to .fbmessengerrc
  • changes where dotfiles are stored. They are now stored in the user's home directory, or wherever their FB_MESSENGER_DATA_DIR environment variable is set to

In addition to this, it fixes the regression tests, so that whenever they are run, a users' settings are not altered. The tests also now won't ping @Alex-Rose directly. 😉

This commit adds the dotfiles to the user's home directory. It also
renames the dotfiles to `.fbmessenger.enc` and `.fbmessengerrc`, to help
distinguish what the files are actually consumed by.
This commit adds tests that are work.
messenger.getFriends((_, friends) => {
const myself = friends[json.c_user];

messenger.getMessagesGraphQl(myself.vanity, myself.id, 10, (_err, messages) => {
Copy link
Author

Choose a reason for hiding this comment

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

I changed this test to use getMessagesGraphQL, since I could not retrieve any messages using getMessages anymore.


messenger.getFriends((_, friends) => {
const myself = friends[json.c_user];
messenger.sendMessage(myself.vanity, myself.id, 'Running tests - Send message', done);
Copy link
Author

Choose a reason for hiding this comment

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

This will ping the user who is using the library:

image


describe('Crypt', () => {
it('getInstance() should create a new singleton object', () => {
const crypt = Crypt.getInstance();
Copy link
Author

Choose a reason for hiding this comment

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

I had an issue using getInstance, so instead this PR monkey patches Crypt's filename and filepath for testing.

It creates a .test.fbmessenger.enc file in the lib/test directory.


after(() => {
Crypt.filename = oldCryptFilename;
Crypt.filepath = oldCryptFilepath;
Copy link
Author

Choose a reason for hiding this comment

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

We restore Crypt back to it's original state.

const os = require('os');
const path = require('path');

const normalizeFilePath = filePath => {
Copy link
Author

Choose a reason for hiding this comment

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

This expands the home directory if it contains a tilde.

This is copied from the untildify javascript library.

};

const getDataDirectory = () => {
const environmentDataDirectory = process.env.FB_MESSENGER_DATA_DIR;
Copy link
Author

Choose a reason for hiding this comment

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

This allows the user to specify a custom location for their dotfiles.

@p1ho
Copy link

p1ho commented Jul 8, 2019

I like the direction you're taking this, but just a few questions.

  1. Because those 2 files were intended to stay inside the folder, it was unnecessary to give them the .fbmessenger prefix. Could you give a scenario where users would find those 2 files elsewhere?

  2. Would moving those files out of the installation folder mean that when fb-messenger-cli is being uninstalled, the 2 files would stay intact instead of being wiped? Also, would there be permission issues (Say program tries to create settings file and credential file in a folder where it does not have write permission)?

  3. If wiping those 2 files is a desired functionality, would it be better to instead create flags for it, so you could, for example, call
    $ fb-messenger-cli --wipe-login or
    $ fb-messenger-cli --wipe-settings
    to interact with those 2 files?

@hectron
Copy link
Author

hectron commented Jul 19, 2019

Thanks for the follow up, @p1ho! I appreciate you taking the time to ask questions.

  1. I figured FB Messenger CLI might want to follow the convention that most command line programs follow: storing configuration files in a user's home directory.
  2. The files would stay in tact after removing the software. I do not know if it's standard behavior to remove the configuration itself upon removing software.
    2.1. That's a great question! I do not think a user can run into installation problems in their home directory (assuming they were able to install the CLI), but they can run into installation problems if they set FB_MESSENGER_DATA_DIR to a folder they do not have permission to write to.
  3. I like having transparency into how an application is configured, and am an advocate for home directory dot files. If this approach doesn't seem like the right approach, a flag to wipe those two files would be the next proposed solution!

@p1ho
Copy link

p1ho commented Jul 20, 2019

Thanks for answering! I'd say everyone has their own habits, so I won't be too opinionated on this.

On Windows, while it's not standard behavior to remove previous configuration, uninstallers usually prompt users whether they want to do that on uninstall (it'd be like a checkbox in the GUI).

const homeDirectory = os.homedir();

if (homeDirectory) {
return filePath.replace(/^~(?=$|\/|\\)/, homeDirectory);
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a path helper that sanitize strings instead of a regex? I always prefer platform agnostic helper/libs for those sort of things

@Alex-Rose
Copy link
Owner

Hi @hectron I'm going to try your PR especially to see how it works out on Windows. Sorry for the late response, as you can imagine we're pretty busy with work and all...

Thanks for your contribution!

@hectron
Copy link
Author

hectron commented Aug 8, 2019

@Alex-Rose , I think there's a bug where if you log in for the first time in a while, it does a different flow than when you've already been signed in, which results in an infinite loop.

I've had success by closing the app and trying again. However, the last few times my account has been flagged by Facebook as fraudulent, so I haven't tried to use the app too much to diagnose.

@p1ho
Copy link

p1ho commented Aug 8, 2019

@Alex-Rose , I think there's a bug where if you log in for the first time in a while, it does a different flow than when you've already been signed in, which results in an infinite loop.

I've had success by closing the app and trying again. However, the last few times my account has been flagged by Facebook as fraudulent, so I haven't tried to use the app too much to diagnose.

I can confirm this happened on my machine as well.

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.

3 participants