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(user profile): fix user profile queries to support postgresql #19519

Merged
merged 4 commits into from Jan 9, 2023

Conversation

AHasanin
Copy link
Contributor

@AHasanin AHasanin commented Jan 8, 2023

Problems:

  • When loading user profile it calls function get_user_rank it contain two queries which group records by user in case of postgres the field user is capitalized into 'USER' making it a different field than the one selected as in the following Image:
    rank_function_postgres
  • In get_energy_points_heatmap_data function unix_timestamp, subdate don't exist in postgresql.

Solutions:

  • Include the table name with the user field so it wont be capitalized by postgresql engine.
  • replace unix_timestamp function with epoch in case of postgresql.
  • remove subdate and used INTERVAL directly in case of postgresql.

@AHasanin AHasanin requested a review from a team as a code owner January 8, 2023 14:27
@AHasanin AHasanin requested review from shariquerik and removed request for a team January 8, 2023 14:27
@github-actions github-actions bot added the add-test-cases Add test case to validate fix or enhancement label Jan 8, 2023
@codecov
Copy link

codecov bot commented Jan 8, 2023

Codecov Report

Merging #19519 (075a2d7) into develop (8273c9f) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #19519      +/-   ##
===========================================
+ Coverage    65.54%   65.57%   +0.02%     
===========================================
  Files          755      755              
  Lines        73899    73937      +38     
  Branches      6134     6134              
===========================================
+ Hits         48438    48482      +44     
+ Misses       21948    21942       -6     
  Partials      3513     3513              
Flag Coverage Δ
server 68.63% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@ankush ankush added the postgres postgres db related issue/PRs label Jan 9, 2023
@ankush ankush requested review from ankush and removed request for shariquerik January 9, 2023 05:13
@ankush
Copy link
Member

ankush commented Jan 9, 2023

Interval subtraction works on mariadb too afaik

https://mariadb.com/kb/en/date-and-time-units/

lemme attempt further simplifying this.

@ankush ankush marked this pull request as draft January 9, 2023 05:41
@ankush ankush force-pushed the fix-userprofile-with-postgres branch from 030a040 to 075a2d7 Compare January 9, 2023 06:36
@ankush ankush marked this pull request as ready for review January 9, 2023 06:36
@ankush ankush removed the add-test-cases Add test case to validate fix or enhancement label Jan 9, 2023
@ankush
Copy link
Member

ankush commented Jan 9, 2023

I've rewritten this entirely in QB and added UnixTimestamp function which is used by most heatmap queries... lets get rid of all remaining raw SQL queries sometime soon. Most are prone to DB incompatibility like this.

cc: @phot0n @surajshetty3416

@ankush ankush merged commit 82d18f0 into frappe:develop Jan 9, 2023
@ankush ankush added the backport version-14-hotfix backport to version 14 label Jan 9, 2023
ankush added a commit that referenced this pull request Jan 9, 2023
…ckport #19519) (#19522)

* fix(user profile): fix user profile qeuries to support postgres

(cherry picked from commit 9d14c68)

* fix(user_profile): remove unnecessary f string

(cherry picked from commit 7881642)

* feat: UnixTimestamp QB function

(cherry picked from commit 075a2d7)

Co-authored-by: AHasanin <ahmedhasanin755@gmail.com>
Co-authored-by: Ankush Menat <ankush@frappe.io>
@AHasanin
Copy link
Contributor Author

AHasanin commented Jan 9, 2023

It looks better now , Thanks for your ankush.

frappe-pr-bot pushed a commit that referenced this pull request Jan 10, 2023
# [14.22.0](v14.21.1...v14.22.0) (2023-01-10)

### Bug Fixes

