Rnd discourse by acshi · Pull Request #6151 · CenterForOpenScience/osf.io

acshi · 2016-08-12T18:03:15Z

Purpose

This PR is being made to link the ongoing Discourse Integration project code to the main OSF.

Changes

framework.discourse module for interaction. Discourse IDs and hooks added to project, file, and wiki node classes. Single-Sign-On for Discourse through the OSF. Completely different comment-pane. Forum-feed Mithril component. See the README.md file in framework/discourse for more.

Example screenshots:

Side effects

Replacing of standard comments with Discourse. Comments will work differently. The APIv2 comment functionality would be broken.

brianjgeiger · 2016-10-04T13:59:12Z

It's Autumn Cleaning time, so I'm closing this PR. Feel free to re-open when development becomes active again.

acshi · 2016-10-04T15:28:09Z

Hmmm... This was opened originally at Steve Loria's request so that my
personal branch would be tied to the COS. Also, it is under active
development! But I'm not sure if the PR needs to be open or if anything
else should be happening with that.

On Tue, Oct 4, 2016 at 9:59 AM, Brian J. Geiger notifications@github.com
wrote:

It's Autumn Cleaning time, so I'm closing this PR. Feel free to re-open
when development becomes active again.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_CenterForOpenScience_osf.io_pull_6151-23issuecomment-2D251395753&d=CwMCaQ&c=-dg2m7zWuuDZ0MUcV7Sdqw&r=7fC-iE5nc1K0x8FY3MV5uEZng4gqi25wl7nIyZPmvbE&m=sjWQ-KTVWMgR017nGuihAvuEdanv_eigVtYu_iQZgtM&s=YCf6WgGlbVweswNZcehKpOMreiPEVitwY_ZqXVFQ5qw&e=,
or mute the thread
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABYf9sYS-2DkKzakDkI9ZIEKqKONelNtqKks5qwluygaJpZM4JjUZZ&d=CwMCaQ&c=-dg2m7zWuuDZ0MUcV7Sdqw&r=7fC-iE5nc1K0x8FY3MV5uEZng4gqi25wl7nIyZPmvbE&m=sjWQ-KTVWMgR017nGuihAvuEdanv_eigVtYu_iQZgtM&s=evS5dueN9Mk-qJ3dp7dL-wjiDlK65mmuEXhxhv37x6E&e=
.

brianjgeiger · 2016-10-05T19:50:37Z

Cool. When you're ready to do another push, go ahead and re-open it then. For now, we should still have access to it for what we need. We just had a lot of open PRs that I was asked to clean out.

brianjgeiger · 2016-10-05T19:51:29Z

Ah, it was only 7 days since the last push. Okay, I'll re-open. From the way Github showed it, I thought it had been months.

alexschiller

Notes:
100 in line comments, some redundant pointing out similar problems. Consider questions to be hypothetical and not requiring answers, but you should probably know the answer. Happy to discuss any comments further if anything unclear.

General punchlist of common suggestions

Fix import orders in all files
Fix importing of symbols-- import modules and call module.symbol instead
Add empty blank line to the ends of files
Use dictionary literals instead of manually adding via dict[key] = value
Use single quotes instead of double quotes
When possible, handle errors inside of functions instead of catching output when the function output fails
Don't use general try-except Exceptions, make sure you are handling only the exceptions you expect
Use triple equals in javascript

General

Some refactoring of functions can occur to simplify things-- some functions were only used once and can easily be made a single line. In those cases it is more readable not to have to look up what that function does.
The difference between a node, filenode, wikinode, project, parent all need to match how things are done in the OSF. Some of your relationships uses were different than I expected.
The mithril and javascript sections would benefit from a refactor. The python was generally easy to read but the javascript was not always.

alexschiller · 2016-12-19T16:06:29Z

@@ -0,0 +1,29 @@
+from website import settings


See these for import ordering, this applies to all imports you modified/added in all files.
https://www.python.org/dev/peps/pep-0008/#imports / http://cosdev.readthedocs.io/en/latest/style_guides/python.html#imports

alexschiller · 2016-12-19T16:15:58Z

+    out_dict = {}
+    for k, v in in_dict.iteritems():
+        if isinstance(v, unicode):
+            v = v.encode('utf8')


Encode/decode is generally written as 'utf-8' in the osf instead of just 'utf8'-- both work though.

alexschiller · 2016-12-19T16:28:46Z

    """Clear users' session(s) and log them out of OSF."""
