Skip to content
This repository has been archived by the owner on Apr 17, 2018. It is now read-only.

Add the ability to submit links. Fixes #548 #575

Merged
merged 4 commits into from
Sep 27, 2016
Merged

Conversation

jglamine
Copy link
Contributor

@jglamine jglamine commented Jul 18, 2016

This turned out to be non-trivial. I'm not sure what tricycle's release process is like, but someone other than me should probably do some additional regression testing.

I suggest using this link to view the diff while ignoring whitespace changes: https://github.com/tricycle/lesswrong/pull/575/files?w=1

@jglamine
Copy link
Contributor Author

home

Link posts are automatically prefixed with "[Link]". There is a new "Submit a new link" button on the sidebar.

@jglamine
Copy link
Contributor Author

submit-post
The submit page has two tabs.

@jglamine
Copy link
Contributor Author

submit-link

@jglamine
Copy link
Contributor Author

jglamine commented Jul 18, 2016

link-page
Clicking on "comments" brings you to this page. The title links to the actual link url.

@jglamine
Copy link
Contributor Author

edit-link
The edit link page allows you to change the title but not the url.

@wezm
Copy link
Contributor

wezm commented Jul 19, 2016

I would be great if you could take some time to increase your confidence that this won't break the site. At the very least there is a manual test script that goes through the core activities of the system:

https://github.com/tricycle/lesswrong/wiki/Hacking-on-Less-Wrong#manual-tests

Additionally, since this changes the link model, which is the core model of the application it will be worth poking at all the places links and comments are shown such as the main listings, sidebar, RSS feeds, user messages, the front page, user profile page listings.

Wesley Moore
Senior Developer
ph: +61 3 9024 2760
TrikeApps http://trikeapps.com/

On 18 Jul 2016, at 12:30 PM, James Lamine notifications@github.com wrote:

This turned out to be non-trivial. I'm not sure what tricycle's release process is like, but someone other than me should probably do some additional regression testing.

You can view, comment on, or merge this pull request online at:

#575 #575
Commit Summary

Add the ability to submit links. Fixes #548
File Changes

M r2/r2/controllers/api.py https://github.com/tricycle/lesswrong/pull/575/files#diff-0 (82)
M r2/r2/controllers/front.py https://github.com/tricycle/lesswrong/pull/575/files#diff-1 (12)
M r2/r2/lib/menus.py https://github.com/tricycle/lesswrong/pull/575/files#diff-2 (9)
M r2/r2/lib/pages/pages.py https://github.com/tricycle/lesswrong/pull/575/files#diff-3 (65)
A r2/r2/lib/tabs.py https://github.com/tricycle/lesswrong/pull/575/files#diff-4 (7)
M r2/r2/models/link.py https://github.com/tricycle/lesswrong/pull/575/files#diff-5 (110)
M r2/r2/public/static/main.css https://github.com/tricycle/lesswrong/pull/575/files#diff-6 (60)
M r2/r2/public/static/utils.js https://github.com/tricycle/lesswrong/pull/575/files#diff-7 (26)
M r2/r2/templates/link.html https://github.com/tricycle/lesswrong/pull/575/files#diff-8 (57)
M r2/r2/templates/newlink.html https://github.com/tricycle/lesswrong/pull/575/files#diff-9 (127)
A r2/r2/templates/tabs.html https://github.com/tricycle/lesswrong/pull/575/files#diff-10 (18)
M r2/test.ini https://github.com/tricycle/lesswrong/pull/575/files#diff-11 (29)
Patch Links:

https://github.com/tricycle/lesswrong/pull/575.patch https://github.com/tricycle/lesswrong/pull/575.patch
https://github.com/tricycle/lesswrong/pull/575.diff https://github.com/tricycle/lesswrong/pull/575.diff

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub #575, or mute the thread https://github.com/notifications/unsubscribe-auth/AABVG-Hmu1nUNHVvqvaQR7v0u4uty4BRks5qWuVigaJpZM4JOX9D.