* after login if redirect is used getting blank screen ([535e3ff](535e3ff))
* Child Table in Dialog: sortable [#19468](#19468) ([a660944](a660944))
* do not allow to change module field if developer_mode is not set ([faa819c](faa819c))
* do not show delete button for standard workspace if developer_mode is not on ([af403fa](af403fa))
* Dont create __init__.py in prod ([23e9ede](23e9ede))
* Dont throw back to `/` if desk page not found ([#19494](#19494)) ([#19501](#19501)) ([5089892](5089892))
* **DX:** Print alter query if it fails during migration ([#19552](#19552)) ([#19556](#19556)) ([38d569e](38d569e))
* For Update for child table ([#19436](#19436)) ([c280dfc](c280dfc))
* ignore unhandled emails while deleting email account ([#19534](#19534)) ([#19535](#19535)) ([dba4394](dba4394))
* Log settings - Handle validation failures ([#19549](#19549)) ([#19550](#19550)) ([a7880f6](a7880f6))
* remove "All" permission for Workpace ([9e958f7](9e958f7))
* render drop-icon if hide/unhide parent workspace ([f13b5ff](f13b5ff))
* RSS feed escaping ([#19546](#19546)) ([#19547](#19547)) ([1dc0b3c](1dc0b3c))
* show settings button only to workspace manager ([124d367](124d367))
* Update File Types Preview (backport [#18887](#18887)) ([#19456](#19456)) ([8615aba](8615aba))
* use `doc.has_permission` instead of `frappe.has_permission` ([#19543](#19543)) ([198dbd5](198dbd5))
* use file name for backups to Google Drive ([#19504](#19504)) ([#19506](#19506)) ([67eb173](67eb173))
* **user profile:** fix user profile queries to support postgresql (backport [#19519](#19519)) ([#19522](#19522)) ([f7baa51](f7baa51))

### Features

* hide/unhide workspace from sidebar ([3ccd773](3ccd773))
* **UI:** Sticky Tabs Header ([#18906](#18906)) ([0e6fa32](0e6fa32))

### Reverts

* checking if user has permission to meta ([#19485](#19485)) ([#19487](#19487)) ([34d9990](34d9990))
stephenBDT pushed a commit to alias/frappe that referenced this pull request Jan 10, 2023
…ckport frappe#19519) (frappe#19522)

* fix(user profile): fix user profile qeuries to support postgres

(cherry picked from commit 9d14c68)

* fix(user_profile): remove unnecessary f string

(cherry picked from commit 7881642)

* feat: UnixTimestamp QB function

(cherry picked from commit 075a2d7)

Co-authored-by: AHasanin <ahmedhasanin755@gmail.com>
Co-authored-by: Ankush Menat <ankush@frappe.io>
stephenBDT pushed a commit to alias/frappe that referenced this pull request Jan 10, 2023
# [14.22.0](frappe/frappe@v14.21.1...v14.22.0) (2023-01-10)

### Bug Fixes

* after login if redirect is used getting blank screen ([535e3ff](frappe@535e3ff))
* Child Table in Dialog: sortable [frappe#19468](frappe#19468) ([a660944](frappe@a660944))
* do not allow to change module field if developer_mode is not set ([faa819c](frappe@faa819c))
* do not show delete button for standard workspace if developer_mode is not on ([af403fa](frappe@af403fa))
* Dont create __init__.py in prod ([23e9ede](frappe@23e9ede))
* Dont throw back to `/` if desk page not found ([frappe#19494](frappe#19494)) ([frappe#19501](frappe#19501)) ([5089892](frappe@5089892))
* **DX:** Print alter query if it fails during migration ([frappe#19552](frappe#19552)) ([frappe#19556](frappe#19556)) ([38d569e](frappe@38d569e))
* For Update for child table ([frappe#19436](frappe#19436)) ([c280dfc](frappe@c280dfc))
* ignore unhandled emails while deleting email account ([frappe#19534](frappe#19534)) ([frappe#19535](frappe#19535)) ([dba4394](frappe@dba4394))
* Log settings - Handle validation failures ([frappe#19549](frappe#19549)) ([frappe#19550](frappe#19550)) ([a7880f6](frappe@a7880f6))
* remove "All" permission for Workpace ([9e958f7](frappe@9e958f7))
* render drop-icon if hide/unhide parent workspace ([f13b5ff](frappe@f13b5ff))
* RSS feed escaping ([frappe#19546](frappe#19546)) ([frappe#19547](frappe#19547)) ([1dc0b3c](frappe@1dc0b3c))
* show settings button only to workspace manager ([124d367](frappe@124d367))
* Update File Types Preview (backport [frappe#18887](frappe#18887)) ([frappe#19456](frappe#19456)) ([8615aba](frappe@8615aba))
* use `doc.has_permission` instead of `frappe.has_permission` ([frappe#19543](frappe#19543)) ([198dbd5](frappe@198dbd5))
* use file name for backups to Google Drive ([frappe#19504](frappe#19504)) ([frappe#19506](frappe#19506)) ([67eb173](frappe@67eb173))
* **user profile:** fix user profile queries to support postgresql (backport [frappe#19519](frappe#19519)) ([frappe#19522](frappe#19522)) ([f7baa51](frappe@f7baa51))

### Features

* hide/unhide workspace from sidebar ([3ccd773](frappe@3ccd773))
* **UI:** Sticky Tabs Header ([frappe#18906](frappe#18906)) ([0e6fa32](frappe@0e6fa32))

### Reverts

* checking if user has permission to meta ([frappe#19485](frappe#19485)) ([frappe#19487](frappe#19487)) ([34d9990](frappe@34d9990))
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14 postgres postgres db related issue/PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants