New user command #10

bookdude13 · 2016-05-19T00:06:24Z

No description provided.

bookdude13 · 2016-05-19T00:07:53Z

Addresses #6.

rjayatilleka · 2016-05-19T22:35:30Z

src/main.rs

@@ -6,15 +6,23 @@ extern crate git2;
 use std::env;
 use std::path::Path;

+fn main2() {


Rebase on top of issue 4 when that's done, and make sure that src/main.rs is all cleaned up afterwards.

rjayatilleka · 2016-05-19T23:08:03Z

Kaaaay, I ripped it apart. Good luck :)

rjayatilleka · 2016-05-20T00:57:06Z

Oh and add some integration tests, like the ones in tests/initialize_project.rs.

rjayatilleka · 2016-05-20T02:36:19Z

src/project_types.rs

@@ -1,17 +1,38 @@
 /// Structure to represent a Proton Project.
 /// This is what will be written to a Protonfile at the project root.


This should be right next to Project. Move User down below pub struct Project and above impl Project.

rjayatilleka · 2016-05-20T02:39:23Z

src/project_types.rs

        }
    }
+
+    // Adds a user to the project


Doc comment (3 slashes)

bookdude13 · 2016-05-20T02:41:39Z

If name is a &String, should User.name be of type &String as well? That can cause a fair amount of manual lifetime management

rjayatilleka · 2016-05-20T02:56:28Z

No, forget everything I said about references earlier. Just keep everything as Strings like earlier. You had it right before, because that String goes from Args -> run_new_user -> new_user -> User -> Project -> disk, and then is never used again. So it's totally fine.

rjayatilleka · 2016-05-20T02:58:02Z

src/project_types.rs

@@ -1,17 +1,38 @@
 /// Structure to represent a Proton Project.
 /// This is what will be written to a Protonfile at the project root.
-#[derive(Debug, RustcEncodable, RustcDecodable)]
+


Get rid of this blank line. Comments should be directly next to what they're doc-ing. Otherwise it just looks weird. Do you not think so?

To me it looks weird next to the #[derive()] statement. Move the derive inside the struct? Otherwise it does look better than having the space

