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

Deleted bookmarks app #3663

Merged
merged 5 commits into from May 1, 2018
Merged

Deleted bookmarks app #3663

merged 5 commits into from May 1, 2018

Conversation

@Jigar3
Copy link
Contributor

@Jigar3 Jigar3 commented Feb 23, 2018

This is with regards to #3659 .
Please correct me if I made any mistakes while deleting the app.

Copy link
Member

@humitos humitos left a comment

Thanks for this PR! Look good!

I found there are some more ophaned things related to the bookmark apps:

  • bookmark_list_detailed.html needs to be removed also
  • remove from prospector file
  • remove from docs index
  • in footer.js there is some references that I'm not sure if we need to remove them

Besides, I left a comment to not create those migrations files (there is a PR for that at #3608)

Please, make those changes and I'd say that we will be ready to merge. Thanks for your contribution!

class Migration(migrations.Migration):

dependencies = [
('gold', '0001_initial'),
Copy link
Member

@humitos humitos Feb 23, 2018

Please, remove these migration files that were created.

@Jigar3
Copy link
Contributor Author

@Jigar3 Jigar3 commented Feb 23, 2018

Thanks for replying this fast @humitos.
I will do the recommended changes.

@Jigar3
Copy link
Contributor Author

@Jigar3 Jigar3 commented Feb 23, 2018

@humitos Done as requested

@@ -1,91 +0,0 @@
# SOME DESCRIPTIVE TITLE.
Copy link
Member

@stsewd stsewd Feb 23, 2018

I think this file should stay

@@ -1,216 +0,0 @@
# SOME DESCRIPTIVE TITLE.
Copy link
Member

@stsewd stsewd Feb 23, 2018

This one too

@stsewd
Copy link
Member

@stsewd stsewd commented Feb 23, 2018

@Jigar3 There are like 6 .po files that shouldn't be deleted. And take a look to @humitos comment about migrations.

@@ -92,23 +92,6 @@ function injectFooter(data) {
}
}


function setupBookmarkCSRFToken() {
Copy link
Member

@humitos humitos Feb 24, 2018

Since we are touching this .js file, I think we need to call gulp build and add the file generated (I'm not sure though).

In any case, if you don't know how to do it, let me know and I can check this by myself before merging: http://docs.readthedocs.io/en/latest/development/standards.html#getting-started

Copy link
Contributor

@agjohnson agjohnson Feb 27, 2018

Correct, you'll need to regenerate the built assets with this PR.

@Jigar3
Copy link
Contributor Author

@Jigar3 Jigar3 commented Feb 24, 2018

I don't know how to do it but I am willing to learn.

@humitos
Copy link
Member

@humitos humitos commented Feb 24, 2018

@Jigar3 Nice! Please, read the guide that I linked in my comment and let me know if you get block on some of those steps.

@Jigar3
Copy link
Contributor Author

@Jigar3 Jigar3 commented Feb 25, 2018

@humitos While running bower install I am getting the

bower jquery#2.0.3                    ENOTFOUND Package jquery not found

error. While running gulp build I get three browserify errors.
Can you help?

@humitos
Copy link
Member

@humitos humitos commented Feb 26, 2018

@Jigar3 I will need you to share more context so I can help you because that output is not enought for me. What commands are you running? What are they output?

@Jigar3
Copy link
Contributor Author

@Jigar3 Jigar3 commented Feb 26, 2018

@humitos As mentioned in the article you shared here.
To install to Front end dependencies I ran the command

bower install

That gave me the following output.

bower not-cached    https://github.com/agjohnson/readthedocs-client-js.git#*
bower resolve       https://github.com/agjohnson/readthedocs-client-js.git#*
bower cached        https://github.com/snide/sphinx-rtd-theme.git#0.2.5b1
bower validate      0.2.5b1 against https://github.com/snide/sphinx-rtd-theme.git#0.2.5b1
bower download      https://github.com/agjohnson/readthedocs-client-js/archive/master.tar.gz
bower extract       readthedocs-client#* archive.tar.gz
bower resolved      https://github.com/agjohnson/readthedocs-client-js.git#58c57a475c
bower ENOTFOUND     Package jquery.payment not found

I then went ahead and tried the command.

gulp build

That gave me following errors

[09:20:02] Using gulpfile ~/rtd/checkouts/readthedocs.org/gulpfile.js
[09:20:02] Starting 'build'...
[09:20:02] Building source files
(node:2399) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
[09:20:11] Browserify error: could not resolve dependency readthedocs-client : bower returns the module as known but not found (did you forget to run bower install ?) (/home/jsc39/rtd/checkouts/readthedocs.org/readthedocs/projects/static-src/projects/js/tools.js) while parsing file: /home/jsc39/rtd/checkouts/readthedocs.org/readthedocs/projects/static-src/projects/js/tools.js
[09:20:12] Browserify error: could not resolve dependency sphinx-rtd-theme : bower returns the module as known but not found (did you forget to run bower install ?) (/home/jsc39/rtd/checkouts/readthedocs.org/readthedocs/core/static-src/core/js/doc-embed/sphinx.js) while parsing file: /home/jsc39/rtd/checkouts/readthedocs.org/readthedocs/core/static-src/core/js/doc-embed/sphinx.js
[09:20:12] Browserify error: could not resolve dependency xss : bower returns the module as known but not found (did you forget to run bower install ?) (/home/jsc39/rtd/checkouts/readthedocs.org/readthedocs/core/static-src/core/js/doc-embed/search.js) while parsing file: /home/jsc39/rtd/checkouts/readthedocs.org/readthedocs/core/static-src/core/js/doc-embed/search.js
[09:20:13] Browserify error: could not resolve dependency jquery.payment : bower returns the module as known but not found (did you forget to run bower install ?) (/home/jsc39/rtd/checkouts/readthedocs.org/readthedocs/payments/static-src/payments/js/base.js) while parsing file: /home/jsc39/rtd/checkouts/readthedocs.org/readthedocs/payments/static-src/payments/js/base.js

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Feb 27, 2018

You have the commands correct, however there is a fun problem happening in the JS community right now: maintainers are removing support for bower and are removing their packages from the bower repository.

A quick fix would be to pull these dependencies from npm instead. If not, I had started a replacement for our gulp + bower build system, using webpack + npm. We're blocked on even building our assets until we figure this out. I'll open a ticket to track work there, but will set this to blocking for now.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Feb 27, 2018

Blocking on #3691

@humitos
Copy link
Member

@humitos humitos commented Apr 17, 2018

@Jigar3 can you try rebuilding the javascript files again? It seems to be fixed now.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 18, 2018

Believe this should be good. I believe during deploy we need to run:

django-admin migrate bookmarks zero --fake

I'm not sure if we need to run this before or after the app is deleted. Will have to see.

from django.db import migrations, models


class Migration(migrations.Migration):
Copy link
Member

@stsewd stsewd Apr 18, 2018

I think this migration isn't really necessary here and the other one I'm not sure

@ericholscher ericholscher merged commit a4f9d63 into readthedocs:master May 1, 2018
1 check passed
@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 1, 2018

Thanks!

@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 1, 2018

Removed the migrations here locally and then merged. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants