-
Notifications
You must be signed in to change notification settings - Fork 235
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
Dashboard (Both design and functionality) #471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sayamkanwar Great work, I haven't tested it locally yet. But left some comments related to coding style fixes and some doubts.
Also have you tested whether this works for real time collaboration feature too?
Because due to multiple share button clicks we used to get different ids, have you tested real time collaboration after this change?
ide/static/js/content.js
Outdated
@@ -979,6 +980,35 @@ class Content extends React.Component { | |||
layer.info.phase = 0; | |||
this.setState({ net }); | |||
} | |||
saveModel(){ | |||
let netData1 = this.state.net; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this variable
ide/static/js/content.js
Outdated
@@ -979,6 +980,35 @@ class Content extends React.Component { | |||
layer.info.phase = 0; | |||
this.setState({ net }); | |||
} | |||
saveModel(){ | |||
let netData1 = this.state.net; | |||
console.log(netData1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove log statements
ide/static/js/content.js
Outdated
@@ -1055,7 +1086,7 @@ class Content extends React.Component { | |||
// Note: this needs to be improved when handling conflict resolution to avoid | |||
// inconsistent states of model | |||
let nextLayerId = this.state.nextLayerId; | |||
|
|||
var shareBool = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename the variable and use let
ide/static/js/content.js
Outdated
@@ -1299,9 +1341,11 @@ class Content extends React.Component { | |||
<div id="sidebar-scroll" className="col-md-12"> | |||
<h5 className="sidebar-heading">ACTIONS</h5> | |||
<TopBar | |||
isPublic_Sharing={this.state.isShared} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove _
} | ||
}; | ||
|
||
class Dashboard extends React.Component{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add two different components in same files
ide/urls.py
Outdated
from django.conf.urls import url, include | ||
from django.contrib import admin | ||
from django.conf.urls.static import static | ||
from django.conf import settings | ||
from views import index, calculate_parameter, fetch_layer_shape | ||
from views import load_from_db, save_to_db, fetch_model_history | ||
from views import load_from_db, loadModel_from_db, deleteModel_from_db, save_to_db, saveModel_to_db, fetch_model_history |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please follow naming conventions for method names
ide/views.py
Outdated
|
||
if Network.objects.filter(name=net_name).exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net_nam is not the primary key, please use mode id as the key to filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will I check using model id? If a person is creating a new model with the same model name then only I can run a query right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, in this method I'm taking the net name from the POST request to filter from database because that's the only parameter that can be used to do this as this would be the only similarity amongst the already existing records and new models. So I can't really use model id to filter out results.
@csrf_exempt | ||
def loadModel_from_db(request): | ||
if request.method == 'POST': | ||
if 'userID' in request.POST: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow naming conventions
ide/views.py
Outdated
|
||
|
||
@csrf_exempt | ||
def saveModel_to_db(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to save model to db methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a save model to db method as the save to db function saves the model in the database with sharing properties and I tried to combine the two but it wasn’t working because they both work differently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The save button triggers this function
Hi @Ram81, I’ll make the rest of the changes which you have mentioned as soon as possible. |
@Ram81, I have made all the changes except 2, I have replied so as to why I can't make those changes, please read those and let me know what should be done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sayamkanwar did you also tested it for real time collaboration feature and please verify if there is any corner case you missed.
@@ -979,6 +980,34 @@ class Content extends React.Component { | |||
layer.info.phase = 0; | |||
this.setState({ net }); | |||
} | |||
saveModel(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand this method performs update on already existing model so please rename it to update model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method doesn’t update on already existing model, it saves the model data on clicking the save button but if the model already exists in the database then it updates the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if you are doing this then why do we have older method still present?
Why can't your remove that or add the code in same method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can’t remover the older method as there is a difference between the 2 methods the older one is saving the model with shared properties while the new method is saving the model with not shared properties, if you still want me to combine the two, I’ll try to do it.
@@ -1072,6 +1101,13 @@ class Content extends React.Component { | |||
// while loading a model ensure paramete intialisation | |||
// for UI show/hide is not executed, it leads to inconsistent | |||
// data which cannot be used further | |||
if (response.public_sharing == false) { | |||
is_shared = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are not changing value before this step then why do you have to set it to false again as the default value is already false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I’ll make the change, thanks for pointing out!
exportNet={this.exportNet} | ||
importNet={this.importNet} | ||
saveDb={this.saveDb} | ||
saveModel={this.saveModel} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the method name to update model (if I understand the functionality correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have explained this function above, please have a look at it, it’s not just updating the already existing model, it updates the model only if it already exists else it saves the model.
ide/static/js/content.js
Outdated
@@ -1299,9 +1339,11 @@ class Content extends React.Component { | |||
<div id="sidebar-scroll" className="col-md-12"> | |||
<h5 className="sidebar-heading">ACTIONS</h5> | |||
<TopBar | |||
isPublicSharing={this.state.isShared} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one flag variable in state of content.js where I maintain the type of model as isShared or not, can't we use that here because defining a new one causes necessity to maintain consistency in both. If not possible then please rename this variable name in props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven’t defined this, this was already in the existing code. isShared is the flag variable which is there in content.js file right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t exactly understand, could you please elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one boolean flag in content.js state variable if you are passing it down as props use the same name and don't define new ones if possible
ide/templates/index.html
Outdated
@@ -26,7 +26,7 @@ | |||
</script> | |||
|
|||
</head> | |||
<body> | |||
<body style="background: #F3F5F7;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't add style tags here maintain them in css
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay!
name='calculate-parameter'), | ||
url(r'^layer_parameter/', fetch_layer_shape, | ||
name='fetch-layer-shape'), | ||
] + static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix alignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was according to the PEP8. It gives an error otherwise.
@Ram81, what exactly do you mean by testing it for real time collaboration feature? I have already mentioned the load page is working fine for shared models. If that’s what you are asking? |
@Ram81, please have a look at the replies and let me know what shall be done further. |
@sayamkanwar I meant did you try making updates in real time collaboration mode and see the changes getting reflected in corresponding tabs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sayamkanwar can you reduce the radius of rounded corners of cards
Sure, I’ll make the changes! |
Hi @Ram81,
Let me know if you need anymore changes. |
@sayamkanwar please reopen the PR on |
Duplicate of #477 |
Hey,
I had claimed the task '[Fabrik] Add backend support for maintaining dashboard for a user' but there was no dashboard in Fabrik presently so I had to design a dashboard first which was another task (https://codein.withgoogle.com/tasks/6415117748011008/). So I have made the deliverables for both the tasks because the former task wasn't possible without completing the latter task.
Work done
saveModel_to_db
for that, it saves the model to the database withpublic_sharing
set to false by default. This function first checks the database if the model already exists, it updates the JSON field instead of creating a new entry if the model already exists.On clicking the save button, if there are no errors. It gives the user a success message.
I have added a button 'Dashboard' in canvas sidebar which takes the user to dashboard. This button is only shown when the user is logged in, also the if someone tries to open dashboard by manually entering the dashboard URL, it redirects the person to home page (i.e. canvas) if the user is not logged in.
The dashboard consists of a button 'Create a new model' and it displays the list of models created by the user with it's name and thumbnail. Each model list item has two options: edit and delete which become visible on hover. Currently the thumbnail is just a sample image as I didn't know how will the actual preview of the model will be rendered as a thumbnail and I was guided by the mentor to just leave it like that for the time being. I have also created the route to dashboard(/dashboard).
[on hover]
[on deleting the model]
[if the user hasn't created any models]
I have modified the existing
load_from_db
function, now it also returns thepublic_sharing
property of the model which is used by the main canvas to check if a model is shared. If it's shared it adds comments option to it and other shared features. If it's not shared, it acts as an editor for the user.The current criteria for rendering the Share/History button in the canvas was that if the path is /load so I had to update the criteria, now I'm passing on the value of
public_sharing
to thetopBar
component as a prop value and in thetopBar
component it renders the Share button if value ofisShared
state is false, else if it's true it renders the History button which shows the history of the model.To make login system more easy to work with, I have added the piece of code in the login function to set localStorage item 'userID' to the value of the user id, it makes the work a lot more easier as different components can use user id value for different functions. On logout, the localStorage item is destroyed as well.
NOTE: I haven't removed any of the already existing code for the login part, I have just made an addition.
I have added the tests for all the new functions that I have created in
tests/unit/caffe_app/test_db.py
.[all tests passed]
Modifications in old features
In the
save_to_db
function, now it checks if the model exists in the database and updates the json field else it creates a new record in the database. Also, if a model exists in the database with public_sharing set to false. On clicking the share button which triggers thesave_to_db
function sets the public_sharing of the existing model to true instead of creating a new shared model.Correction
For the people who are using docker to run Fabrik, they'll have to edit the
settings/test.py
file because the variables have been set to the values being used in the virtual environment method, I have added the comments in this file for the changes that have to be made for docker, otherwise it will throw an error.Please review my work @Ram81 @RishabhJain2018, thank you! :)