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

analytics - fixing filtering by roles #2698

Merged
merged 2 commits into from
Sep 6, 2019

Conversation

pmauduit
Copy link
Member

See #2685 ;

Note: this PR introduces some integration tests on the analytics webapp, inspired from the ones introduced in 19.04 for the console, but:

  • Integration tests follow the same conventions as the regular default maven failsafe plugin (code into src/test instead of src/it)
  • The introduced IT makes use of the exact same spring beans configuration as the one used in the webapp in production (ws-servlet.xml)
  • We still have to activate the "it" profile to make use of the postgresql docker image.

Tests:

  • mvn test ok
  • mvn verify -Pit ok

Credits goes to @groldan for the help !

@pmauduit pmauduit requested a review from groldan August 29, 2019 09:03
@@ -1,599 +1,704 @@
<?xml version="1.0"?>
Copy link
Member Author

Choose a reason for hiding this comment

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

I did not expect such a reindentation on the pom, sorry about that. Maybe we shall stick with 2 spaces on xml files ?

@fvanderbiest fvanderbiest added this to the 19.06 milestone Sep 3, 2019
@fvanderbiest fvanderbiest added this to In progress in Geo2France via automation Sep 3, 2019
@fvanderbiest fvanderbiest self-requested a review September 3, 2019 13:39
Geo2France automation moved this from In progress to Reviewer approved Sep 3, 2019
@fvanderbiest
Copy link
Member

When merged, this will require to merge 18.06 into 19.04 into master ...

Geo2France automation moved this from Reviewer approved to Needs review Sep 4, 2019
Copy link
Member

@groldan groldan left a comment

Choose a reason for hiding this comment

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

Looks good overall. Please check this branch for some fixes/improvements: https://github.com/groldan/georchestra/tree/pmauduit-it-analytics-2685

Had to test it out thoroughly to catch those hence the commits instead of just comments.
It'd be good if you can squash all commits in the PR into a single one and provide a descriptive commit message of what it encompasses functionality wise.

@groldan
Copy link
Member

groldan commented Sep 4, 2019

When merged, this will require to merge 18.06 into 19.04 into master ...

The correct command would then be:

git checkout 19.04
git merge --strategy-option ours 18.06

You'll get a number of files deleted in 19.04, so do:

git mergetool

And select the d option when prompted for what to do with each deleted file, to keep'em deleted
now git merge --continue and voilà

Since the group -> role renaming in georchestra, this feature (being
able to filter by roles via a parameter) has been probably broken.

This also adds an integration test to validate the current fix.
@pmauduit
Copy link
Member Author

pmauduit commented Sep 4, 2019

It'd be good if you can squash all commits in the PR into a single one and provide a descriptive commit message of what it encompasses functionality wise.

Done

private static boolean dataLoaded = false;

public @Before void before() throws Exception {
if (!dataLoaded) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think @groldan provided a better approach while working on GPDR:

https://github.com/georchestra/georchestra/pull/2613/files#diff-0cdf168c2c66425c514a83589eeddd9dR62

The annotation comes from com.github.database.rider and should do the work better than I can do in this before() method

Copy link
Member

Choose a reason for hiding this comment

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

IIUC, you do not consider this one is ready for merge ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering if it was worth to integrate this database-rider dependency (on a test scope) or not. In the current state, it works, it would be fancier with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did some rapid tests with database-rider (the DataSet annotation), as well as the Sql from spring-tests, but with no luck, I cannot get them work to load my fixtures.

Copy link
Member Author

Choose a reason for hiding this comment

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

trying harder I succeeded \o/ I'll update my PR actually

@fvanderbiest fvanderbiest merged commit 1638c92 into georchestra:18.06 Sep 6, 2019
Geo2France automation moved this from Needs review to Done Sep 6, 2019
@fvanderbiest
Copy link
Member

fvanderbiest commented Sep 6, 2019

When merged, this will require to merge 18.06 into 19.04 into master ...

Thanks Gabriel. I successfully merged 18.06 into 19.04.
I gave up merging 19.04 into master, there are too many conflicts that I was not able to resolve.
Asking for help here... @cmangeat @groldan

@fvanderbiest fvanderbiest modified the milestones: 19.09, 19.12 Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Geo2France
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants