Fix/behaviour #11

MurhafSousli · 2017-09-09T13:19:28Z

closes #9

- Assign roles -> alert - Assign groups -> alert - Edit User -> alert

adrianq · 2017-09-10T08:14:19Z

@tokland I am adding you as a reviewer. Can you please have a look at this PR when you are back to work? I will also do a review after yours...

tokland · 2017-09-11T08:37:01Z

src/List/ContextActions.js

@@ -8,29 +8,52 @@ config.i18n.strings.add('details');
 config.i18n.strings.add('assignToOrgUnits');

 const contextActions = Action.createActionsFromNames([
-    'details',


I see (ad807e6) that those conversions to 2 spaces are intentional, but this way we are mixing 2 and 4 spaces in the same project? Are you planning to convert the whole project to 2 spaces?

@tokland I didn't intentionally want to change the spaces actually, but I had to hit the organize code shortcut before committing anyway, and It turned out whether I choose 2 or 4 spaces, git was seeing it as total change, so I went with 2 spaces (which I use usually)

@tokland what do you use in general? four spaces, right? no tabs, correct? This is a sort of a fork from https://github.com/EyeSeeTea/organisationUnit-User-Assigment. In there, we were using four spaces so probably it is better if we keep the 4 spaces conventions. I definitively would like the three of us to use the same convention

Personally, with JS, I like 2-spaces-no-tabs. But for existing projects let's keep whatever we had, here 4-spaces-no-tabs.

tokland · 2017-09-11T08:54:44Z

src/i18n/i18n_module_en.properties

@@ -341,6 +341,9 @@ data_set=Data Set
 intro_data_set=Create, update, view and delete data sets and custom forms. A data set is a collection of data elements for which data is entered.

 assignToOrgUnits=Assign to organisation units
+assignRoles=Assign roles


Side note: at some point, we should fix the i18n files; it has lots of unused keys and each language has different keys.

@MurhafSousli Can you please create a card for this?

@adrianq sure

tokland · 2017-09-11T09:20:56Z

src/List/ContextActions.js

@@ -9,14 +9,19 @@ config.i18n.strings.add('assignToOrgUnits');

 const contextActions = Action.createActionsFromNames([
  'details',
-  'assignToOrgUnits'
+  'assignToOrgUnits',


Do you see the icons in the app? I needed to change all "*" to "**" in webpack.config.js proxy routes for the proxy to work (webpack/webpack-dev-server#458 (comment))

tokland · 2017-09-11T09:23:41Z

Btw, did you see the icons? I had to replace * by ** in webpack proxied urls (webpack/webpack-dev-server#458 (comment))

MurhafSousli · 2017-09-11T09:57:49Z

@tokland I added material fonts manually in index.html to fix the icon in dev.

adrianq · 2017-09-24T19:09:13Z

@tokland are you fine with this PR? any pending change or something to discuss?

tokland · 2017-09-24T19:24:13Z

I don't see any changes commited in this branch from the original review, am I missing something?

I added material fonts manually in index.html to fix the icon in dev.

Did you try the proposal in my comment? it seemed easier to fix webpack.config.js.

MurhafSousli · 2017-09-24T20:42:34Z

@tokland Sorry I didn't notice that you requested a change. yea it is much better like this!

tokland · 2017-09-24T21:01:38Z

Cool! And what was the final decision about the spaces? the PR still has the mixture 2 (new) / 4 (old)

MurhafSousli · 2017-09-24T21:04:45Z

@tokland 4 spaces right! I did 4 in the next PR, but I didn't manage to finish it

adrianq

@MurhafSousli This code looks good to me. A couple of things...
Can you please change the indentation of the new code to 4 spaces? I know that you changed it in the next PR but if you can change it here as well it would definitely help to see the difference. Also, it shouldn't cause you any major conflict in your next PR.

In the card, we talked about two different options for assign org unit:
Assign OU Data Capture -> It is implemented
Assign OU Output -> It is implemented

Keep in mind the two of them should open the same graphical interface (layout): 'assign org units' That graphical interface is currently working so please double check it keeps working. We will make changes to that interface in another PR.

MurhafSousli · 2017-09-27T21:02:08Z

@adrianq sorry for the delay, I added 4 spaces to all files

adrianq · 2017-09-28T08:12:10Z

Thanks @MurhafSousli What about my second comment?

MurhafSousli · 2017-09-29T23:54:31Z

@adrianq For the context menu, we already have one of them which opens the dialog.

But which property does the current Assign OU refer to? Assign OU Data Capture? or Assign OU Output?

I can only think about duplicating this field and rename them to

Assign to organization units data capture
Assign to organization units output

Assuming that we get the OU data capture from { fields: ":all,assignToOrgUnits[id,path,displayName]" };

What should we use for OU data output?
I tried { fields: ":all,assignToOrgUnitsOutput[id,path,displayName]" }; didn't work

After checking your last commit again, I found out that my code was almost identical except the field was userGroups instead of userGroups[id,displayName,publicAccess]

This is so frustrating, There should be a documentation for this somewhere, the documentation you gave me was for the REST API not the JS client we use

MurhafSousli · 2017-10-04T06:15:01Z

@adrianq PR is ready for review

adrianq · 2017-10-04T22:25:58Z

@MurhafSousli Sorry for being picky but there a couple of things I would like you to polish:

If you look at the files comparison, there are a lot of files and bits of codes that aren't actually changed, just the format is changing. This may cause some issues (in general it is not considered a good practice): it is more difficult for us to review the code, if in the future we would like to compare it with the original app (organisation user assignment app) it is gonna be more difficult, if in the future I want to see the author of a line because is causing problems or whatever it is gonna be a nightmare, mixing formats, etc. Can you please clean up the code and just commit actual changes?
There is some code that doesn't have anything to do with this task https://github.com/EyeSeeTea/user-app/issues/9. For instance these lines https://github.com/EyeSeeTea/user-app/pull/11/files#diff-3f783c9074ac4c268b871dac8a540b69R64 I know you will be using this code in the next PR but, in any case, this code should be part of this PR.

Next tasks are much more difficult and can be a nightmare for the two of us if we are not particularly clean with the PRs. Hopefully, these changes shouldn't take you long. It would be great if we can close these PRs between tomorrow and the day after.

MurhafSousli · 2017-10-05T03:41:00Z

@adrianq I am sorry for the difficulties you are having in reviewing this PR. But do you remember when we talked about the 4 spaces? (A couple of comments above)

@MurhafSousli This code looks good to me. A couple of things...
Can you please change the indentation of the new code to 4 spaces? I know that you changed it in the next PR but if you can change it here as well it would definitely help to see the difference. Also, it shouldn't cause you any major conflict in your next PR.

So this is the result of adding 4 spaces. I literally sat the spaces to 4 and then I applied code organizing to all files.

As you see I am not committing all the changes in one giant commit. I made separate commits for this purpose. IMHO I think you should not review the code from the total file changes. instead, it is easier to review each commit on its own. The lines you highlighted are related to the Add 4 space commit (4abdafa) I believe.

If the code that was being pushed in earlier PRs was being pushed in a correct format, we wouldn't have seen these annoying changes. but code with extra spaces and wrong formatting will cause such issue and it needs to be fixed sooner or later. otherwise, we are keeping the unfixed code with us from PR to PR.

There is some code that doesn't have anything to do with this task #9

Sorry, I don't know how this happened, I probably made mistake while checking out between branches and forgot what branch I was working on. this mistake was produced in the commit Add group to the detail panel(1371215)

I honestly don't know how to clean up at this point due to my limited git skills. I don't think I can go back to that state. But I can undo the last commit and push it again into multiple commits if you want.

Would you like remove this PR and start a new one. or if you know a better way, can you guide me through the steps? thanks.

MurhafSousli added 5 commits September 9, 2017 09:49

organize code space

ad807e6

fix(Left click): display the details

b1188ad

update(contextMenu)

312cc67

- Assign roles -> alert - Assign groups -> alert - Edit User -> alert

Add english translation for the new context menu items

d7c3564

Add config.js to .gitignore

9006067

MurhafSousli requested a review from adrianq September 9, 2017 13:19

MurhafSousli added the testing label Sep 9, 2017

adrianq requested a review from tokland September 10, 2017 08:13

tokland reviewed Sep 11, 2017

View reviewed changes

fix(webpack proxy)

4506154

adrianq requested changes Sep 24, 2017

View reviewed changes

Use 4 spaces

4abdafa

Add groups the details panel

1371215

Add context menu item (assign to organization unit output)

9e84fde

MurhafSousli added 2 commits October 5, 2017 06:49

Add "package.lock.json" to .gitingore

f255650

Add OU Output context menu item

2b9e9f0

MurhafSousli and others added 6 commits October 5, 2017 06:54

Merge remote-tracking branch 'origin/fix/behaviour' into fix/behaviour

83288bb

fix format

01d2164

reverting formatting changes and removing user groups code

fbfe776

more formating

69f7673

remove redundant key and user groups key

02f4461

remove break line

40de8e3

adrianq merged commit 968ba5f into development Oct 6, 2017

adrianq removed the testing label Oct 6, 2017

adrianq deleted the fix/behaviour branch February 21, 2018 09:54

Fix/behaviour #11

Fix/behaviour #11

Uh oh!

Conversation

MurhafSousli commented Sep 9, 2017

Uh oh!

adrianq commented Sep 10, 2017

Uh oh!

tokland Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

MurhafSousli Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianq Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

tokland Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

tokland Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

adrianq Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

MurhafSousli Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

tokland Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

tokland commented Sep 11, 2017

Uh oh!

MurhafSousli commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrianq commented Sep 24, 2017

Uh oh!

tokland commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MurhafSousli commented Sep 24, 2017

Uh oh!

tokland commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MurhafSousli commented Sep 24, 2017

Uh oh!

adrianq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MurhafSousli commented Sep 27, 2017

Uh oh!

adrianq commented Sep 28, 2017

Uh oh!

MurhafSousli commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MurhafSousli commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrianq commented Oct 4, 2017

Uh oh!

MurhafSousli commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

MurhafSousli Sep 11, 2017 •

edited

Loading

MurhafSousli commented Sep 11, 2017 •

edited

Loading

tokland commented Sep 24, 2017 •

edited

Loading

tokland commented Sep 24, 2017 •

edited

Loading

adrianq left a comment •

edited

Loading

MurhafSousli commented Sep 29, 2017 •

edited

Loading

MurhafSousli commented Oct 4, 2017 •

edited

Loading

MurhafSousli commented Oct 5, 2017 •

edited

Loading