/** * Merges stuff * @param dataIds */ @Override public String runMerge(List<String> dataIds) throws TException {

Idk, it looks normal to me to not have a line there in Java. Well, do it however you want. Maybe we should run rustfmt on it?

Nah, don't bother. Looks pretty good w/o the blank line

Can you even move the derive? I don't think you can. Though you shouldn't anyway, it would be different from everything else. You can move the doc comment into the struct by changing it to use //!, but again, taht would be inconsistent.

Anyway, for now, just leave the line there. We can try rustfmt out later, in a different issue.

rjayatilleka · 2016-05-22T16:23:35Z

src/project_types.rs

@@ -43,3 +56,13 @@ impl PartialEq for Project {

 impl Eq for Project {
 }
+
+impl PartialEq for User {


Get rid of both PartialEq and Eq impls. Add #![derive(Eq)] to Project and User.

rjayatilleka · 2016-05-22T16:30:07Z

Sort of. You did find_user. I should really just read the code before I comment.

User should contain a Vec for keys to handle the one to many.
You need to implement a lookup_user_by_pub_key, and then check that on new user.
It doesn't look like you're using find_user anywhere. I don't think its actually necessary?
Make sure to use deriving on the Eq traits. We already let them get out of sync with the actual data: Project's eq impl doesn't consider the users.

bookdude13 · 2016-05-22T16:34:18Z

1 and 2: Could we use a HashSet or HashMap, with the public key being the key and the user being the value? That takes care of uniqueness of the key, and might make the lookup simpler
3: Using it in the test to check that the user was added properly. Thought it might be helpful to have as well, so I put it within the Project implementation.
4. Will do. Didn't know about that derive.

rjayatilleka · 2016-05-22T16:54:57Z

Sounds good. So, something like this:

Project
- name : string
- username_to_user : map<string, User>
- pub_key_to_username : map<string, string>
User
- Name (this will be a duplicate storage, but its okay)
- (in the future) permissions, admin, etc

Can we JSON en/decode the in built hash maps? -- Yes we can, the traits are implemented for hashmap in the rustc-serialize package that we use.

Then we need two commands instead. new-user <username> and add-key <username> <pub-key>. Usernames should be unique. This is kinda complicated, but its necessary if we want one user to many keys.

Do we actually want that? I think it's actually better to just make it one to one, because then its less complicated for the users. I mean, we're going to be distributing the private key to every person doing sequencing anyway (at least, I can't think of another way to do it. We can just call it a password and tell them to plug it in when they open the GUI). It's not like they're going to use the private keys for general use on their own computers.

bookdude13 · 2016-05-22T17:14:08Z

I'm alright with keeping it simple and making it one to one. If they use multiple systems, they will just have to copy the private key manually.

That would make Project and User look the same as before, just with added checks inside add_user()

rjayatilleka · 2016-05-22T17:25:10Z

Yup, sounds good.

rjayatilleka · 2016-05-22T17:31:21Z

Now find user is only checking if the name AND key are equal. You need to check if name OR key are equal.

rjayatilleka · 2016-05-22T17:33:25Z

tests/new_user.rs

+}
+
+/// Creates a temporary directory to run a test out of
+/// and initializes a new project inside this directory


This doesn't initialize a new project.

bookdude13 · 2016-05-22T17:41:43Z

Thanks, good catch. Just in time to make my tests pass too

bookdude13 · 2016-05-22T18:16:37Z

Good?

rjayatilleka · 2016-05-22T18:41:22Z

Almost good. The only thing I noticed was in the tests was that you when you reset the current directory, you only go up to ... TempDir library makes a directory in the environment temp dir, so /tmp generally. So going up isn't correct. You want to save the current directory, then return directly to the saved directory.

But actually, I don't think you need that in the first place. Tests shouldn't depend on the directory they're in, unless they set it up themselves. So you can probably just get rid of all the directory resets. After that, LGTM.

rjayatilleka · 2016-05-22T18:42:07Z

And let me check real quick if there's a way to specify how many threads to use in the cargo.toml.

rjayatilleka · 2016-05-22T18:49:29Z

And no, there's not currently a way to set the RUST_TEST_THREADS in the cargo.toml (see rust-lang/rust#25636), so just forget about it. We might want to add a makefile to wrap around cargo, but we can think about that later if we need.

rjayatilleka · 2016-05-23T00:26:19Z

Great!

bookdude13 · 2016-05-23T00:35:24Z

Whoops, needs to commit changes still

rjayatilleka · 2016-05-23T00:49:12Z

Oh k. Well, go ahead and merge when you're ready.

* Added name to authors (#4) * Name (#7) * Added Vlad to authors list (#10) * Overhaul of structure to use DAOs and database for backend storage (#11) * Initial database implementations (#12) * Updated README (#14) * Working code for 2016 show (#19) * Created initial 'it works' test * Restructured into separate tests. 'is-working' tests complete? * Restructured 'all working' test * Added more tests. Reorganized again * One more test * Added test for seq sec editor added on perm change. Implemented; passing * All tests implemented except working and 0 sections * All tests passing * Restructured so sequence not returned on resection * Converted main code to new permissions api * Tests using new api. Removed check for sequence section editor. Some tests failing * All tests passing * Removed editor from sequence_section type * Tests now want seq sec in range [0, numSections) * SeqSec now 0-indexed. Removed last traces of editor field * Updated README and fixed error in main * Added checks * Moving towards Fixtures instead of 2d blocks of sequence data * Restructured to use DAOs and database setup instead of storing in flat files * Added initial database backup * Postgres connection working in channel dao * Generalized common functionality * Updated names, public instead of private key, uid for list-permissions * Init project returns generated public key * Updated database definition * Looser Cargo package versions for upgrading/good documentation * Adding initial user mostly implemented. Util function to gen pub/priv keys * Store both pub/priv key. Trim before compare. Get uid from key started * Permission handling stuff. PermissionEnum is string now for simplicity * new-user fully implemented * A little cleanup of tests; moving towards using again * Removed unused imports. New return type for get-uid * PermissionEnum updated. Preliminary work on add_sequence done * new_sequence working? * get_sequence command added to main * new-layout function set up * Reading layout from file. Locations and rotations checked * Adding layout implemented. Transactions/removing not implemented * Layout as of 11/21/2016 added * Project in db behind DAO now as well. General cleanup * set-sequence-layout working * Existential dao functions * Channel data now in separate table * Moved db backups to separate folder * get-layout-id command added, not implemented * Finished implementing add-sequence * get-playlist-data implemented * new-vixen-sequence working. Nullable values changed to options. * Updated Cargo.toml * Added seqid to playlist output * Changed the format of playlist data * Added music file to playlist outputted data * Made functional again after rebase * Added sequence duration to Sequence object for accurate num_frames calc * Added internal channel to Channel type and layout reading * Patching stuff * Added current db backup with internal channel * Patching working now; hackish fix * Added patch file * Updated layout and patch * Working show backed up * Stopped copying music over * Using postgres FUNCTION for patching * Added DB with FUNCTION in last commit added * New patch * Added newer patches * main.rs alphabetized. Added get-project command * Cleaned up warnings * More verbose patching * Added SQL to reset database to clean state * Fixed error with reading file as string * WORKINGgit st! * Working show backup * Adding admin key for 1202 backup * Actual working show backup (whoops) * add-sequence turned into insert-sequence * Udated patch * Print statemets for verbosity * 12_03 patch * Different format for outputting playlist data * Removed public keys from repo * New sequences don't copy their music files to a Music/ directory * Version now 0.19.1 as per SemVer (#33) * Adding beginning of testing framework post-overhaul (#32) * Documentation (#28) * Removed rebase artifacts. Updated version in Cargo.lock (#34) * Permissions checks moved from the second-tier functions to main.rs (#35) * Permissions checks moved from the second-teir functions to main.rs * Fixes issues to moving permissions check * Updated version numbers for dependencies (removing wildcards) (#36)

rjayatilleka reviewed May 19, 2016
View reviewed changes

bookdude13 added 4 commits May 19, 2016 20:06

Framework for 'user new' command added

805af5c

Adding users works. Needs error checking

d58b457

User new implemented

de4a8fd

Renamed to new_user

39bb46c

bookdude13 force-pushed the iss6 branch from 338c6f4 to 39bb46c Compare May 20, 2016 01:17

bookdude13 added 2 commits May 19, 2016 20:18

Removed redundant main function

8106664

Added Ramith's suggestions except name reference instead of borrow

a5aecbc

rjayatilleka reviewed May 20, 2016
View reviewed changes

Rearranged order of structs

e4914fd

rjayatilleka reviewed May 20, 2016
View reviewed changes

src/project_types.rs

}

}

// Adds a user to the project

Copy link

Contributor

rjayatilleka May 20, 2016

Choose a reason for hiding this comment

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

Doc comment (3 slashes)

bookdude13 added 2 commits May 19, 2016 21:39

Added comment

f58397d

Made comment doc comment

e993b45

rjayatilleka reviewed May 20, 2016
View reviewed changes

rjayatilleka reviewed May 22, 2016
View reviewed changes

bookdude13 added 2 commits May 22, 2016 11:35

Deriving PartialEq and Eq for Project and User

3ebcdf7

Removed unnecessary clone and debug print statement

b8fe35d

Renamed find_user to user_exists. Added duplicate user check to add_user

f945f02

rjayatilleka reviewed May 22, 2016
View reviewed changes

bookdude13 added 2 commits May 22, 2016 12:43

Fixed duplicate user check

89d8daa

Added duplicate user tests. Fixed comment. Removed unnecessary map_err

7c4610b

bookdude13 added 2 commits May 22, 2016 19:06

Removed backing out of directories at end of tests

f73b10b

Removed unused import

a29dd14

Tweaked error message

3e63976

bookdude13 merged commit efc7867 into master May 23, 2016

bookdude13 deleted the iss6 branch May 23, 2016 01:07

bookdude13 mentioned this pull request May 23, 2016

New user #6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New user command #10

New user command #10

bookdude13 commented May 19, 2016

bookdude13 commented May 19, 2016

rjayatilleka May 19, 2016

rjayatilleka commented May 19, 2016

rjayatilleka commented May 20, 2016

rjayatilleka May 20, 2016

rjayatilleka May 20, 2016

bookdude13 commented May 20, 2016

rjayatilleka commented May 20, 2016

rjayatilleka May 20, 2016

bookdude13 May 20, 2016

rjayatilleka May 20, 2016 •

edited

Loading

bookdude13 May 20, 2016

rjayatilleka May 20, 2016

rjayatilleka May 20, 2016 •

edited

Loading

rjayatilleka May 22, 2016

rjayatilleka commented May 22, 2016

bookdude13 commented May 22, 2016

rjayatilleka commented May 22, 2016

bookdude13 commented May 22, 2016

rjayatilleka commented May 22, 2016

rjayatilleka commented May 22, 2016

rjayatilleka May 22, 2016

bookdude13 commented May 22, 2016

bookdude13 commented May 22, 2016

rjayatilleka commented May 22, 2016

rjayatilleka commented May 22, 2016

rjayatilleka commented May 22, 2016

rjayatilleka commented May 23, 2016

bookdude13 commented May 23, 2016

rjayatilleka commented May 23, 2016

		@@ -1,17 +1,38 @@
		/// Structure to represent a Proton Project.
		/// This is what will be written to a Protonfile at the project root.

New user command #10

New user command #10

Conversation

bookdude13 commented May 19, 2016

bookdude13 commented May 19, 2016

Choose a reason for hiding this comment

rjayatilleka commented May 19, 2016

rjayatilleka commented May 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bookdude13 commented May 20, 2016

rjayatilleka commented May 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjayatilleka May 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjayatilleka May 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjayatilleka commented May 22, 2016

bookdude13 commented May 22, 2016

rjayatilleka commented May 22, 2016

bookdude13 commented May 22, 2016

rjayatilleka commented May 22, 2016

rjayatilleka commented May 22, 2016

Choose a reason for hiding this comment

bookdude13 commented May 22, 2016

bookdude13 commented May 22, 2016

rjayatilleka commented May 22, 2016

rjayatilleka commented May 22, 2016

rjayatilleka commented May 22, 2016

rjayatilleka commented May 23, 2016

bookdude13 commented May 23, 2016

rjayatilleka commented May 23, 2016

rjayatilleka May 20, 2016 •

edited

Loading

rjayatilleka May 20, 2016 •

edited

Loading