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

Hotfix/routine default file overwrite #4927

Merged

Conversation

Ltwo3five
Copy link
Contributor

@Ltwo3five Ltwo3five commented Apr 28, 2023

Description

- Summary of the change / bug fix.

Personal routines created on https://my.openbb.co/app/terminal/routines with the same name as default routines 
overwrite the default routine in the hubs folder. This was caused by the dictionary routines_dict in 
routines_handler.download_routines having the same key so the default script gets overwritten in the hubs folder. My 
solution  was to create two dictionaries one for personal routines and the other one for default routines and store them in 
a list. 
[{"A" : ["script","personal]}, {"B" : ["script","default"]}]
Another change I  made was to store personal routines and default routines in two separate folders in the hub folder. To 
do this, I put the routine script in a list as the first element, and the string "default" or "personal as the second element to 
denote their respective types. ["script", "personal/default"]
I made changes in call_download in the accounts controller to account for the changes in save routines 
and in download_and_save routines in session_model.py  to account for the changes in download_routines and      
save_routines in routines_handler.py. Lastly I made changes to read_routine in routines_handler.py to account for the 
change in directory structure so the files in their respective folders can still be found.

- Link to the issue

#4826

- Relevant motivation and context.

I think it would make more sense for there to be two folders in the hubs folder for easy navigation and to allow
users to locate files with the same names more easily.
personal routines created on the website with the same name as default routines are causing the latter to be overwritten
when both routines are downloaded from the server.

- dependencies

import walk from os

How has this been tested?

-Please describe the tests that you ran to verify your changes.

I redid the unit test for test_read_routines and test_read_routines_exception in test_routines_handler, and I updated the unit
tests for call_download in test_account_controller to account for the change in parameters for the save_routine mock.
I performed end to end test by testing the upload, download, list, and delete commands for routines. I also went to
https://my.openbb.co/app/terminal/routines and created a new personal routine with the same name as the default routine
to verify the changes have fixed the issue.
I made unit tests for download_routines in test_routines_handler. The first test tests if the function returns the correct object
the rest of the tests test for when downloading personal routines raises an exception, when downloading default routines
raises an exception and when downloading both personal and default routines raises an exception

-Provide instructions so we can reproduce.

Run relevant pytests and call commands listed above.
Go to the routines folder when logged into account
Go to https://my.openbb.co/app/terminal/routines and create a new personal routine with the same name as default

-Please also list any relevant details for your test configuration.

I redid test_read_routine and test_read_routine_exception to create new mocks for os.walk and .os.path.relpath and to
test the logic and parameter changes in the function. I also removed mocks that are not relevant.

  • Make sure affected commands still run in terminal
  • Ensure the SDK still works
  • Check any related reports

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.

jmaslek and others added 13 commits April 25, 2023 21:31
…-installer

hotfix/ fix windows pyinstaller subprocess fail if space in path
…personal routines in difference folders and changed logic in call_dowload in account controller to account for the change in logic for save_routine
…ry structure and also changed the corresponding unit tests. Slightly amended test_save_routine to account for the new routine param
…_with to account for the change in the input parameter of the save_routine function
@reviewpad reviewpad bot added the feat S Small T-Shirt size Feature label Apr 28, 2023
@jmaslek jmaslek requested a review from montezdesousa May 1, 2023 14:27
@montezdesousa montezdesousa linked an issue May 2, 2023 that may be closed by this pull request
@montezdesousa
Copy link
Contributor

montezdesousa commented May 2, 2023

Hi, thanks for the PR.

IIUC, if I create a routine with name already in use by default routines like The Influence of the Central Bank.openbb and run exe --file The Influence of the Central Bank.openbb it executes the default routine, right?

Did you consider how to run the personal routine with the same name? The simple way to achieve this would be to specify the full path
exe --file /Users/username/OpenBBUserData/routines/hub/personal/The Influence of the Central Bank.openbb, but does not work on my side. If this not easy to do in this PR we open a new issue (e.g. "Allow running routines from path") and take it elsewhere.

@Ltwo3five
Copy link
Contributor Author

Ltwo3five commented May 2, 2023

Hi, thanks for the PR.

IIUC, if I create a routine with name already in use by default routines like The Influence of the Central Bank.openbb and run exe --file The Influence of the Central Bank.openbb it executes the default routine, right?

Did you consider how to run the personal routine with the same name? The simple way to achieve this would be to specify the full path exe --file /Users/username/OpenBBUserData/routines/hub/personal/The Influence of the Central Bank.openbb, but does not work on my side. If this not easy to do in this PR we open a new issue (e.g. "Allow running routines from path") and take it elsewhere.

Hello , I missed that and did not take that the exe command after the user logs in into consideration. I know where the issue is and will push the changes when I am done.

…h --file default/filename.openbb, --file personal/filename.openbb or --file Users/Username/openBBUserData/Routines/hub/default/filename.openbb
@Ltwo3five
Copy link
Contributor Author

Ltwo3five commented May 2, 2023

Hi Monte, I have pushed the changes.
A user can now put in --file default/filename.openbb, --file personal/filename.openbb or --file user/username/openBBUserData/routines/~. Full path was working before, the issue was that C: has to be specified before /users. I have made a regex to account for when a user puts in C:/ at the beginning of the path and when they don't.

@montezdesousa
Copy link
Contributor

Hi Monte, I have pushed the changes. A user can now put in --file default/filename.openbb, --file personal/filename.openbb or --file user/username/openBBUserData/routines/~. Full path was working before, the issue was that C: has to be specified before /users. I have made a regex to account for when a user puts in C:/ at the beginning of the path and when they don't.

Hi, apparently exe --file /Users/username/OpenBBUserData/routines/hub/personal/The Influence of the Central Bank.openbb seems to work now if I remove the regex, I will write the suggestion in the review.

Otherwise, I get this (since I'm on mac). Windows user might not use C drive as well. So it's better to force user to specify that if needed.

Error: [Errno 2] No such file or directory: 'C:/Users/username/OpenBBUserData/routines/hub/personal/The Influence of the Central Bank.openbb'

file_path = " ".join(ns_parser.file)
path = self.ROUTINE_FILES.get(file_path, Path(file_path))
full_path = Path('C:/' + re.sub(r'^(C:\/?|c:\/?)', '', file_path).strip())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
full_path = Path('C:/' + re.sub(r'^(C:\/?|c:\/?)', '', file_path).strip())
full_path = file_path

Copy link
Contributor Author

@Ltwo3five Ltwo3five May 3, 2023

Choose a reason for hiding this comment

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

I am not sure why it is not working with the regex for you as it is the opposite for me and
exe --file /Users/username/OpenBBUserData/routines/hub/personal/The Influence of the Central Bank.openbb only works for me with the regex otherwise i need to include the C drive at the start for it to work without regex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I guess it is better to remove the regex as the full path with the C:/ at the start still works with or without it. The purpose of the regex was for users to be able to choose to include the C;/ or not include it and the path will still work but at the end of the day the issue seems trivial and it was simply for convenience

@@ -403,32 +405,32 @@ def call_download(self, other_args: List[str]):
# Default routine
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we should give priority to user routines if there's a naming conflict.

                name = " ".join(ns_parser.name)
                if name in self.REMOTE_CHOICES:
                    # User routine
                    response = Hub.download_routine(
                        auth_header=get_current_user().profile.get_auth_header(),
                        name=name,
                    )
                    data = [
                        (
                            response.json()
                            if response and response.status_code == 200
                            else None
                        ),
                        "personal",
                    ]
                else:
                    # Default routine
                    data = [
                        next(
                            (r for r in self.DEFAULT_ROUTINES if r["name"] == name),
                            None,
                        ),
                        "default",
                    ]

@reviewpad reviewpad bot added feat M Medium T-Shirt size feature and removed feat S Small T-Shirt size Feature labels May 3, 2023
@montezdesousa
Copy link
Contributor

montezdesousa commented May 3, 2023

I've made a couple suggestions and fixed linting, other than that looks good to me!

@Ltwo3five
Copy link
Contributor Author

I've made a couple suggestions and fixed linting, other than that looks good to me!

Thank you!

@montezdesousa montezdesousa self-requested a review May 5, 2023 11:13
@montezdesousa montezdesousa changed the base branch from main to develop May 5, 2023 12:55
@jmaslek jmaslek enabled auto-merge May 5, 2023 12:59
@jmaslek jmaslek added this pull request to the merge queue May 5, 2023
Merged via the queue into OpenBB-finance:develop with commit 1f0c3ff May 5, 2023
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat M Medium T-Shirt size feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Personal routines overwrite default if name collision
4 participants