-
-    for key in ['auth_user_username', 'auth_user_id', 'auth_user_fullname', 'auth_user_access_token']:
+    for key in ['auth_user_username', 'auth_user_id', 'auth_user_fullname',


If you're going to split the line, each should probably go on its own line (see line 15 of same file)

alexschiller · 2016-12-19T16:43:24Z

+    try:
+        discourse.logout()
+    except (discourse.DiscourseException, requests.exceptions.ConnectionError):
+        logger.exception('Error logging user out of Discourse')


Hypothetical questions that I don't need answers to:

What happens if the user is not logged out of discourse but is logged out of the osf?

What happens when they try to log back in?

Should logout fail (I doubt we should force someone to remain logged in, but the fact that this error catch needs to exist does raise a few questions)?

alexschiller · 2016-12-19T18:44:09Z

@@ -0,0 +1,7 @@
+import framework.discourse.common
+# import here so they can all be imported through the module


It would be better if you pulled in discourse and simply called the function from where it actually lives. This is a lot of importing symbols when you should be keeping them name spaced better. There is a reason you had to noqa everything

e.g.,

from framework import discourse discourse.project.sync_projects(...)

alexschiller · 2016-12-21T17:44:52Z

+            url: "${settings.DISCOURSE_SERVER_URL}/session/current.json",
+            xhrFields: { withCredentials: true }}
+        ).then(function(json) {
+            if (json.current_user && json.current_user.username == contextVars.currentUser.id) {


use triple equals

alexschiller · 2016-12-21T17:47:03Z

+
+function discourseAutoLogin() {
+    if (contextVars.currentUser.id) {
+        if (typeof(Storage) !== "undefined" && sessionStorage.discourseLoggedIn == "true") {


use triple equals, single quotes

alexschiller · 2016-12-21T17:53:01Z

+    }
+};
+
+if (window.addEventListener) {


Is there a reason this cannot be run with the mithril controller is getting everything set up? I may be missing what it is supposed to be doing, but a lot of this looks like things that can be configured in the controller.

alexschiller · 2016-12-21T17:54:38Z


+        <!-- Forum Feed (Latest Topics) -->
+        % if not node['anonymous']:
+        <div class="panel panel-default">


alexschiller · 2016-12-21T17:58:56Z

+        % if not node['anonymous']:
+        <div class="panel panel-default">
+            <div class="panel-heading clearfix">
+                <h3 class="panel-title">Latest Forum Topics</h3>


http://accessibility.psu.edu/headingshtml/

…configure_discourse_server function was added as well, and may be used after the discourse settings are properly set in local settings.

…more data from the correct project and file nodes, and to add the discourse comment plugin to file pages, automatically making the correct groups, categories, and topics

…name. Ensured group sync test passes

…edded the discourse comments into the comments side panel

…rough them, in the db.

…o, and better topic creation.

…ted, moved, or renamed

…box or other files are renamed.

…ds a comment from the system to the topic

…d a minimum number of times. Fixed the SSO get method signature. Added automatic configuration of the welcome-to-discourse topic contents

…ttempt to use it triggers an additional try. gave it a 0.25s timeout, since it should be local

…dpoint on Discourse for creating projects and dealing with them only by project_guid. fixed tests to make sure they all pass. added some documentation.

…kis, deleting projects/components, and made auto discourse log-in smarter

…ment pane iframe height calculation.

brianjgeiger · 2017-03-28T20:25:34Z

I'm trying to install this on my local setup (finally!), and when I invoke the server I'm getting:

  File "/Users/bgeiger/cos/osf/osf/models/user.py", line 368, in OSFUser
    discourse_apikey_date_created = NonNaiveDatetimeField(default=None, null=True, blank=True)
NameError: name 'NonNaiveDatetimeField' is not defined

I've invoked all the requirements that I could (using the -q flag). Is there anything that maybe isn't in a requirements file?

acshi · 2017-04-03T19:53:58Z

@brianjgeiger Sorry about the delay in a response. I think this must be from some recent change to the develop branch of osf.io. I recently did a rebase, but haven't gone through running all the code again. I am a little reluctant to, because my environment has diverged substantially since I can't run docker in the specified way. (I don't have a recent-year mac). By and large, I aimed to use the same conventions as the OSF already was. That specific datetime field type is not important as long as it is consistent.

brianjgeiger · 2017-04-03T19:57:38Z

This should definitely not try to be merged with develop. The branch itself should be, for the purposes of this review, its own island. If it's in a partway state, then I'm not going to be able to get it running. Can this be rolled back to a last-known working state?

acshi · 2017-04-07T05:11:28Z

Well, NonNaiveDatetimeField is actually just a typo. It should be NonNaiveDateTimeField.
For me to really be sure it is working on either an older rollback or this version might take a little (I'm not super familiar with undoing a rebase), so be patient with me as I try to find time to look more at this now.

…k after rebase.

acshi · 2017-04-07T18:14:23Z

@brianjgeiger I've fixed the typo and I can get it to run locally. However, the amount of testing I can do is limited right now since I'm having trouble being able to create an account on my new instance. I get the error: "Invalid Token This confirmation link is invalid." when trying to make a new account.

brianjgeiger · 2017-04-07T18:30:21Z

Cool, thanks! I'll pull and give it a go again.

acshi force-pushed the rnd-discourse branch from 671c15a to b355494 Compare September 28, 2016 20:23

brianjgeiger closed this Oct 4, 2016

brianjgeiger reopened this Oct 5, 2016

acshi force-pushed the rnd-discourse branch from 7fa1c17 to 444065b Compare October 13, 2016 20:09

acshi force-pushed the rnd-discourse branch from a619824 to fa60190 Compare December 17, 2016 18:51

alexschiller suggested changes Dec 22, 2016

View reviewed changes

acshi force-pushed the rnd-discourse branch 2 times, most recently from bb37e0d to 8134590 Compare January 19, 2017 02:35

acshi added 18 commits March 21, 2017 21:49

Initial work allowing single sign on for discourse. It works.

1d49921

log out on either discourse or osf logs out the other. a convenience …

67c9a22

…configure_discourse_server function was added as well, and may be used after the discourse settings are properly set in local settings.

more api calls for discourse implemented for groups and categories.

b1ee1fe

made a roughly blank forum page, updated discourse framework to pull …

0005c70

…more data from the correct project and file nodes, and to add the discourse comment plugin to file pages, automatically making the correct groups, categories, and topics

fixed username lookup to only cache the current user's discourse user…

c750320

…name. Ensured group sync test passes

Added comment handling functions for discourse api

99eec43

better handling of users

5ed59c4

refactored api code to work with files, wiki, and components, and emb…

2345c97

…edded the discourse comments into the comments side panel

saving comment topic ids in the file, project, and wiki nodes, and th…

1e9ddc1

…rough them, in the db.

storing discourse ids in the osf db, better syncing of between the tw…

c68c514

…o, and better topic creation.

added css/html customizations into automated discourse config settings

49c1fce

working on making sure that comments are not lost when files are upda…

0394aa3

…ted, moved, or renamed

small changes trying to figure out how to persist topic ids when drop…

318f86c

…box or other files are renamed.

added automatic creation of discourse users when needed by the OSF

a4d4ccc

finally got topic ids to persist in dropbox files. renaming a file ad…

eb4bb65

…ds a comment from the system to the topic

fixed sso import error

ec4ebe5

changed discourse usernames to user guids

f8fa455

using guids for topic and user ids

8d2799d

acshi added 22 commits March 21, 2017 21:54

undefined check for view_only links

6806346

flake8 style fixes

1b0ea0d

[skip ci] added some discourse settings

c6e5d50

[skip ci] made the discourse framework make sure node.save() is calle…

2dca694

…d a minimum number of times. Fixed the SSO get method signature. Added automatic configuration of the welcome-to-discourse topic contents

readme bug-list updates

b004c31

fixed error logging from discourse. if discourse is not found, each a…

15294fc

…ttempt to use it triggers an additional try. gave it a 0.25s timeout, since it should be local

added function documentation, updating tests

af385b1

[ci skip]removed groups from framework.discourse in favor of a new en…

e02dd33

…dpoint on Discourse for creating projects and dealing with them only by project_guid. fixed tests to make sure they all pass. added some documentation.

[ci skip]missing import

e8a62c6

[ci skip]added missing hooks for syncing view_only links, deleting wi…

d5af1c0

…kis, deleting projects/components, and made auto discourse log-in smarter

[ci skip]updating instructions for docker-machine setup to be persistent

ac59106

[ci skip]allow redirects in Discourse get request

e2a15f1

[ci skip]discourse configure puts correct names into the TOS. fix com…

2e7dd2c

…ment pane iframe height calculation.

Update README.md

9e1c06a

Update README.md

106d737

Update README.md

ef79058

locking down on a specific Discourse version

ab5dafa

[ci skip]fixes for how discourse url is used.

fc562c5

in process of responding to code review

217696d

changes in response to code review

9ab13f0

[ci skip]more changes made in response to code review.

4b801be

[ci skip]discourse changes for djangOsf

d8414d4

acshi force-pushed the rnd-discourse branch from 8134590 to d8414d4 Compare March 22, 2017 01:58

fixed spelling of NonNaiveDateTimeField and updated migrations to wor…

9f00e9c

…k after rebase.

		@@ -0,0 +1,7 @@
		import framework.discourse.common
		# import here so they can all be imported through the module

Conversation

acshi commented Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Changes

Side effects

Uh oh!

brianjgeiger commented Oct 4, 2016

Uh oh!

acshi commented Oct 4, 2016

Uh oh!

brianjgeiger commented Oct 5, 2016

Uh oh!

brianjgeiger commented Oct 5, 2016

Uh oh!

alexschiller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianjgeiger commented Mar 28, 2017

Uh oh!

acshi commented Apr 3, 2017

Uh oh!

brianjgeiger commented Apr 3, 2017

Uh oh!

acshi commented Apr 7, 2017

Uh oh!

acshi commented Apr 7, 2017

Uh oh!

brianjgeiger commented Apr 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

acshi commented Aug 12, 2016 •

edited

Loading