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

Version 3 Rewrite #31

Merged
merged 46 commits into from
Jan 2, 2022
Merged

Version 3 Rewrite #31

merged 46 commits into from
Jan 2, 2022

Conversation

JonathanTreffler
Copy link
Owner

@JonathanTreffler JonathanTreffler commented Oct 27, 2020

implements #21

Todo

  • Implement boilerplate code for sidebar

  • implement sidebar loader

  • call Sidebar loader

  • install necessary dependencies (vuejs, webpack)

  • add webpack configuration

  • add build scripts

  • Add functionality to sidebar

  • Styling

  • Final Testing

@JonathanTreffler JonathanTreffler added the enhancement New feature or request label Oct 27, 2020
@JonathanTreffler JonathanTreffler self-assigned this Oct 27, 2020
@JonathanTreffler JonathanTreffler mentioned this pull request Oct 27, 2020
@DecaTec
Copy link
Collaborator

DecaTec commented Jun 30, 2021

Can we merge this or should we wait until all the todos are done?

@JonathanTreffler
Copy link
Owner Author

Since it is not ready to be published to the app store, i think we should keep it in this branch and keep master stable.

@DecaTec
Copy link
Collaborator

DecaTec commented Jun 30, 2021

OK, then we don't merge (yet).

Have you received my mail? 😉

@JonathanTreffler JonathanTreffler linked an issue Jul 8, 2021 that may be closed by this pull request
@gaellafond
Copy link

I can confirm what RiCkYB-667 said: the latest v3 commit is not working for me either. My old modified version is still working, so I can use it to compare with the v3 branch to see what is different. I will spend some time on it tomorrow and hopefully find out what need to be done to make it work with NC 21.

@RiCkYB-667
Copy link

RiCkYB-667 commented Sep 1, 2021

I can confirm what RiCkYB-667 said: the latest v3 commit is not working for me either. My old modified version is still working, so I can use it to compare with the v3 branch to see what is different. I will spend some time on it tomorrow and hopefully find out what need to be done to make it work with NC 21.

Hey there @gaellafond !
Did you by any chance manage to find the errors?
It would be nice to know what's up! ;)
Would really love to be able to rename those pesky links...
Also should you be able to share your working version until this finalizes, i would be grateful!

Thank you!

@gaellafond
Copy link

gaellafond commented Sep 23, 2021

@RiCkYB-667 Sorry for late reply. I'm currently spending some time looking at it. I'm comparing my working version with the latest commit from this branch, but I don't quite understand what prevent this one from working. The structure is very different, but the code looks the same. I believe the code needs to be compiled in a very specific way and I don't know the process.

The sharerenamer app have another big issue for us. The shares are limited to 32 characters due to DB field limitation. We could increase the field size, but changing the DB schema is risky. Future NextCloud updates could revert the schema, which would cause shared links to fail.

We are planning to implement our own app. The idea, at this point, is to create some sort of "Proxy". Instead of changing the share in the DB, we will add a public API which redirect the named link to the shared hash URL.

@JonathanTreffler
Copy link
Owner Author

@RiCkYB-667 Sorry for late reply. I'm currently spending some time looking at it. I'm comparing my working version with the latest commit from this branch, but I don't quite understand what prevent this one from working. The structure is very different, but the code looks the same. I believe the code needs to be compiled in a very specific way and I don't know the process.

The compilation process Is quite simple. Just run:
npm ci
and then
npm run build

Here's a compiled version of the latest commit of this PR ready to go: sharerenamer.zip

That is probably the reason why the testing on NC21 didn't work for anyone but me. The last zip i provided you all for testing was compiled, but after @gaellafond 's testing i didn't create another compiled release.

@gaellafond
Copy link

Thank you very much @JonathanTreffler . I must be doing something wrong. I pull the latest version of the project and re-compiled it / redeployed it again this morning, but still no success. I tried your pre-compiled version and it's working. I compared the version I compiled (found in sharerenamer/build/artifacts/sign/sharerenamer) with the version you provided and they are completely different. Pretty much none of the files in your build are in my build.

I assumed the compiler was not actually doing anything so I manually deleted the build folder and recompiled it. It didn't trigger any compile error but the build folder was not recreated.

So, I still don't understand the build process. What is the "npm run build" supposed to do? Where should the compiled version be after compilation? Where is the "build" folder coming from? (It's not on Git, so I assume it was created from one of my numerous attempts at building the project...)

I assume most of those questions are standard knowledge for NextCloud, or even npm. I have very little experience with those at the moment. I need deeper explanations.

@gaellafond
Copy link

gaellafond commented Sep 24, 2021

I tried to run "make all" but it ended with an error:
make: *** No rule to make target 'test', needed by 'all'. Stop.

I edited the Makefile. There is indeed no "test" defined in the file, so I assume the "all" goal is not part of the build process otherwise you would have fixed it. Nonetheless, I removed the "test" at the end of the "all" goal and re-run the "make all" command. It completed without errors, but the build folder is still not re-created.

@gaellafond
Copy link

gaellafond commented Sep 24, 2021

I managed to get it to work. Here is what I did:

$ git clone https://github.com/JonathanTreffler/sharerenamer.git
$ cd sharerenamer/
$ git checkout v3
$ npm ci
$ npm run build
$ rm -R node_modules

Then I deploy the whole project folder to NextCloud apps folder.

Usually, when you compile a project, it generates a deployable artifact. I was not expecting to have to deploy the whole project. I deleted the node_modules folder because it was not in the build you provided, and because it was very big (~150MB).

My artifact do not have the "vendor" folder yours have. I assume that's not important.

@JonathanTreffler
Copy link
Owner Author

My artifact do not have the "vendor" folder yours have. I assume that's not important.

Yes, i just did a fast release for testing. For an actual release the vendor folder would be removed (because nextcloud will download the libraries in it automatically).

@JonathanTreffler
Copy link
Owner Author

I managed to get it to work. Here is what I did:

$ git clone https://github.com/JonathanTreffler/sharerenamer.git
$ cd sharerenamer/
$ git checkout v3
$ npm ci
$ npm run build
$ rm -R node_modules

Then I deploy the whole project folder to NextCloud apps folder.

Usually, when you compile a project, it generates a deployable artifact. I was not expecting to have to deploy the whole project. I deleted the node_modules folder because it was not in the build you provided, and because it was very big (~150MB).

That is exactly the correct way :)

@JonathanTreffler
Copy link
Owner Author

I probably never adjusted the Makefile for v3. I will look into it.

@JonathanTreffler
Copy link
Owner Author

Usually, when you compile a project, it generates a deployable artifact. I was not expecting to have to deploy the whole project.

Yes it compiles the js and vue files from src with the libraries from node_modules (installed by npm ci) and outputs to js/. It doesn't do anything with the php code, so you need to deploy the full folder structure.

@gaellafond
Copy link

Thanks @JonathanTreffler for taking the time to answer all my questions.
In the master branch, it creates a .tar.gz file in the build folder then it sign the archive using a private key (which is obviously not in the project sources). At least, that's how I understand the Makefile. Is that an old practice for old version of NextCloud? Will you have to implement something similar for NextCloud 20+?

@JonathanTreffler
Copy link
Owner Author

JonathanTreffler commented Sep 24, 2021

I will need to figure that out. It is still the principle, but lately everything has been automated with github actions. I will need to get a new key for the appstore (because of the owner transfer), which should not be a problem.

@JonathanTreffler
Copy link
Owner Author

Finally all checks pass, so this can be merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comply to NC 18+
6 participants