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

Launch Add new repository when Add existing repository fails/is not initialized. #1816

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

Conversation

SAMAD101
Copy link
Collaborator

@SAMAD101 SAMAD101 commented Sep 9, 2023

Description

The Add new window window will automatically pop up when an existing repository fails/is not initialized.
#1799

Motivation and Context

This will reduce confusion in adding new and adding existing repositories. Will fill some entries automatically when the add new repository window is opened through this route.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Sep 9, 2023

This PR is not complete yet. The problems I'm facing right now are:

  • When the add new window comes up through this route, the entries are filled, but when clicked on Add, the URL appears to be invalid. You have to reselect the repo (which is to be initialized)
  • Even if added, the window appears to have no current_profile.id
    (src/vorta/store/models.py/BackupProfileMixin)
 return BackupProfileModel.get(id=self.window().current_profile.id)

I was wondering why this is happening?

Copy link
Collaborator

@real-yfprojects real-yfprojects left a comment

Choose a reason for hiding this comment

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

You already did very well implementing this although I would suggest a little bit of restructuring.

  1. Implement a from_values method for the repo winows.
  2. Use a QMessageBox to inform the user about the failure and ask them whether they want to create a new repository instead.
  3. When the user confirms, call a method on RepoTab that opens AddRepoWindow using the from_values method.

This will make the user experience more clear and the code more modular.

@real-yfprojects
Copy link
Collaborator

  • When the add new window comes up through this route, the entries are filled, but when clicked on Add, the URL appears to be invalid. You have to reselect the repo (which is to be initialized)

This due to the is_remote_repo attribute. It is checked here:

if self.is_remote_repo and not re.match(r'.+:.+', self.values['repo_url']):
self._set_status(self.tr('Please enter a valid repo URL or select a local path.'))
return False

When using the file selector it is set to False:

self.is_remote_repo = False

@SAMAD101
Copy link
Collaborator Author

You already did very well implementing this although I would suggest a little bit of restructuring.

1. Implement a `from_values` method for  the repo winows.

2. Use a `QMessageBox` to inform the user about the failure and ask them whether they want to create a new repository instead.

3. When the user confirms, call a method on `RepoTab` that opens `AddRepoWindow` using the `from_values` method.

This will make the user experience more clear and the code more modular.

I've added the QMessageBox. I don't quite understand what you mean by the from_values method of RepoTab.
I've also tried launching the AddRepoWindow by using the new_repo() method of RepoTab, but the problem I faced was that importing RepoTab was giving this error:

from vorta.views.repo_tab import RepoTab

ImportError: cannot import name 'RepoTab' from partially initialized module 'vorta.views.repo_tab' (most likely due to a circular import) (/home/sam/Codes/vorta/src/vorta/views/repo_tab.py)

@real-yfprojects
Copy link
Collaborator

You already did very well implementing this although I would suggest a little bit of restructuring.

1. Implement a `from_values` method for  the repo winows.

2. Use a `QMessageBox` to inform the user about the failure and ask them whether they want to create a new repository instead.

3. When the user confirms, call a method on `RepoTab` that opens `AddRepoWindow` using the `from_values` method.

This will make the user experience more clear and the code more modular.

I've added the QMessageBox. I don't quite understand what you mean by the from_values method of RepoTab. I've also tried launching the AddRepoWindow by using the new_repo() method of RepoTab, but the problem I faced was that importing RepoTab was giving this error:

from vorta.views.repo_tab import RepoTab

ImportError: cannot import name 'RepoTab' from partially initialized module 'vorta.views.repo_tab' (most likely due to a circular import) (/home/sam/Codes/vorta/src/vorta/views/repo_tab.py)

If you only import vorta.views.repo_tab and reference the class later through repo_tab.RepoTab it works. However you shouldn't need to use the RepoTab class inside repo_add_dialog.py.

The add dialogs already have a values method returning all relevant data entered in the dialog. It would make sense to add a (possibly static) from_values method expecting those values. The from_values method would then initialize the dialog with the values provided.

self.exception_handler()

def exception_handler(self):
exception_window = QMessageBox.question(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't answer be a better name for this variable?

…ting setting of values from the values method
@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Sep 15, 2023

@real-yfprojects I've implemented the setting of values from the values method. However, for some reason, the repo addition is still failing for some reason.
Its just saying Unable to add your repository. Just have to fix this now.

@SAMAD101
Copy link
Collaborator Author

@real-yfprojects I've made the necessary changes.

SAMAD101 and others added 5 commits September 20, 2023 17:02
This improves the code that open the `AddRepoWindow` when one tries to add an existing repo although it doesn't exist.

* src/vorta/views/repo_add_dialog.py
This allows accessing `msgid` from `ExistingRepoWindow`.

* src/vorta/application.py
* src/vorta/borg/borg_job.py
* src/vorta/views/repo_add_dialog.py
* tests/unit/test_repo.py
@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Oct 20, 2023

wow, that simplifies a lot of things, the 'confirm password' field is not auto-filled. And, the repo is still unable to be added for some reason.

@real-yfprojects
Copy link
Collaborator

the 'confirm password' field is not auto-filled

I implemented that on purpose.

he repo is still unable to be added for some reason

You are right. Fixed that in the following commit.

Now when the `AddRepoWindow` with the values from `ExistingRepoWindow` the `is_remote_repo` attribute will be restored,
preventing an error message when trying to add the repository.

* src/vorta/views/repo_add_dialog.py
@m3nu
Copy link
Contributor

m3nu commented Oct 20, 2023

Looks like all macOS tests are timing out on Github today. Likely unrelated to this PR, seeing it elsewhere too.

@real-yfprojects
Copy link
Collaborator

@m3nu ping

@m3nu
Copy link
Contributor

m3nu commented Jan 10, 2024

This is what I get when adding an empty repo as existing repo. There is no change in dialog.

Screenshot 2024-01-10 at 11 19 49

@SAMAD101
Copy link
Collaborator Author

This is what I get when adding an empty repo as existing repo. There is no change in dialog.

@m3nu Works fine on mine. When you try to add an empty directory which is not a repository already. It will simply ask you to make it into one. Maybe, you're not running the same branch issue1799.
Here's a video:

untitled.mp4

@m3nu
Copy link
Contributor

m3nu commented Jan 11, 2024

I tried with a remote repo. Maybe that's the reason?

Should it also work for an empty remote repo?

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Jan 12, 2024

I tried with a remote repo. Maybe that's the reason?

Should it also work for an empty remote repo?

that's odd, the from_values function should deal with (empty) remote repos too. Lemme check once again, will fix this behavior soon

Comment on lines 280 to 282
if 'msgid' in error and error['msgid'] in ['Repository.DoesNotExist', 'Repository.InvalidRepository']:
self.init_new_repo()
break
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@real-yfprojects need some help here.
It seems like this condition is not turning True in case of uninitiated (empty) remote repos, while it is working fine for local repos. What could be going wrong here?
And due to this, the new init_new_repo method is never called.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure. E.g. by using the cmd, can you find out what the command outputs as an error/warning. Use the json flag so that you can find out which msgid is used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, for remote repository it was just InvalidRepository
So now it works for both local and remote repositories.

@real-yfprojects
Copy link
Collaborator

@m3nu Can you check again?

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