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

Fixing free drive space issue in webadmin, close #11570 #1695

Merged
merged 3 commits into from Nov 5, 2013
Merged

Fixing free drive space issue in webadmin, close #11570 #1695

merged 3 commits into from Nov 5, 2013

Conversation

atarkowska
Copy link
Member

Fixing https://trac.openmicroscopy.org.uk/ome/ticket/11570

Testing:
Log in as root to webadmin and in Statistics check Free space just under the pie-chart
Log in again as a user, go to User settings and check if in Statistics is exactly the same value

Previously, mismatch example:

  • User: Free space: 225770 GB
  • Admin�: Free space: 220.48 GB

@@ -42,6 +42,7 @@
$(this).css('visibility', 'hidden');
});
});

Choose a reason for hiding this comment

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

Is this line here necessary? It's not a big issue but seems very odd...

@bpindelski
Copy link

All works as expected - free space reported for both a normal user and an admin is in the same units. Looks good to merge.

} else if (bytes < (1024*1024*1024*1024*1024*1024)) {
return (bytes / (1024*1024*1024*1024*1024)).toFixed(2) + ' PB';
} else {
return bytes + ' B';
Copy link
Member

Choose a reason for hiding this comment

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

One last question: is there any reason to fall back to B rather than leaving really big sizes PB? i.e. remove the final } else { clause and simply let big things be in petabytes?

Copy link
Member

Choose a reason for hiding this comment

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

I assume that the web gateway isn't a place where we can simply use Django's filesizeformat builtin.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't use filesizeformat as this is JavaScript. It is loaded after template is rendered

@atarkowska
Copy link
Member Author

@joshmoore like that?

@joshmoore
Copy link
Member

@aleksandra-tarkowska : exactly. Thanks. (I wonder how we're going to test it :) )

@atarkowska
Copy link
Member Author

I do not know any better way then http://jsfiddle.net/aleksandratarkowska/LEUU8/

@atarkowska
Copy link
Member Author

@bpindelski done

@bpindelski
Copy link

@aleksandra-tarkowska Thanks. Looks good to merge now (unless @joshmoore says otherwise).

@joshmoore
Copy link
Member

That's a great way to test it. Thanks again, @aleksandra-tarkowska! Changing the value to:

    var freespace = 5127899908842624000;

however, makes the display disappear. You'll need to change the final else if to else (i.e. remove if (bytes < (1024 * 1024 * 1024 * 1024 * 1024 * 1024))

@atarkowska
Copy link
Member Author

I updated the link, sorry are you sure you used http://jsfiddle.net/aleksandratarkowska/LEUU8/ ? there is definitely else only

@joshmoore
Copy link
Member

Got it now. Sorry, there's no notification on a comment modification, so I didn't realize there was a new one. 👍

@jburel
Copy link
Member

jburel commented Nov 5, 2013

@aleksandra-tarkowska: one small thing. The date in header needs to be modified
i.e. 2011 becomes 2011-2013

joshmoore added a commit that referenced this pull request Nov 5, 2013
Fixing free drive space issue in webadmin, close #11570
@joshmoore joshmoore merged commit 5729ad6 into ome:develop Nov 5, 2013
@jburel
Copy link
Member

jburel commented Nov 5, 2013

too late

@atarkowska
Copy link
Member Author

@jburel I will review all files in the another PR

@atarkowska atarkowska deleted the 11570_drivespace branch November 5, 2013 14:04
@atarkowska
Copy link
Member Author

--no-rebase

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

Successfully merging this pull request may close these issues.

None yet

5 participants