V2.3 from atolcd by ftavin · Pull Request #106 · sigmah-dev/sigmah

ftavin · 2017-10-27T15:04:36Z

No description provided.

ghost · 2017-11-03T09:13:53Z

src/main/resources/db/migration/V22__add_contact_list_element_refs_checkbox_element.sql

@@ -0,0 +1,7 @@
+ALTER TABLE public.contact_list_element


Please, remove hard-coded schema prefix public.

ghost · 2017-11-03T09:20:18Z

src/main/java/org/sigmah/shared/dto/element/ContactListElementDTO.java

+  public void setCheckboxElement(CheckboxElementDTO checkboxElement) {
+		set(CHECKBOX_ELEMENT, checkboxElement);
+	}
+


Please, fix indentation of getCheckboxElement and setCheckboxElement: (hint: @xavier-calland should ckeck his IDE configuration 😉 )

ghost · 2017-11-03T09:22:07Z

src/main/java/org/sigmah/server/service/ContactHistoryService.java

      FlexibleElement flexibleElement = flexibleElementDAO.findById(historyToken.getElementId());
      // We are looking here for relationships going on the current contact
      // The flexible element must be a contact list element
+      // This should have already been filtered by the request.


Instead of a comment, could you thow an IllegalStateException instead of just continue? This way, we may not ignore a bug in findByIdInSerializedValueAndElementType.

ghost · 2017-11-03T09:24:32Z

src/main/java/org/sigmah/server/dao/impl/HistoryTokenHibernateDAO.java

+        "OR ht." + EntityConstants.HISTORY_TOKEN_COLUMN_VALUE + " ~ ('^(.*~)?'||:id||'(~.*)?$') ) " +
+        (elementTypeTableName == null ? "" : "AND EXISTS (SELECT * FROM " + elementTypeTableName + ") ") +
+        "ORDER BY ht." + EntityConstants.HISTORY_TOKEN_COLUMN_DATE + " DESC" +
+        (lastOnly ? " LIMIT 1" : ""), HistoryToken.class)


Why do you use a LIMIT sql statement instead of using Hibernate setMaxResult(1) (like in the above method)?

ghost · 2017-11-03T09:26:59Z

src/main/java/org/sigmah/server/dao/impl/FrameworkFulfillmentHibernateDAO.java

+            "JOIN pm.frameworkFulfillments ff " +
+            "JOIN FETCH ff.frameworkElementImplementations fei " +
+            "WHERE pm.id = :projectModelId " +
+            "AND ff.framework.availabilityStatus = 'AVAILABLE' ",


'AVAILABLE' should not be hard-coded but be passed as an Hibernate parameter with setParameter. Also, it ease "find usages". Same problem in the method below.

ghost · 2017-11-03T09:30:57Z

...gmah/client/ui/presenter/admin/models/importer/AddImportationSchemeModelsAdminPresenter.java