@jglamine
Copy link
Contributor Author

Good idea, will do.

@jglamine
Copy link
Contributor Author

jglamine commented Aug 8, 2016

I fixed a few issues with rss feeds, side-bar, and front page links not being correct (linking to the url instead of the lesswrong post).

I ran through the manual tests.

This should be good to go.

@wezm
Copy link
Contributor

wezm commented Aug 16, 2016

I have taken some time to review this and have identified a few things that need addressing:

  • If you attempt to submit a link but get the CAPTCHA wrong (shown to low karma users), then resubmit with the correct CAPTCHA the submission ends up as an article instead of link.
  • If you submit a link that has already been submitted you are shown a little flash message (pictured) that states you can resubmit it, clicking this does not work. screen shot 2016-08-12 at 4 00 40 pm
  • The size of the editor on the article view is a lot smaller than it is on the live site. I think the new size is unsuitable for writing a long article.
  • Inline validations no longer work. On the live site if you submit the article form with nothing filled in you get an error against the title indicating this it is required. Additionally similar errors should be shown on the link submission form.
  • Articles are referred to as articles in most places on this site so can you change the label of the tab on the submission page from, "Text Article", to "Article".

@jglamine
Copy link
Contributor Author

Thank for catching these issues. I'll reproduce them and update the PR with fixes.

@jglamine
Copy link
Contributor Author

jglamine commented Sep 6, 2016

I was able to reproduce your bugs. Two of them were a bit tricky to reproduce. The do not appear to have anything to do with captchas. Here are the steps:

Bug 1 - Links turn into articles:

  1. Click "Create new article"
  2. Switch to the link tab.
  3. Fill in info, submit.

Expected behavior: brings you to the page for a newly created link
Actual behavior: brings you to the page for a newly created article

Bug 2 - article editor is too small:

  1. Click "Submit a new link"
  2. Switch to the article tab

Expected behavior: the text editor is 500px tall.
Actual behavior: the text editor is 84px tall.

@jglamine
Copy link
Contributor Author

jglamine commented Sep 6, 2016

PR updated.

I was not able to reproduce the "That link has already been submitted." flash message. If it still happens, please provide me with steps to reproduce and let me know what browser you're using.

Changes are as follows:

  • Fixed inline validation on article submission page.
    • Due to how the validation code is written, only one new error is displayed per submission of the form. So if you're missing both URL and title, it will initially only show a message next to the url.
  • Changed title of the article tab to "Article"
  • Prevent article editor from being too small when switching tabs.
  • Prevent links from turning into articles when switching tabs.

When you get a chance, I'd love to get your feedback on this.

@jglamine
Copy link
Contributor Author

jglamine commented Sep 6, 2016

Also note that there is not much validation around URLs. For example, links don't have to be valid urls. I don't think it's that important because people won't upvote broken links. If it turns out we really want better validation, we can always do it in another issue.

Also note that it's not possible to edit a link after submitting it, only the title. If we want admins to be able to edit links, we can do that as a separate issue. They can still delete them or edit the title, which should be enough for now.

@wezm
Copy link
Contributor

wezm commented Sep 27, 2016

I was not able to reproduce the "That link has already been submitted." flash message. If it still happens, please provide me with steps to reproduce and let me know what browser you're using.

I think this was only being shown due to the inline validation not working. Now that it is fixed, it doesn't show up for me either.

@wezm
Copy link
Contributor

wezm commented Sep 27, 2016

This looks good now. I'll deploy in the morning. For future reference I'd prefer you didn't force update an existing PR. If you add commits to address feedback it's easier for me to review just those changes instead of needing to review all changes again.

@wezm wezm merged commit 872505d into bellroy:master Sep 27, 2016
@wezm
Copy link
Contributor

wezm commented Sep 28, 2016

Deployed

@vaniver vaniver mentioned this pull request Sep 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants