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

fix: Fetching analytics for analytical object with undefined aggregationType in plugin #232

Merged
merged 2 commits into from
Apr 3, 2019

Conversation

neeilya
Copy link
Contributor

@neeilya neeilya commented Apr 2, 2019

Problem:

When we pass full analytical to the plugin without aggregationType property specified, it passes it to analytics request.

As a result we have request like this:
localhost:8080/api/29/analytics.json?dimension=dx:ntzmpYRSKGg&dimension=ou:ImspTQPwCqd;LEVEL-3&filter=pe:2018&aggregationType=undefined&skipMeta=false&skipData=true&includeMetadataDetails=true

This request returns 500 server error.
Removing aggregationType=undefined from analytics URL makes request correct and it returns correct response.

Proposal:

To fix this we can check if option value (e.g. aggregationType) is undefined and if it is - check if defaultValue is/can be undefined. If the answer to both questions is yes, then assume we can assign undefined to option value, otherwise skip assigning (see files changed).

@neeilya neeilya requested a review from edoardo April 2, 2019 13:44
Copy link
Member

@edoardo edoardo left a comment

Choose a reason for hiding this comment

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

Perhaps instead of passing undefined in the query string, we can just skip those arguments.
I don't think that undefined is a meaningful value in the query string.
The 2 parameters that currently can be passed in the request are aggregationType and completedOnly and for both undefined does not make sense.

@neeilya
Copy link
Contributor Author

neeilya commented Apr 2, 2019

@edoardo

Perhaps instead of passing undefined in the query string, we can just skip those arguments.

Just to clarify, you suggest to remove/make non-requestable aggregationType and completedOnly from options object which we use to iterate through when we generate options for request?

@edoardo
Copy link
Member

edoardo commented Apr 2, 2019

What I mean is to only add aggregationType and completedOnly to the request arguments only if the value is not undefined and different from the default.

Basically change this condition:
if (visualization[option] !== props.defaultValue)
to
if (visualization[option] !== undefined && visualization[option] !== props.defaultValue)

@neeilya
Copy link
Contributor Author

neeilya commented Apr 2, 2019

@edoardo

Thanks, fix pushed.

@neeilya neeilya merged commit bfe41b4 into master Apr 3, 2019
@neeilya neeilya deleted the fix/aggregation-undefined-case branch April 3, 2019 08:32
dhis2-bot added a commit that referenced this pull request Feb 12, 2020
# [33.0.0](v32.0.3...v33.0.0) (2020-02-12)

### Bug Fixes

