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

Added Proof of concept Fork functionality #353

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Added Proof of concept Fork functionality #353

wants to merge 3 commits into from

Conversation

jahandaniyal
Copy link
Collaborator

This patch implements #336 - Allow users to fork/clone a graph.
The following scenarios are covered :

  1. A user opens a graph he had uploaded himself.
    Observation - UI has no Fork Button or forked from message.
  2. User opens a graph he had forked earlier.
    Observation - forked from parent_graph_owner's_email / Name of graph is displayed. The user can click on this link to go to the parent graph. Similar to what he have on Github.
  3. User opens the Parent Graph which he already forked and tries to Fork again.
    Observation - A message is displayed stating that the user has already forked the graph.
    Github on the other hand, redirects to the existing fork instead of showing any message.
    I can implement similar behaviour for Graphspace if desired.
  4. User shares a graph to public and tries to fork it himself.
    Observation - Same as case 1.

A general scenario would be that User A uploads a graph and shares it to public. User B can then fork the graph as described in the above cases.

fork_demo

Copy link
Collaborator

@adbharadwaj adbharadwaj left a comment

Choose a reason for hiding this comment

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

Few more changes. Overall logic looks good.

@@ -8,6 +8,7 @@
url(r'^graphs/(?P<graph_id>[^/]+)$', views.graph_page, name='graph'),
url(r'^graphs/(?P<email>[^/]+)/(?P<graph_name>[^/]+)$', views.graph_page_by_name, name='graph_by_name'),
url(r'^upload$', views.upload_graph_page, name='upload_graph'),
url(r'^fork_graph$', views.fork_graph, name='fork_graph'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cna you fix the indentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Surely, I will fix it.

'graph_json':JSON.stringify(graph_meta_data, null, 4),
'style_json':JSON.stringify(cytoscapeGraph.getStyleJSON(graphPage.cyGraph), null, 4)
}
$.ajax({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of making an ajax call here. Can you add an API function at the top of the file (checkout Line 5)- just like - apis.graphs.get etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review. I will make the changes you have requested and send another pull request.

</div>
{% elif uid != graph.owner_email %}
<div class="btn-group pull-right" style=": 1em !important; z-index: 20000">
<button type="button" id='ForkGraph' class="btn btn-sm btn-default"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ID should be ForkGraphBtn

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I will make the changes.

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

Successfully merging this pull request may close these issues.

2 participants