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

Feat/daemon selection #394

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Conversation

janepie
Copy link

@janepie janepie commented Sep 25, 2024

Resolves #276

I had to prolong the waiting time for the registering as (at least on my setup) it took 15-20 sec (also when done per occ command directly, I checked)

Did not comment the Vue parts as you don't seem to use comments there. If I should add comments just let me know!

@bigcat88 bigcat88 self-requested a review September 25, 2024 08:49
@bigcat88
Copy link
Member

Thanks for the pull request, it's good, but there are a couple of small inaccuracies that will be good to be fixed:

  1. Daemons with the type "manual-install" should not be displayed
  2. If there is only one Daemon (after filter by "manual-install"), the selection window should not appear
  3. When the selection window opens, you see the text "Selection Modal" at the top center - it should be removed (see on attached screen)

image

@janepie
Copy link
Author

janepie commented Sep 25, 2024

Aah yes, thank you, I will look into that tomorrow!

@janepie janepie force-pushed the feat/daemon-selection branch 2 times, most recently from 57a9ece to 23f79a2 Compare September 26, 2024 08:16
@janepie janepie marked this pull request as draft September 26, 2024 08:17
@janepie janepie force-pushed the feat/daemon-selection branch 2 times, most recently from 5988bcc to e3bd4f6 Compare September 26, 2024 18:01
@janepie janepie marked this pull request as ready for review September 26, 2024 18:05
@andrey18106
Copy link
Collaborator

It seems there are some syntax errors in new components (missing prop, error prop type check) in console while opening daemon selection - I don't see the list of daemons to choose from (I have one manual-install and one docker-install daemon).

@janepie
Copy link
Author

janepie commented Sep 27, 2024

Then you shouldn't see the list as it's only one valid daemon. Can you check with two docker-install?

Edit: ah damn, I see what you mean, will check

@janepie janepie marked this pull request as draft September 27, 2024 16:04
Signed-off-by: janepie <[email protected]>
Signed-off-by: janepie <[email protected]>
Signed-off-by: janepie <[email protected]>
@janepie
Copy link
Author

janepie commented Sep 27, 2024

Should be fixed, but something in my NC instance seems broken right now and I cannot validate. I will try to fix it and check, feel free to check yourself or wait :)

@janepie janepie marked this pull request as ready for review September 27, 2024 19:47
@janepie
Copy link
Author

janepie commented Sep 27, 2024

Works!

@bigcat88
Copy link
Member

Works!

Great, it almost works, there is a few last little details left.

When the application is disabled, the window for selecting a daemon to enable should not appear, since this window is for choosing where to install the application (on which daemon to deploy)
When enabling the application, we do not need to display it.

Steps to reproduce:

  1. Click "Deploy and Enable"
  2. Once the deploying is complete, click "Disable"
  3. After clicking "Enable", the ModalWindow should not appear and we just need to enable it

https://github.com/nextcloud/app_api/pull/394/files#diff-07f98afc574e5706d7ad885a1c359bf699dd1045b0cfcf54e325110c9f45a0aaR189

this string executes and make request to the server for each ExApp on these tabs(you can see this in Network tab in browser):

image image

Is there a way to fix this so we can consolidate PRs and consider all tasks complete and done?

@janepie
Copy link
Author

janepie commented Oct 1, 2024

Totally, both points done :)

Copy link
Member

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

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

Thank you for putting effort on this, great job.

@bigcat88
Copy link
Member

bigcat88 commented Oct 1, 2024

now it’s up to you, Andrey, when we will combine this and whether we will make adjustments on our part.

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.

Daemon selection for ExApp installation
3 participants