* @dhis2/analytics 2.6.11 ([#448](#448)) ([231bcd8](231bcd8))
* added icons to tooltip for warning and locked ([#447](#447)) ([26a95e8](26a95e8))
* added lint, commit message and test commit/push hooks ([#526](#526)) ([0292a63](0292a63))
* adjust options dialog for easier scanning ([#580](#580)) ([b0393ff](b0393ff))
* assigned categories and new dimension types ([#576](#576)) ([2faae50](2faae50))
* avoid infinite loading and make chart plugin use hooks (DHIS2-8290) ([#653](#653)) ([9de60dd](9de60dd))
* avoid React warning about required proptype ([#311](#311)) ([5dda862](5dda862))
* changed height to min-height for the axes area ([#361](#361)) ([8c0ed13](8c0ed13))
* changed incorrect prop types and tests ([#372](#372)) ([c96ac09](c96ac09))
* chip cursor ([#566](#566)) ([8701256](8701256))
* convert numeric options to strings to silence ui-core warnings ([#617](#617)) ([c3fe18d](c3fe18d))
* correctly set showDimensionLabels option ([#621](#621)) ([01427ac](01427ac))
* d2-ui-analytics 1.0.2 ([#277](#277)) ([62241f8](62241f8))
* dashboard items resize should trigger chart reload ([#282](#282)) ([86070ae](86070ae))
* dimension dialog update validation ([#486](#486)) ([429c51e](429c51e))
* dimensions panel divided in to sections ([#581](#581)) ([3a9627a](3a9627a))
* do not run default adapter for pivot tables ([#616](#616)) ([27d8ab7](27d8ab7))
* don't render pivot table before data has loaded ([#635](#635)) ([844d989](844d989))
* download menu options for PT ([#624](#624)) ([c131970](c131970))
* dynamic axis names based on vis type ([#623](#623)) ([24510e0](24510e0))
* equal padding for the AO title bar ([#567](#567)) ([17238ed](17238ed))
* Fetching analytics for analytical object with undefined aggregationType in plugin ([#232](#232)) ([bfe41b4](bfe41b4))
* Fetching analytics for year over year charts in plugin ([#231](#231)) ([16853ef](16853ef))
* fix prop type for value in SortOrder option component ([#568](#568)) ([8379621](8379621))
* fix styling issues in options ([#585](#585)) ([d4c5bc6](d4c5bc6))
* fix values for topLimit option ([#579](#579)) ([3d249a6](3d249a6))
* fixes for DND between axes ([#554](#554)) ([886de23](886de23))
* fixes for options with numeric values and toggleable Select ([#583](#583)) ([8a40296](8a40296))
* gauge now follows the behaviour of single value instead of pie ([#489](#489)) ([9969e61](9969e61))
* gauge plot lines and range values in options ([#654](#654)) ([7736b29](7736b29))
* i18n merge conflict ([ac8c383](ac8c383))
* impl max dims rule ([#517](#517)) ([241de69](241de69))
* implemented the error code for assigned categories ([#557](#557)) ([7b9f52b](7b9f52b))
* layout for pivot tables ([#577](#577)) ([033be21](033be21))
* loading spinner for plugins (DHIS2-8117) ([#587](#587)) ([f8be30b](f8be30b))
* locked dims not array ([#491](#491)) ([15ae8a8](15ae8a8))
* long names for dimensions, chip and tooltip (DHIS2-7932) ([#556](#556)) ([2212398](2212398))
* manifest credentials bug in chrome ([#233](#233)) ([a0032e0](a0032e0))
* map is not vis type ([#371](#371)) ([22c36e2](22c36e2))
* merge conflicts ([3eb1c77](3eb1c77))
* only point series to axes for certain types ([#264](#264)) ([24b6ac0](24b6ac0))
* only run empty check after all values have been added ([#627](#627)) ([75b6ae4](75b6ae4))
* open modal for empty dims ([#625](#625)) ([e07d786](e07d786))
* options with numeric values (sortOrder) ([#564](#564)) ([94ad11d](94ad11d))
* org unit selector - only act on path if it exists ([#276](#276)) ([9299038](9299038))
* pie tooltip DHIS2-7532 ([#330](#330)) ([72eb5db](72eb5db))
* pin to analytics@2.6.11 ([#495](#495)) ([5f21406](5f21406))
* pin to analytics@2.6.11 ([#495](#495)) ([25b4545](25b4545))
* properly set layout on vis type change for all vis types ([#586](#586)) ([489fbf9](489fbf9))
* provide useful error messages (DHIS2-5029) ([#552](#552)) ([ef16c68](ef16c68))
* remove api.baseUrl duplicate ([#573](#573)) ([ac05af6](ac05af6))
* remove code climate config for now ([80c891a](80c891a))
* remove colon from chip if no selection ([#312](#312)) ([e667134](e667134))
* remove custom title when Auto is selected (DHIS2-8252) ([#636](#636)) ([44ed1a5](44ed1a5))
* remove ripple effect in dimension list when drag starts/ends ([#584](#584)) ([c91ed3b](c91ed3b))
* rename axis name constants ([#445](#445)) ([b75967a](b75967a))
* rules dim handling ([#446](#446)) ([445f1d8](445f1d8))
* single value without dx items ([#313](#313)) ([f1fbe22](f1fbe22))
* start screen icons ([#558](#558)) ([841c9c7](841c9c7))
* swap full axis dims to filter ([#518](#518)) ([a79e329](a79e329))
* tooltip message ([#449](#449)) ([ca55b7e](ca55b7e))
* turn class vars into functions ([#569](#569)) ([236885b](236885b))
* ui glitches ([#618](#618)) ([2788daf](2788daf))
* ui updates to the start screen ([#548](#548)) ([20dd6ca](20dd6ca))
* ui updates to the start screen ([#548](#548)) ([fee2153](fee2153))
* update @dhis2/analytics dependency ([#319](#319)) ([5752bea](5752bea))
* update analytics and plugin dep ([#324](#324)) ([8bb0202](8bb0202))
* update charts api dep ([#297](#297)) ([ab13e1c](ab13e1c))
* update gradient colors check ([#261](#261)) ([b39cec2](b39cec2))
* update plugin dependency ([#318](#318)) ([452a287](452a287))
* update plugin version ([#320](#320)) ([3921334](3921334))
* update recuder to handle empty axes ([#630](#630)) ([077dd68](077dd68))
* updated proptype for error ([#553](#553)) ([21a9b4a](21a9b4a))
* upgrade @dhis2/analytics to fix dimension dialog styling ([#551](#551)) ([c90f2b8](c90f2b8))
* upgrade @dhis2/cli-app-scripts and use platform-provided height ([#550](#550)) ([b6ffdfb](b6ffdfb))
* upgrade cli-app-scripts to fix package resolution ([#540](#540)) ([a52f7f3](a52f7f3))
* use a doc-like icon for PT downloads ([#626](#626)) ([a407372](a407372))
* use axisName instead of axisKey ([204fe7a](204fe7a))
* use layout type to get the right layout comp ([#563](#563)) ([17c26ba](17c26ba))
* use locked dimensions rule from analytics ([#444](#444)) ([883304f](883304f))
* use max items rule ([#373](#373)) ([ded0ee9](ded0ee9))
* use shared vis types ([#368](#368)) ([26d12c6](26d12c6))
* viewport layout with visible interpretations panel ([#323](#323)) ([2e2803b](2e2803b))
* vis type imports ([#496](#496)) ([2a1daaf](2a1daaf))

### chore

* **github-actions:** add workflows for lint and publish ([#638](#638)) ([739bb13](739bb13))

### Features

* add dnd from dimensions to specific position in layout axis ([#575](#575)) ([7c573b7](7c573b7))
* assigned categories DHIS2-7701 ([#539](#539)) ([636b365](636b365))
* drag 'n drop for reordering dimension items in layout ([#519](#519)) ([efd4acb](efd4acb))
* dual axis support ([#239](#239)) ([a50075c](a50075c))
* multiple orgunit roots support ([#328](#328)) ([e85668f](e85668f))
* show most viewed saved AO (DHIS2-7835) ([#547](#547)) ([97b0622](97b0622))
* summarize chart filters that have orgunit levels and/or groups ([#298](#298)) ([e5e0a7b](e5e0a7b))
* use layout rules for add-to-layout ui ([#362](#362)) ([a3dbb9f](a3dbb9f))
* use new pivot engine ([#574](#574)) ([f4ccef1](f4ccef1))
* WIP pivot table type ([#335](#335)) ([626f447](626f447))

### Reverts

* Revert "fix: use lerna to share code instead of copying resources (#214)" (#216) ([de00b4f](de00b4f)), closes [#214](#214) [#216](#216)

### BREAKING CHANGES

* **github-actions:** Ensure that the plugin and app versions are locked to each other.
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 33.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

dhis2-bot added a commit that referenced this pull request Jun 8, 2020
# [32.1.0](v32.0.3...v32.1.0) (2020-06-08)

### Bug Fixes

* upgrade d2-ui dependencies for latest translations [v32] [DHIS2-8892] ([#1061](#1061)) ([6bc2f8c](6bc2f8c))
* **translations:** sync translations from transifex (v32) ([ff4839e](ff4839e))
* **translations:** sync translations from transifex (v32) ([bb026d0](bb026d0))
* **translations:** sync translations from transifex (v32) ([c15e3b6](c15e3b6))
* **translations:** sync translations from transifex (v32) ([0620c69](0620c69))
* @dhis2/analytics@2.4.9 ([4ed10df](4ed10df))
* allow : after bold and italic ([#314](#314)) ([59aed20](59aed20))
* changed height to min-height for the axes area ([#361](#361)) ([#366](#366)) ([5848460](5848460))
* d2-charts-api@32.0.9 ([27eff19](27eff19))
* deep check styles for changes ([#283](#283)) ([e76555c](e76555c))
* display 'and x others...' on tooltip (DHIS2-8753) v32 backport ([#945](#945)) ([0590fdf](0590fdf)), closes [#927](#927) [#925](#925)
* epi curve bug (v32) ([#529](#529)) ([2908206](2908206))
* epi curve bug v32 (v2) ([#530](#530)) ([de40d3d](de40d3d))
* Fetching analytics for analytical object with undefined aggregationType in plugin ([#232](#232)) ([bfe41b4](bfe41b4))
* Fetching analytics for year over year charts in plugin ([#231](#231)) ([16853ef](16853ef))
* manifest credentials bug in chrome ([#233](#233)) ([a0032e0](a0032e0))
* merge conflict ([#545](#545)) ([8e5f7aa](8e5f7aa))
* pie tooltip v32 ([#332](#332)) ([77b2628](77b2628))
* plugin - add deep checks to determine whether to redraw chart  ([#284](#284)) ([4b8837e](4b8837e))
* remove api.baseUrl duplicate ([#571](#571)) ([fafd58e](fafd58e))
* v32 backport, upgraded to Analytics v2.4.7 to support long dimension names (DHIS2-7932) ([#622](#622)) ([2cd524a](2cd524a))
* working translations [v32] ([#1001](#1001)) ([c32c623](c32c623))

### Features

* dual axis support ([#239](#239)) ([a50075c](a50075c))
* multiple org units support ([#333](#333)) ([6392efa](6392efa))

### Reverts

* Revert "fix: use lerna to share code instead of copying resources (#214)" (#216) ([de00b4f](de00b4f)), closes [#214](#214) [#216](#216)
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 32.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 33.1.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants