Migration of zuliprc location to a config directory #1503

Niloth-p · 2024-05-21T09:03:16Z

What does this PR do, and why?

Adds the following features:

Support recognizing zuliprc from multiple select locations.
- ~/zuliprc
- the user's downloads directory
- the user's config directory
Move zuliprc creation into user's config directory.
The new path would look like .config/zulip-terminal/zuliprc.
Create realm-wise zuliprc files in the configuration directory.
Each realm gets its own directory.
The new directory structure would look like:

<user's config directory>/zulip-terminal/
  org1-name/zuliprc
  org2-name/zuliprc
  zuliprc

Add new positional argument - prefix of realm name to get its configuration.
New command examples:
zulip-term zuli
zulip-term "full organization name"
Add -o argument to list realms with stored zuliprc.

$zulip-term -o
The following organizations are available:
  Zulip Community
  Another Organization

Add -n argument to login to a realm, and create config.

$zulip-term -n
Please enter your credentials to login into your Zulip organization.

What does this PR do, and why? (expired)

What does this PR do, and why?

Moves config location to 'XDG_CONFIG/zulip/'.
Makes zuliprc files dot files - .zuliprc.
Use independent org.zuliprc files for each organization.

Does not yet separate the zterm config from the api config.
Does not yet update the documents (README.md, FAQ.md, etc).
New tests not added.

The code is not yet meant for review.
This is only a draft to play around with the configuration in order to finalize what behavior is desired.

External discussion & connections

Discussed in #zulip-terminal in Moving config
Fully fixes #
Partially fixes issue Improve zuliprc location/specifying (eg. dot-file or .config/) #678
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

Niloth-p

Updated the PR description, to describe the new features.
Each item in that feature list is its own commit (in the same order), with preparatory commits in between (mentioned for easier reviewing).

I have not yet added tests in this branch.
The ones I had were too tightly tied to the functions that were introduced.
Planning to re-write them to directly test main([]) instead.

Reference: Testing cases:

Feel free to come up with any missing / edge cases.

default call (without positional argument)
- loose zuliprc file in config
- zuliprc files in config inside realms directories
  - one realm
  - multiple realms
- mix of both loose zuliprc and inside realms in config
- several zuliprc files found error
- no zuliprc in any locations
positional argument
- too few characters (<3)
- no org found
- several matching orgs
- one matching org, but zuliprc is inside subdirectory
- one matching org
- full name instead of prefix
- use along with --config-file option & -c options
login newly into a few servers, test the created directory structure
n option:
- if configuration for that realm already exists
- if a directory with that realm name exists, but it does not have a zuliprc file inside it
o option:
- no orgs found
- several orgs found

Niloth-p · 2024-06-11T05:56:31Z

setup.py

@@ -116,5 +116,6 @@ def long_description():
        "pytz>=2022.7.1",
        "tzlocal>=2.1",
        "pyperclip>=1.8.1",
+        "platformdirs>=4.2.0",


Version matches that of the dev dependency in Pipfile.lock.

You use that installation method? I use make most of the time.

This is presumably the current version, selected by pipfile, and is fine for setup.py.

Note that we add/upgrade dependencies in a separate commit.

Niloth-p · 2024-06-11T06:00:39Z

tests/cli/test_run.py

@@ -370,32 +370,25 @@ def unreadable_dir(tmp_path: Path) -> Generator[Tuple[Path, Path], None, None]:
    unreadable_dir.chmod(0o755)


-@pytest.mark.parametrize(


Removed parametrize as there's only one test case remaining, moved the values inside the test function.
The test function could be renamed to be more specific too.

Niloth-p · 2024-06-11T07:13:37Z

zulipterminal/cli/run.py

@@ -49,7 +49,7 @@ class SettingData(NamedTuple):

 DOWNLOADED_PATH_ZULIPRC = str(downloads_file_path() / "zuliprc")
 HOME_PATH_ZULIPRC = str(Path.home() / "zuliprc")
-CONFIG_PATH_ZULIPRC = str(config_file_path() / "zulip-terminal" / "zuliprc")


Changed names here.
Felt CONFIG_PATH_ZULIPRC was a more consistent name for the single zuliprc in config.
Feel ZULIP_CONFIG_PATH would be better when pointing to the directory. Could use CONFIG_PATH_ZULIP too.
Would be open to rebasing any name changes if the noise of this change is more important than the consistency of the name in the initial commits.

Niloth-p · 2024-06-11T07:17:11Z

zulipterminal/cli/run.py

+    valid_locations.extend(
+        str(file) for file in Path(ZULIP_CONFIG_PATH).glob("zuliprc")
+    )
+    valid_locations.extend(


The current implementation explicitly does not use Path(ZULIP_CONFIG_PATH).glob("**/zuliprc"), to avoid the situation (in later commits) where the realm has multiple zuliprc files inside it (in folders or as loose file). This implementation will only recognize the outermost zuliprc within a realm's directory, irrespective of the subdirectories' contents.

Niloth-p · 2024-06-11T07:18:48Z

zulipterminal/cli/run.py

+        exit_with_error(
+            f"Could not find any organizations starting with '{realm_name_prefix}'"
+        )
+    matching_files = list(matching_dirs[0].glob("zuliprc"))


If we used path.glob(**/zuliprc) in the previous commit, this is where we would need to add another case where len(matching_files) > 1.

Niloth-p · 2024-06-11T07:20:22Z

zulipterminal/cli/run.py

@@ -448,6 +448,30 @@ def list_themes() -> str:
    )


+def path_to_realm_zuliprc(realm_name_prefix: str) -> str:
+    if len(realm_name_prefix) < 3:


Assumption: There are no zulip realms whose name property contains 2 or fewer characters.
We could remove this case.

Niloth-p · 2024-06-11T07:25:05Z

zulipterminal/cli/run.py

    )
    while zuliprc_path == "" or not path.exists(zuliprc_path):
        try:
-            zuliprc_path = fetch_zuliprc(zuliprc_path)
+            zuliprc_path = fetch_zuliprc(zuliprc_path, new_realm)


Re. naming,
used 'realm' for all internal references,
and 'org' / 'organization' for all references visible to the user.

rsashank

Great work, @Niloth-p! I manually tested it, and it works well. I added a few comments in some areas where I think it would be nice to consider changes, but those are more opinion-based.

This looks very useful!

rsashank · 2024-06-18T12:17:51Z

zulipterminal/cli/run.py

+    locations_checked = (
+        zuliprc_path
+        if zuliprc_path != ""
+        else f"any of the following locations: "
+        f"{DOWNLOADED_PATH_ZULIPRC} or {CONFIG_PATH_ZULIPRC} or {HOME_PATH_ZULIPRC}"
+    )
    print(
-        f"{in_color('red', f'zuliprc file was not found at {zuliprc_path}')}"
+        f"{in_color('red', f'zuliprc file was not found at {locations_checked}')}"


zuliprc file was not found at any of the following locations: /home/ubuntu/Downloads/zuliprc or /home/ubuntu/.config/zulip-terminal or /home/ubuntu/zuliprc

I think this error seems a bit odd, only my opinion though. Could we put each location on a separate line? :)

rsashank · 2024-06-18T12:20:25Z

zulipterminal/cli/run.py

@@ -49,7 +49,7 @@ class SettingData(NamedTuple):

 DOWNLOADED_PATH_ZULIPRC = str(downloads_file_path() / "zuliprc")
 HOME_PATH_ZULIPRC = str(Path.home() / "zuliprc")
-CONFIG_PATH_ZULIPRC = str(config_file_path() / "zulip-terminal" / "zuliprc")


rsashank · 2024-06-18T14:13:50Z

zulipterminal/cli/run.py

+def check_for_default_zuliprc() -> str:
+    zuliprc_locations = [
+        DOWNLOADED_PATH_ZULIPRC,
+        HOME_PATH_ZULIPRC,
+    ]
+    valid_locations = [
+        location for location in zuliprc_locations if Path(location).exists()
+    ]
+    valid_locations.extend(
+        str(file) for file in Path(ZULIP_CONFIG_PATH).glob("zuliprc")
+    )
+    valid_locations.extend(
+        str(file) for file in Path(ZULIP_CONFIG_PATH).glob("*/zuliprc")
+    )
+
+    count = len(valid_locations)
+    if count > 1:
+        valid_locations_string = "\n".join(valid_locations)
+        exit_with_error(
+            f"Found multiple zuliprc files at:\n{valid_locations_string}\n"
+            "Please retry by specifying the path to your target zuliprc file."
+        )
+    elif count == 1:
+        return valid_locations[0]
+    return ""


This implementation seems great! However, should we consider giving priority to HOME_PATH_ZULIPRC if multiple configuration files are found, since that was previously the location of zuliprc?

If users want to use a different configuration file instead of the default, they can specify a specific target zuliprc file to override the default. We can notify the config file used while loading ZT, similar to how we do with the settings.

Alternatively, instead of exiting with an error, we could include an index and prompt the user to choose which organization they would like to log into.

For example:

Choose which organization you'd like to log in to (Enter the number): 1: Zulip Community: chat.zulip.org 2: CTF Chat: ctf.zulipchat.com 3: Example: example.zulip.com

rsashank · 2024-06-18T14:18:46Z

zulipterminal/cli/run.py

+    matching_dirs = [
+        dir
+        for dir in Path(ZULIP_CONFIG_PATH).glob("*/")
+        if dir.name.startswith(realm_name_prefix)
+    ]


Can we make this case insensitive?

Maybe:

matching_dirs = [ dir for dir in Path(ZULIP_CONFIG_PATH).glob("*/") if dir.name.startswith(realm_name_prefix) or dir.name.lower().startswith(realm_name_prefix) ]

rsashank · 2024-06-18T14:24:09Z

zulipterminal/cli/run.py

+    if len(matching_dirs) > 1:
+        matching_dirs_string = "\n".join(str(dir) for dir in matching_dirs)
+        exit_with_error(
+            f"Found multiple organizations starting with '{realm_name_prefix}':\n"
+            f"{matching_dirs_string}\n"
+        )


I'm not sure how feasible this is, but should we consider prompting the user to select which directory to log into from the matched directories?

rsashank · 2024-06-18T14:25:44Z

zulipterminal/cli/run.py

+    elif len(matching_dirs) == 0:
+        exit_with_error(
+            f"Could not find any organizations starting with '{realm_name_prefix}'"
+        )


Maybe we could suggest using zulip-term -o to show the list of available organizations. Alternatively, we could display the list along with the error message?

rsashank · 2024-06-24T20:30:25Z

Thanks for updating this, @Niloth-p! I'll leave it for the mentor and maintainers to review. I'm removing the buddy review label now.

neiljp

@Niloth-p I hope this review helps get you back on track with this work 👍

The discussion in the channel seems to have helped too :)

neiljp · 2024-08-07T04:20:06Z

.github/workflows/lint-and-test.yml

-    name: Install & test - CPython 3.7 (ubuntu), codecov
+    name: Install & test - CPython 3.8 (ubuntu), codecov


If there's a temporary workaround, it's best to keep it in a separate commit - it makes it easier to drop later, and keep the other changes separate.

neiljp · 2024-08-07T04:21:50Z

setup.py

@@ -116,5 +116,6 @@ def long_description():
        "pytz>=2022.7.1",
        "tzlocal>=2.1",
        "pyperclip>=1.8.1",
+        "platformdirs>=4.2.0",


You use that installation method? I use make most of the time.

This is presumably the current version, selected by pipfile, and is fine for setup.py.

Note that we add/upgrade dependencies in a separate commit.

neiljp · 2024-08-07T04:23:36Z

zulipterminal/platform_code.py

+def downloads_file_path() -> Path:
+    return Path(user_downloads_dir())


I think we discussed this not being necessary, at least at this stage - though it potentially could be used for other cases, like file downloads, I'm not sure.

neiljp · 2024-08-07T04:29:25Z

zulipterminal/cli/run.py

+DOWNLOADED_PATH_ZULIPRC = str(downloads_file_path() / "zuliprc")
+HOME_PATH_ZULIPRC = str(Path.home() / "zuliprc")
+CONFIG_PATH_ZULIPRC = str(config_file_path() / "zulip-terminal" / "zuliprc")


The CONFIG_PATH_ZULIPRC part fits nicely with the remaining code change in the earlier commit, and then possibly with a change later, into the first commit using it?

The HOME_PATH_ZULIPRC part would be a nice refactor to improve platform compatibility, given the monkeypatch change at least. It doesn't assume any movement to the new config on its own.

neiljp · 2024-08-07T04:52:05Z

zulipterminal/cli/run.py

+    supported_locations = [
+        CONFIG_PATH_ZULIPRC,
+        HOME_PATH_ZULIPRC,
+        DOWNLOADED_PATH_ZULIPRC,
+    ]
+    locations_checked = (
+        zuliprc_path
+        if zuliprc_path != ""
+        else "any of the following locations:\n  " + "\n  ".join(supported_locations)
+    )
    print(
-        f"{in_color('red', f'zuliprc file was not found at {zuliprc_path}')}"
+        f"{in_color('red', f'zuliprc file was not found at {locations_checked}')}"


Generally we shouldn't keep multiple copies of data - here the supported_locations, which are also in the other function you added.

The interesting part about this change is that it only affects this first line of the print, which suggests the change can be isolated to the calling location instead, if that refactor occurs first.

This doesn't surprise me, since this part of the code is rather non-ideal as it stands. This function name could also be improved, as possibly could others as they are updated. Once the 'could not be found' line is moved, this function becomes very clearly a 'prompt for login details, fetch api details and write zuliprc'; the leading line confuses that slightly since it refers to a file that's been looked for already, not the new one :)

neiljp · 2024-08-07T05:41:24Z

tests/cli/test_run.py

    tmp_path, unusable_path = unreadable_dir

    # This is default base path to use
    zuliprc_path = os.path.join(str(tmp_path), path_to_use)
-    mocker.patch("zulipterminal.cli.run.HOME_PATH_ZULIPRC", zuliprc_path + "/zuliprc")
+    mocker.patch("zulipterminal.cli.run.CONFIG_PATH_ZULIPRC", zuliprc_path + "/zuliprc")


It would be good to have these overall changes go straight to the alias-name/zuliprc format, simply so we can avoid the intermediate step of files in the folder between them :)

neiljp · 2024-08-07T05:42:00Z

zulipterminal/cli/run.py

@@ -333,6 +333,7 @@ def _write_zuliprc(
    Only creates new private files; errors if file already exists
    """
    try:
+        Path(to_path).parent.mkdir(parents=True, exist_ok=True)


What permissions is this folder made with? Does it matter for security?

neiljp · 2024-08-07T06:10:07Z

zulipterminal/cli/run.py

@@ -452,6 +452,30 @@ def list_themes() -> str:
    )


+def path_to_realm_zuliprc(realm_name_prefix: str) -> str:


I'll wait to see this updated for the new version first, but I expect this and later commits would be the mapping of aliases to zuliprcs instead.

neiljp · 2024-08-07T06:15:01Z

zulipterminal/cli/run.py

+    missing_zuliprc_text = (
+        ""
+        if new_realm
+        else f"{in_color('red', f'zuliprc file was not found at {locations_checked}')}"
+    )
    print(
-        f"{in_color('red', f'zuliprc file was not found at {locations_checked}')}"
+        f"{missing_zuliprc_text}"


This will likely be simplified with the other changes I suggested?

neiljp · 2024-08-07T06:16:29Z

zulipterminal/cli/run.py

+    parser.add_argument(
+        "-n",
+        "--new-organization",
+        action="store_true",
+        help="login to a new organization",
+    )


It doesn't quite fit within the remit of the zuliprc movement work, but it'd be good to have -c work with creating a zuliprc file from username/password, which I don't think it does right now?

Created resolve_to_valid_path() to return a valid zuliprc path. Added docstring.

Previously, zuliprc_path was set directly in main(). Now, we are setting it 2 levels deeper, inside of login_and_save(), passing it to main() through resolve_to_valid_path(). main() now sets the default value of zuliprc_path to None, and uses the returned path from resolve_to_valid_path(). This is done to allow adding support for multiple zuliprc paths later, where we will dynamically compute the path in login_and_save(), after taking user input.

In addition to the previous default path ~/zuliprc. Introduce the .config path of zulip-terminal as a str constant. Add a function check_for_default_zuliprc() to search for zuliprc files in both the default locations, when the --config-file argument is not used. It is added to ensure that users who have only a single account can skip using the account alias argument that will be added. Docstring updated. Tests updated.

Newly generated zuliprc files were being saved to `~/zuliprc`. This moves default zuliprc creation into the user's config directory. The --config-file option will no longer be able to save zuliprc files in custom locations. The new config path including the .config directory are handled safely, to allow creation even if they do not already exist. Tests updated. The test with id: `path_does_not_exist`, in the test function `test_main_cannot_write_zuliprc_given_good_credentials()` has been removed, as the path will now be created even if it does not exist.

The usage of the account-alias positional argument is made mutually exclusive with the config-file option.

Lists all the accounts in the config path, by checking the directory structure. The home path zuliprc is not considered in this list. Tests updated.

Previously, we had to pass the zuliprc_path because the --config-file option could be used to create a zuliprc at a custom location. Now, all accounts are only saved to the standard config path. So, by moving the print statement to the outer function, we can simplify login_and_save().

- If --new-account is used along with --config-file, - If the given path is valid, that zuliprc will be loaded - Else, a new account will be created, but not at the given path. - If --new-account is used along with account-alias, the new account argument will be ignored. Tests updated.

zulipbot added the size: XL [Automatic label added by zulipbot] label May 21, 2024

Niloth-p force-pushed the config-zuliprc branch from 11d7e73 to 9a5dc7f Compare May 21, 2024 09:25

Niloth-p changed the title ~~Config zuliprc~~ Migration of zuliprc location to a config directory May 27, 2024

Niloth-p force-pushed the config-zuliprc branch from 9a5dc7f to ee18654 Compare June 11, 2024 05:48

Niloth-p commented Jun 11, 2024

View reviewed changes

Niloth-p marked this pull request as ready for review June 11, 2024 07:42

Niloth-p added area: config PR needs buddy review labels Jun 11, 2024

rsashank reviewed Jun 18, 2024

View reviewed changes

rsashank removed the PR needs buddy review label Jun 18, 2024

Niloth-p force-pushed the config-zuliprc branch from ee18654 to bc9df22 Compare June 22, 2024 21:54

Niloth-p added PR needs buddy review PR needs mentor review labels Jun 22, 2024

rsashank removed the PR needs buddy review label Jun 24, 2024

Niloth-p added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs mentor review labels Jun 28, 2024

neiljp reviewed Aug 7, 2024

View reviewed changes

Niloth-p added 12 commits August 13, 2024 00:20

refactor: run: Rename fetch_zuliprc function, and add docstring.

59828e4

refactor: run: Split parse_zuliprc() to create a separate function.

8d17aa9

Created resolve_to_valid_path() to return a valid zuliprc path. Added docstring.

refactor: run: Use a constant for the default zuliprc path.

1bde5fd

platform_code: Add a function that gets the path to XDG_CONFIG_HOME.

c0a0726

run: Add new positional argument account-alias to load config.

ffdd5ee

The usage of the account-alias positional argument is made mutually exclusive with the config-file option.

run: Add new argument --list-accounts.

330d6c9

Lists all the accounts in the config path, by checking the directory structure. The home path zuliprc is not considered in this list. Tests updated.

run: List all accounts when the user enters an invalid account alias.

27473c1

Niloth-p force-pushed the config-zuliprc branch from bc9df22 to 9681331 Compare August 24, 2024 05:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration of zuliprc location to a config directory #1503

Migration of zuliprc location to a config directory #1503

Niloth-p commented May 21, 2024 •

edited

Loading

Niloth-p left a comment •

edited

Loading

Niloth-p Jun 11, 2024

neiljp Aug 7, 2024

Niloth-p Jun 11, 2024

Niloth-p Jun 11, 2024

rsashank Jun 18, 2024

Niloth-p Jun 11, 2024

Niloth-p Jun 11, 2024

Niloth-p Jun 11, 2024

Niloth-p Jun 11, 2024

rsashank left a comment

rsashank Jun 18, 2024

rsashank Jun 18, 2024

rsashank Jun 18, 2024

rsashank Jun 18, 2024 •

edited

Loading

rsashank Jun 18, 2024

rsashank Jun 18, 2024 •

edited

Loading

rsashank commented Jun 24, 2024

neiljp left a comment

neiljp Aug 7, 2024

neiljp Aug 7, 2024

neiljp Aug 7, 2024

neiljp Aug 7, 2024

neiljp Aug 7, 2024

neiljp Aug 7, 2024

neiljp Aug 7, 2024

neiljp Aug 7, 2024

neiljp Aug 7, 2024

neiljp Aug 7, 2024

		@@ -370,32 +370,25 @@ def unreadable_dir(tmp_path: Path) -> Generator[Tuple[Path, Path], None, None]:
		unreadable_dir.chmod(0o755)


		@pytest.mark.parametrize(

		name: Install & test - CPython 3.7 (ubuntu), codecov
		name: Install & test - CPython 3.8 (ubuntu), codecov

		def downloads_file_path() -> Path:
		return Path(user_downloads_dir())

		@@ -452,6 +452,30 @@ def list_themes() -> str:
		)


		def path_to_realm_zuliprc(realm_name_prefix: str) -> str:

Migration of zuliprc location to a config directory #1503

Are you sure you want to change the base?

Migration of zuliprc location to a config directory #1503

Conversation

Niloth-p commented May 21, 2024 • edited Loading

What does this PR do, and why?

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Niloth-p left a comment • edited Loading

Choose a reason for hiding this comment

Reference: Testing cases:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsashank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsashank Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsashank Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

rsashank commented Jun 24, 2024

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niloth-p commented May 21, 2024 •

edited

Loading

Niloth-p left a comment •

edited

Loading

rsashank Jun 18, 2024 •

edited

Loading

rsashank Jun 18, 2024 •

edited

Loading