Deleted bookmarks app #3663

Jigar3 · 2018-02-23T14:32:14Z

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

humitos

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!

humitos · 2018-02-23T14:46:07Z

readthedocs/gold/migrations/0002_auto_20180217_1121.py

+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('gold', '0001_initial'),


Please, remove these migration files that were created.

Jigar3 · 2018-02-23T16:16:21Z

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

Jigar3 · 2018-02-23T20:01:52Z

@humitos Done as requested

stsewd · 2018-02-23T20:03:09Z

docs/locale/pt_BR/LC_MESSAGES/api/builds.po

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


I think this file should stay

stsewd · 2018-02-23T20:03:34Z

docs/locale/pt_BR/LC_MESSAGES/api/core.po

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


This one too

stsewd · 2018-02-23T20:06:46Z

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

humitos · 2018-02-24T01:56:17Z

readthedocs/core/static-src/core/js/doc-embed/footer.js

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

-
-function setupBookmarkCSRFToken() {


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

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

Jigar3 · 2018-02-24T01:59:10Z

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

humitos · 2018-02-24T02:00:31Z

@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 · 2018-02-25T06:39:51Z

@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 · 2018-02-26T00:46:08Z

@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 · 2018-02-26T03:51:57Z

@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 · 2018-02-27T07:47:44Z

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 · 2018-02-27T07:56:33Z

Blocking on #3691

humitos · 2018-04-17T20:44:18Z

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

ericholscher · 2018-04-18T08:53:13Z

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.

stsewd · 2018-04-18T09:16:49Z

readthedocs/redirects/migrations/0002_auto_20180217_1121.py

+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):


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

ericholscher · 2018-05-01T00:20:40Z

Thanks!

ericholscher · 2018-05-01T00:22:45Z

Removed the migrations here locally and then merged. 👍

deleted bookmarks app

ab55c92

RichardLitt added the PR: ready for review label Feb 23, 2018

humitos requested changes Feb 23, 2018

View reviewed changes

humitos added PR: work in progress Pull request is not ready for full review and removed PR: ready for review labels Feb 23, 2018

Jigar3 added 2 commits February 24, 2018 01:27

Cleaned up remaining code

7239f43

Merge branch 'master' into develop

dd0cc9a

stsewd reviewed Feb 23, 2018

View reviewed changes

docs/locale/pt_BR/LC_MESSAGES/api/builds.po Outdated

@@ -1,91 +0,0 @@

# SOME DESCRIPTIVE TITLE.

Copy link

Member

stsewd Feb 23, 2018

Choose a reason for hiding this comment

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

I think this file should stay

stsewd reviewed Feb 23, 2018

View reviewed changes

docs/locale/pt_BR/LC_MESSAGES/api/core.po Outdated

@@ -1,216 +0,0 @@

# SOME DESCRIPTIVE TITLE.

Copy link

Member

stsewd Feb 23, 2018

Choose a reason for hiding this comment

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

This one too

Done requested changes

c77078b

humitos reviewed Feb 24, 2018

View reviewed changes

agjohnson added the Status: blocked Issue is blocked on another issue label Feb 27, 2018

ericholscher added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Mar 1, 2018

ericholscher removed the Status: blocked Issue is blocked on another issue label Apr 18, 2018

Merge branch 'master' into develop

a4f9d63

stsewd reviewed Apr 18, 2018

View reviewed changes

davidfischer mentioned this pull request Apr 18, 2018

Fix bookmark app lint failures #3969

Merged

ericholscher merged commit a4f9d63 into readthedocs:master May 1, 2018

stsewd mentioned this pull request May 8, 2018

Remove the 'bookmarks' app completely #3659

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deleted bookmarks app #3663

Deleted bookmarks app #3663

Jigar3 commented Feb 23, 2018

humitos left a comment

humitos Feb 23, 2018

Jigar3 commented Feb 23, 2018

Jigar3 commented Feb 23, 2018

stsewd Feb 23, 2018

stsewd Feb 23, 2018

stsewd commented Feb 23, 2018 •

edited

Loading

humitos Feb 24, 2018

agjohnson Feb 27, 2018

Jigar3 commented Feb 24, 2018

humitos commented Feb 24, 2018

Jigar3 commented Feb 25, 2018

humitos commented Feb 26, 2018

Jigar3 commented Feb 26, 2018 •

edited

Loading

agjohnson commented Feb 27, 2018

agjohnson commented Feb 27, 2018

humitos commented Apr 17, 2018

ericholscher commented Apr 18, 2018

stsewd Apr 18, 2018

ericholscher commented May 1, 2018

ericholscher commented May 1, 2018

		from django.db import migrations, models


		class Migration(migrations.Migration):

Deleted bookmarks app #3663

Deleted bookmarks app #3663

Conversation

Jigar3 commented Feb 23, 2018

humitos left a comment

Choose a reason for hiding this comment

humitos Feb 23, 2018

Choose a reason for hiding this comment

Jigar3 commented Feb 23, 2018

Jigar3 commented Feb 23, 2018

stsewd Feb 23, 2018

Choose a reason for hiding this comment

stsewd Feb 23, 2018

Choose a reason for hiding this comment

stsewd commented Feb 23, 2018 • edited Loading

humitos Feb 24, 2018

Choose a reason for hiding this comment

agjohnson Feb 27, 2018

Choose a reason for hiding this comment

Jigar3 commented Feb 24, 2018

humitos commented Feb 24, 2018

Jigar3 commented Feb 25, 2018

humitos commented Feb 26, 2018

Jigar3 commented Feb 26, 2018 • edited Loading

agjohnson commented Feb 27, 2018

agjohnson commented Feb 27, 2018

humitos commented Apr 17, 2018

ericholscher commented Apr 18, 2018

stsewd Apr 18, 2018

Choose a reason for hiding this comment

ericholscher commented May 1, 2018

ericholscher commented May 1, 2018

stsewd commented Feb 23, 2018 •

edited

Loading

Jigar3 commented Feb 26, 2018 •

edited

Loading