-				} else {
+				} else if (model instanceof  OrgUnitDTO) {
 					newImportationSchemeModelProperties.put(AdminUtil.ADMIN_ORG_UNIT_MODEL, model);
+				} else {


can you be more precise about your expectations with an else if (model instanceOf ContactDTO) (or whatever suitable class) and finishing by a catchall else throwing an IllegalArgumentException?

Same issue line 200.

Also, rename the method and make it start with a lowercase.

ghost · 2017-11-03T09:34:59Z

src/main/java/org/sigmah/shared/computation/instruction/Pick.java

+	 */
+	@Override
+	public void execute(Stack<ComputedValue> stack, Map<Dependency, ComputedValue> variables) {
+		// No-op


I don't get it. Can you add a Javadoc in this class explaining what is the purpose of this new "pick" function?

ghost · 2017-11-03T09:37:11Z

src/main/java/org/sigmah/shared/command/GetContactByNameOrEmail.java

+    return allowedModelIds;
+  }
+
+  public Set<Integer> getExcludedIds() { return excludedIds; }


Please indent (seems that Thibault's IDE is also doing strange things)

osarrat · 2017-11-08T16:55:53Z

Hi, thanks for this pull request !
I would have a question: according to the date of your commits which seem to all date from the 7th of September or earlier (except the commits made for this pull request and some technical changes afterwards), it seems that you couldn't yet find the time to tackle all the functional feedbacks I gave on v2.3-alpha between the 12th and the 19th of September. Can you confirm to me that those feedbacks remain to be tackled ?

ftavin · 2017-11-10T12:56:40Z

Hi Olivier Thibault is currently correcting our pull request according to Brendan's comments. I will personnaly check your previous remarks afterwards. Best regards 2017-11-08 17:55 GMT+01:00 Olivier Sarrat <notifications@github.com>:

…

Hi, thanks for this pull request ! I would have a question: according to the date of your commits which seem to all date from the 7th of September or earlier (except the commits made for this pull request and some technical changes afterwards), it seems that you couldn't yet find the time to tackle all the functional feedbacks I gave on v2.3-alpha between the 12th and the 19th of September. Can you confirm to me that those feedbacks remain to be tackled ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#106 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APGcDbYC-e4f_P0KQTDycIzc1xyP9pZfks5s0d0agaJpZM4QJMZH> .

-- François TAVIN Chef de projet Atol Conseils et Développements t. 03 80 68 81 68 | e. f.tavin@atolcd.com | www.atolcd.com a. ZAE Les Terres D'Or, 21220 Gevrey-Chambertin <https://www.linkedin.com/company/atol-conseils-et-d-veloppements> <https://www.facebook.com/atolcd/> <https://blog.atolcd.com/> <https://www.youtube.com/user/atolcd> <http://www.slideshare.net/CarolineChanlon> <https://twitter.com/atolcd>

Change-Id: Ic762723c7eaa8bbec476826fa6bae465e7201313

ghost · 2017-11-14T12:58:54Z

it seems that you couldn't yet find the time to tackle all the functional feedbacks I gave on v2.3-alpha between the 12th and the 19th of September. Can you confirm to me that those feedbacks remain to be tackled ?

@osarrat is the PR OK regarding those feedbacks?

osarrat · 2017-11-14T14:41:47Z

Hi @numero-six ,
I had today a phone meeting with @ftavin .
He will the fix the remaining conflict soon. After that, you can merge the pull request, and AtolCD's team will open a new PR or the fix of the feedbacks I made.
Best !

- sql migration script - update domain object (refs #928) Change-Id: I4c87fbb26f963aa0bc86c3a6cd19da12430e926a

(refs #928) Change-Id: I2b36bd910c3e7c54804760a88f2aa804d3652443

…ions Force null value for iteration_type field if not has_iterations on save (refs #928) Change-Id: I77df31b6f01b1a98cf70df4ace76c50559bf8918

Use iteration_type if not null in UI and exports (refs #928) Change-Id: Ifb19ae135970ad58940f429dbf055d0399a9df8e

(ref #892) Change-Id: Ib177cf0051230864b5504331c67fc7226c35f00a

(ref #892) Change-Id: Ife2e21deab9c037f5d4cac3c32421fd610384e16

(ref #892) Change-Id: I6de86ad3c1cd2b3a8cd59182541afcbe6dd2a76d

SQL requests were always retrieving the whole history even when only the last result was actually needed, filtering the whole list afterwards. They now retrieve only the last item or each type (property or relation change) and then pick the last of them. This additionally requires filtering relation changes by type at request time rather than as post processing the results. Also, the ContactListWidget on the dashboard was retrieving the whole history for each contact whereas it's only interested in the last item; and the mechanism for retrieving only the last change was already there, just not used. Change-Id: I358260fa171d964876484244b17e33f671da7f52

Create a specific command to retrieve the latest history for a list of contact (refs #1016) Change-Id: I8e4789c1b22133e473aa71332c90c619d040badc

Load all contacts in one query, then iterate over contacts to find its history (refs #1016) Change-Id: If86efe7bbb38f62305b4ddec71e9d5daa1285f7c

Load the latest history for all flexible elements of a contact (refs #1016) Change-Id: I64923fe0c6f1a1ad0a544af691d85599ebdcf129

Explicitly load and set secondary OrgUnit to avoid a query for every contact (refs #1016) Change-Id: Ie8e1892db00c253c4323aaa3386b4d58d4adcadc

Change-Id: Ic66b5d57c96157a569ea3ef413b63bd3b9b4990d

SQL migration script Add field in domain and DTO objects (ref #1018) Change-Id: Id14b64c93be4fab4e4c958e4f01b07a4a2798202

(ref #1018) Change-Id: I3e2de8731b36cb25d7427b28269289bab57df80f

We need contactModel fields to edit contactListElement referencing checkBoxElement (ref #1018) Change-Id: I0c1fd0169080bcf1d3f8de93a207496cd813b4ac

…Element The combobox is visible when exactly one ContactModel is selected Allowed values are all active checkboxElement of the ContactModel (ref #1018) Change-Id: I58f4910a2baad11b12ecd869d62070b24d1cb78d

…the ContactListeElement Created contacts in the contactListCombobox will have the checkboxElement value set to TRUE (ref #1018) Change-Id: I38b4e942dd1fb4145c3a1e22a543995d7e3bf6d4

(ref #933) Change-Id: Iae3ac3c2e9ba003790fcd362a77e88837db90583

(ref #933) Change-Id: I7e1f66a3d786cec5e144eea3a07a0f8ff4d791e4

(ref #933) Change-Id: I23be1ddd0a895d310be44e074e973d5d5f1be5c2

- pass groupId in formula validation - pass iterationId on formula computation (refs #1038) Change-Id: Ifdee277c7c44ea80b3dde8a591f03470339bdeeb

* most GridColumnRenderers were useless and actually harmful for performance. By default, a column's value is displayed as-is (dates and numbers are formatted using a ColumnConfig-assigned formatter, and that one was correctly set, at least of the change date column), so there's no need to use a renderer just to return the value as-is (or formatted in case of the date). Morever, renderers wrapped the value in Html widgets, which GXT's GridView will have to queue into a list and later attach to the rendered HTML table (the table is rendered as a string that's eventually parsed by innerHTML); vs. just adding the value inline into the rendered string. * most calls to setRenderer were done on the firstnameColumn (or emailColumn) instead of the actually intended ColumnConfig. This wasn't a problem in practice because most renderers were useless (as explained above), and fortunately the last renderer assigned to the column was expecting the appropriate type (otherwise we'd have had ClassCastExceptions at runtime, which incidentally is what I got after fixing the bad assignments, and why I investigated and how I found the above uselessness and harmfulness of our renderers.) In the end, the only renderers left are the one for the contact type (but which now returns a String rather than an Html widget), and the contact name, which returns a link eventually triggering a navigation to the contact details. Change-Id: I82f0766f30b9375a263b260838b9f07f67a2248a

Use a table with same layout as files list field, but use the same icon as ListComboBox to remove an item, and don't ask user confirmation (like ListComboBox). Only display information from contacts (not from contact history like in dashboard). Contact name is a link to the related contact page. Adding or creating a contact is still done with a combo box with an "Add" button, and a "Create" button displaying a popup. (refs #910) Change-Id: I9fff3cfbc03d74c5fcfde2120338ded23f033bcc

Move the combo box to a popup. For now the combo box still loads the whole contact list (and does so each time you want to add a contact), but will soon be replaced with server-side search. Change-Id: I508c49c064ccb114b575e36628873fe37b6373e2

(refs #634) Change-Id: I85652eace8669256a4a2f3202cbfe47cfad0d4c6

Elements can't change group for an iterative group Element in an iterative group can't leave its group and container Iterative groups can't be switched to non/iterative Fix layout groups loading in combobox Change-Id: I4a9a542c2fda734c885f5595d2ca0a68bda4b750

Avoiding nullpointers after requests that return no result Fix creation from contact if contact has no parent organization Change-Id: I02c9f271a341f12f612ff0d4f24393c09e709a0f

Change-Id: I8898bfb2326269bfb0a9f587ed65015786e7c4c4

Adding a request to search contacts with a character chain (e.g from a textfield) The request looks for results in both contact and user tables Results can be filtered on contact models and with exclusions (e.g contacts already in use) Change-Id: I8035fb59dadbece093cd6672edb3d877e120f199

Replace the combo box with a text field, search button and result grid from which to select a contact. Change-Id: I1492209965ab6b7086cd91ef5488a280d9a8f412

Allow to show a simple string value The value can be changed by clicking the button (e.g opens a popup) Change-Id: I297ff29e20c4c9cbdd343daf24ac2bf627a2ad70

Allows to search contacts without related user (ref #1016) Change-Id: I2c8e629259f9b502e3103b5c248ad4c020ed8276

Use contact search popup instead of the previous combobox with formatted result list (ref #1016) Change-Id: Iefaed63c34f9e38e0d0ca0d790b102daf031cde4

Removing combobox with full list of organization contacts Adding a button that opens a popup to search organizations (ref #1016) Change-Id: I27ea90b0d8cba1ded366037f4113056a553a1218

Change-Id: I4cbcd79c40503d4bbfaebd76d5c07d39d745422e

(resolves #634) Change-Id: I445c56459bc199dd5fcdb9e90183a53b3af64edb

(resolves #634) Change-Id: I883fc4e0d6ecce1cd64ac41f68b4c2d338397697

Change-Id: I4888a97290f8f0906cf795e9e55810c4bcd0568e

It only appens in unit tests Change-Id: Ic5f02136a3e1528686cf2f91bd0dce835a133858

Change-Id: Ief72ce50cf3d64af189b3638e3ee38178aea0be8

(refs #1038) Change-Id: Iba57487d451ee7a3524a1ca893a4f0ac319dc64f

Change-Id: Ic762723c7eaa8bbec476826fa6bae465e7201313

Change-Id: Ieed6dab8f75ac85b13ce70dafedd30d1448bf327

ftavin · 2017-11-14T18:42:28Z

Pull request rebased to handle merge conflict.

Brendan, It's up to you now !

osarrat · 2017-11-17T14:50:15Z

Hi @numero-six !
This PR can be merged now. The codacy checks are considered secondary by me and @ftavin .
Thanks for this work all of you !

ghost suggested changes Nov 3, 2017

View reviewed changes

atolcd-gerrit pushed a commit to atolcd/urd-sigmah that referenced this pull request Nov 14, 2017

Fixes pull request sigmah-dev#106

12ed347

Change-Id: Ic762723c7eaa8bbec476826fa6bae465e7201313

ghost approved these changes Nov 14, 2017

View reviewed changes

xavier-calland and others added 23 commits November 14, 2017 16:00

Add layout_group.iteration_type field

14be9b8

- sql migration script - update domain object (refs #928) Change-Id: I4c87fbb26f963aa0bc86c3a6cd19da12430e926a

Add input for layout_group.iteration_type field

652cac1

(refs #928) Change-Id: I2b36bd910c3e7c54804760a88f2aa804d3652443

Disable input for layout_group.iteration_type field if not has_iterat…

dabeab9

…ions Force null value for iteration_type field if not has_iterations on save (refs #928) Change-Id: I77df31b6f01b1a98cf70df4ace76c50559bf8918

Change iterable layout_group label

48eaf14

Use iteration_type if not null in UI and exports (refs #928) Change-Id: Ifb19ae135970ad58940f429dbf055d0399a9df8e

Add refresh action in ContactList

6488031

(ref #892) Change-Id: Ib177cf0051230864b5504331c67fc7226c35f00a

Add ContactList cache in the dashboard

1a0ac43

(ref #892) Change-Id: Ife2e21deab9c037f5d4cac3c32421fd610384e16

Hide default refresh button in contact list paging toolbar

8ed761d

(ref #892) Change-Id: I6de86ad3c1cd2b3a8cd59182541afcbe6dd2a76d

Batch load contact history in ContactListView

1a76528

Create a specific command to retrieve the latest history for a list of contact (refs #1016) Change-Id: I8e4789c1b22133e473aa71332c90c619d040badc

Improve load of latest history of contacts

b6c09c4

Load all contacts in one query, then iterate over contacts to find its history (refs #1016) Change-Id: If86efe7bbb38f62305b4ddec71e9d5daa1285f7c

Improve load of latest history for contact values

20e4f1b

Load the latest history for all flexible elements of a contact (refs #1016) Change-Id: I64923fe0c6f1a1ad0a544af691d85599ebdcf129

Improve load of Contacts

6702f7c

Explicitly load and set secondary OrgUnit to avoid a query for every contact (refs #1016) Change-Id: Ie8e1892db00c253c4323aaa3386b4d58d4adcadc

Fix contactModel verification when contactDuplication check

7305577

Change-Id: Ic66b5d57c96157a569ea3ef413b63bd3b9b4990d

Add a reference to a checkboxElement in a contactListElement

6c18eae

SQL migration script Add field in domain and DTO objects (ref #1018) Change-Id: Id14b64c93be4fab4e4c958e4f01b07a4a2798202

Handle checkboxElement reference when saving a contactListElement

400b2ed

(ref #1018) Change-Id: I3e2de8731b36cb25d7427b28269289bab57df80f

Add a parameter on GetContactModels to retrieve all fields

034eb84

We need contactModel fields to edit contactListElement referencing checkBoxElement (ref #1018) Change-Id: I0c1fd0169080bcf1d3f8de93a207496cd813b4ac

Add a combobox to select a checkboxElement when editing a contactList…

69ae9ef

…Element The combobox is visible when exactly one ContactModel is selected Allowed values are all active checkboxElement of the ContactModel (ref #1018) Change-Id: I58f4910a2baad11b12ecd869d62070b24d1cb78d

Filter contactListCombobox values if a checkboxElement is defined in …

a0a6920

…the ContactListeElement Created contacts in the contactListCombobox will have the checkboxElement value set to TRUE (ref #1018) Change-Id: I38b4e942dd1fb4145c3a1e22a543995d7e3bf6d4

Add a maintenance alert in the contact detail page

02a2acd

(ref #933) Change-Id: Iae3ac3c2e9ba003790fcd362a77e88837db90583

Update contact model status from READY to USED when creating a contact

77fee41

(ref #933) Change-Id: I7e1f66a3d786cec5e144eea3a07a0f8ff4d791e4

Do not propose ContactModel under maintenance when creating a contact

57584bd

(ref #933) Change-Id: I23be1ddd0a895d310be44e074e973d5d5f1be5c2

Handle computation fields in iteration groups

1e994a3

- pass groupId in formula validation - pass iterationId on formula computation (refs #1038) Change-Id: Ifdee277c7c44ea80b3dde8a591f03470339bdeeb

tbroyer and others added 21 commits November 14, 2017 16:08

Improve contact list field usability (2)

1e8be04

Move the combo box to a popup. For now the combo box still loads the whole contact list (and does so each time you want to add a contact), but will soon be replaced with server-side search. Change-Id: I508c49c064ccb114b575e36628873fe37b6373e2

Add Contact export in Frameworks

89e839d

(refs #634) Change-Id: I85652eace8669256a4a2f3202cbfe47cfad0d4c6

Fix contacts list improvements

cc6c30a

Avoiding nullpointers after requests that return no result Fix creation from contact if contact has no parent organization Change-Id: I02c9f271a341f12f612ff0d4f24393c09e709a0f

Fix : removing Guava use in contact duplication

b9bbc31

Change-Id: I8898bfb2326269bfb0a9f587ed65015786e7c4c4

Improve contact list field usability (3)

5e11a03

Replace the combo box with a text field, search button and result grid from which to select a contact. Change-Id: I1492209965ab6b7086cd91ef5488a280d9a8f412

Creating a label field with a button

7bb1aeb

Allow to show a simple string value The value can be changed by clicking the button (e.g opens a popup) Change-Id: I297ff29e20c4c9cbdd343daf24ac2bf627a2ad70

Adding withoutUser parameter to contact search

3453bf9

Allows to search contacts without related user (ref #1016) Change-Id: I2c8e629259f9b502e3103b5c248ad4c020ed8276

Changing contact creation by email popup

30a7823

Use contact search popup instead of the previous combobox with formatted result list (ref #1016) Change-Id: Iefaed63c34f9e38e0d0ca0d790b102daf031cde4

Changing organization combobox in contact details

22c1cfb

Removing combobox with full list of organization contacts Adding a button that opens a popup to search organizations (ref #1016) Change-Id: I27ea90b0d8cba1ded366037f4113056a553a1218

Fix contact loading w/ orgUnits

27c5751

Change-Id: I4cbcd79c40503d4bbfaebd76d5c07d39d745422e

Add importation schemes tab to admin of contact models

81e9b5b

(resolves #634) Change-Id: I445c56459bc199dd5fcdb9e90183a53b3af64edb

Modify general importation mechanism to include contact imports

f8ca348

(resolves #634) Change-Id: I883fc4e0d6ecce1cd64ac41f68b4c2d338397697

Fix details to please Codacy

1694c23

Change-Id: I4888a97290f8f0906cf795e9e55810c4bcd0568e

Handle element with no group in computations

86f1da6

It only appens in unit tests Change-Id: Ic5f02136a3e1528686cf2f91bd0dce835a133858

Adding missing organization search in contact list element

65a5150

Change-Id: Ief72ce50cf3d64af189b3638e3ee38178aea0be8

Add javadoc to Pick function

68e567b

(refs #1038) Change-Id: Iba57487d451ee7a3524a1ca893a4f0ac319dc64f

Fixes pull request sigmah-dev#106

ce776dc

Change-Id: Ic762723c7eaa8bbec476826fa6bae465e7201313

Codacy fixes

e80077c

Change-Id: Ieed6dab8f75ac85b13ce70dafedd30d1448bf327

atolcd-gerrit force-pushed the v2.3-atol branch from 55a89d6 to e80077c Compare November 14, 2017 15:19

ghost merged commit 7337127 into sigmah-dev:master Nov 20, 2017

ftavin mentioned this pull request Dec 12, 2017

First sprint from AtolCD #23

Merged

This pull request was closed.

Conversation

ftavin commented Oct 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Nov 3, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osarrat commented Nov 8, 2017

Uh oh!

ftavin commented Nov 10, 2017 via email

Uh oh!

ghost commented Nov 14, 2017

Uh oh!

osarrat commented Nov 14, 2017

Uh oh!

ftavin commented Nov 14, 2017

Uh oh!

osarrat commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ghost Nov 3, 2017 •

edited by ghost

Loading