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.

Sign up for GitHub

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

Jump to bottom

🎨 Removes stringified data field in socketio message #5335

Merged
merged 18 commits into from
Feb 22, 2024
Merged

🎨 Removes stringified data field in socketio message #5335

merged 18 commits into from
Feb 22, 2024

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Feb 14, 2024 •
edited
Loading

What do these changes do?

Following from #5078 (comment) this PR removes removes stringified data field in the socketio message.

Basically we have changed (1) by (2).
image
The rest is all adaptation to this change and some refactoring to reduce code duplication. Specifically:

  • backend: socketio's data field is not stringified but json-encoded
    • ♻️ small refactoring to unify code
  • frontend: Refactor to expect an object and not string @ignapas

Related issue/s

How to test

Dev Checklist

  • No ENV changes or I properly updated ENV

DevOps Checklist

@pcrespov pcrespov self-assigned this Feb 14, 2024
@pcrespov pcrespov requested a review from ignapas February 14, 2024 16:17
@pcrespov pcrespov added this to the Schoggilebe milestone Feb 14, 2024
@pcrespov pcrespov added a:frontend issue affecting the front-end (area group) a:webserver issue related to the webserver service labels Feb 14, 2024
@codecov Codecov
Copy link

codecov bot commented Feb 14, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dac655c) 87.4% compared to head (1e62736) 87.4%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5335   +/-   ##
======================================
  Coverage    87.4%   87.4%           
======================================
  Files        1315    1315           
  Lines       54075   54078    +3     
  Branches     1174    1174           
======================================
+ Hits        47304   47313    +9     
+ Misses       6521    6515    -6     
  Partials      250     250
Flag Coverage Δ
integrationtests 65.1% <100.0%> (+<0.1%) ⬆️
unittests 85.3% <100.0%> (+<0.1%) ⬆️

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

Files Coverage Δ
...rc/simcore_service_webserver/projects/_observer.py 100.0% <100.0%> (ø)
...rc/simcore_service_webserver/socketio/_handlers.py 86.2% <100.0%> (+0.3%) ⬆️
...src/simcore_service_webserver/socketio/messages.py 93.5% <100.0%> (ø)

... and 4 files with indirect coverage changes

ignapas
ignapas reviewed Feb 15, 2024
View reviewed changes
Copy link
Contributor

@ignapas ignapas left a comment

Choose a reason for hiding this comment

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

✅

@pcrespov pcrespov changed the title WIP: 🎨 Is5078/fix json payment ack soi message 🎨 Removes stringified data field in socketio message Feb 15, 2024
@pcrespov pcrespov marked this pull request as ready for review February 15, 2024 17:44
@pcrespov pcrespov enabled auto-merge (squash) February 15, 2024 17:47
@pcrespov pcrespov force-pushed the is5078/fix-json-payment-ack-soi-message branch from 4f66ce7 to 1923c8e Compare February 15, 2024 19:26
@pcrespov pcrespov disabled auto-merge February 15, 2024 19:27
@pcrespov pcrespov marked this pull request as draft February 15, 2024 19:27
sanderegg
sanderegg approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thanks a lot, that is a very nice change!
I do not see the counterpart in the frontend though? should you not have a bunch of files that stop parsing the strings back?

sanderegg
sanderegg reviewed Feb 15, 2024
View reviewed changes
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thanks a lot, that is a very nice change!
I do not see the counterpart in the frontend though? should you not have a bunch of files that stop parsing the strings back?

GitHK
GitHK reviewed Feb 19, 2024
View reviewed changes
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

@pcrespov I guess this one goes away now, right? can we close it?

@pcrespov
Copy link
Member Author

@pcrespov I guess this one goes away now, right? can we close it?

@GitHK no, this does not go away. @ignapas will push changes in the front-end and then I will adapt the backend

@ignapas ignapas force-pushed the is5078/fix-json-payment-ack-soi-message branch from 1923c8e to 65a3c53 Compare February 21, 2024 09:15
@pcrespov pcrespov force-pushed the is5078/fix-json-payment-ack-soi-message branch from 65a3c53 to 1c809e6 Compare February 21, 2024 09:24
@pcrespov pcrespov marked this pull request as ready for review February 21, 2024 09:25
@pcrespov pcrespov requested a review from GitHK February 21, 2024 09:25
@ignapas
Merge branch 'is5078/fix-json-payment-ack-soi-message' of github.com:…
fc8b396 
…pcrespov/osparc-simcore into is5078/fix-json-payment-ack-soi-message

 Conflicts:
	services/web/server/src/simcore_service_webserver/socketio/messages.py
@ignapas
removed remaining JSON.parse
462c997
ignapas
ignapas approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@ignapas ignapas left a comment

Choose a reason for hiding this comment

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

👍🏼

jsaq007
jsaq007 reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@jsaq007 jsaq007 left a comment

Choose a reason for hiding this comment

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

👍🏼

@pcrespov pcrespov requested a review from jsaq007 February 21, 2024 11:27
@pcrespov pcrespov enabled auto-merge (squash) February 22, 2024 10:31
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@pcrespov pcrespov merged commit 226c183 into ITISFoundation:master Feb 22, 2024
55 checks passed
@pcrespov pcrespov deleted the is5078/fix-json-payment-ack-soi-message branch February 22, 2024 13:00
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Feb 23, 2024
Closed
21 tasks
jsaq007 pushed a commit to jsaq007/osparc-simcore that referenced this pull request Feb 27, 2024
@pcrespov @ignapas @jsaq007
🎨 Removes stringified data field in socketio message (ITISFoundatio…
a871647 
…n#5335)

Co-authored-by: Ignacio Pascual <[email protected]>
jsaq007 pushed a commit to jsaq007/osparc-simcore that referenced this pull request Mar 4, 2024
@pcrespov @ignapas @jsaq007
🎨 Removes stringified data field in socketio message (ITISFoundatio…
c8f0d43 
…n#5335)

Co-authored-by: Ignacio Pascual <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsaq007 jsaq007 jsaq007 left review comments

@ignapas ignapas ignapas approved these changes

@GitHK GitHK GitHK left review comments

@sanderegg sanderegg sanderegg approved these changes

@matusdrobuliak66 matusdrobuliak66 matusdrobuliak66 approved these changes

@bisgaard-itis bisgaard-itis Awaiting requested review from bisgaard-itis

Assignees

@ignapas ignapas

@pcrespov pcrespov

Labels
a:frontend issue affecting the front-end (area group) a:webserver issue related to the webserver service
Projects
None yet
Milestone
Schoggilebe
Development

Successfully merging this pull request may close these issues.
6 participants
@pcrespov @jsaq007 @ignapas @GitHK @sanderegg @matusdrobuliak66