Page MenuHomeSoftware Heritage

Raw revision message download and display
ClosedPublic

Authored by jbertran on Jun 8 2016, 7:20 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Commits
R65:3fc92ddfb078: utils: add logic to calculate raw revision message url to enrich_revision…
R65:bd480fa6df2a: revision, revision-log: update jinja2 to reflect changes to raw message url…
R65:176280f7a10c: utils, test_utils: refactor to eliminate useless tests and avoid redundant…
R65:e9360a46f830: test_utils: rename helper function to show it's a helper
R65:70d27ee04733: converters, service: * remove url logic to fit with converters being api…
R65:fe8b01656b71: templates: update revision and revision-log to reflect raw message display…
R65:8635f93bf14c: service: * remove decoding logic from revision message handling, now in…
R65:6c75b9f6c83b: api: update api_revision_raw_message to automatically download the raw revision…
R65:4a59ce752ade: converters: setup revision message decoding here. Update tests to match
R65:9ec1cbb0cc96: browse: add raw rev message browsing route, update tests to match
R65:28276ee94c4b: Converters, Service, Api, Browse: change 'msg_url' to 'message_url'
rDWAPPS3fc92ddfb078: utils: add logic to calculate raw revision message url to enrich_revision…
rDWAPPSe9360a46f830: test_utils: rename helper function to show it's a helper
rDWAPPS70d27ee04733: converters, service: * remove url logic to fit with converters being api…
rDWAPPSfe8b01656b71: templates: update revision and revision-log to reflect raw message display…
rDWAPPS176280f7a10c: utils, test_utils: refactor to eliminate useless tests and avoid redundant…
rDWAPPSbd480fa6df2a: revision, revision-log: update jinja2 to reflect changes to raw message url…
rDWAPPS28276ee94c4b: Converters, Service, Api, Browse: change 'msg_url' to 'message_url'
rDWAPPS9ec1cbb0cc96: browse: add raw rev message browsing route, update tests to match
rDWAPPS6c75b9f6c83b: api: update api_revision_raw_message to automatically download the raw…
rDWAPPS8635f93bf14c: service: * remove decoding logic from revision message handling, now in…
rDWAPPS4a59ce752ade: converters: setup revision message decoding here. Update tests to match
Summary

Modify the revision raw message display approach

Test Plan

Tests included

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jbertran retitled this revision from to Raw revision message download and display.
jbertran updated this object.
jbertran edited the test plan for this revision. (Show Details)
olasd requested changes to this revision.Jun 8 2016, 7:28 PM
olasd added a reviewer: olasd.
olasd added a subscriber: olasd.

I think the implementation of this is reasonable. I would just prefer that you use message_url instead of msg_url to keep the object keys consistent with the actual field.

swh/web/ui/converters.py
182–183

Please use message_url instead of msg_url for consistency.

This revision now requires changes to proceed.Jun 8 2016, 7:28 PM
jbertran edited edge metadata.

Converters, Service, Api, Browse: change 'msg_url' to 'message_url'

ardumont added inline comments.
swh/web/ui/converters.py
183

The converter and the service layer modules are api/view agnostic.
To be separated as the existing code is doing, the decode call should be done in the service layer.
If it breaks, then the message is None.

It should be the concern of the api layer to test for the message.
If none exists, then the message_url is populated with the message url of the raw api.

swh/web/ui/service.py
218 ↗(On Diff #138)

That would be here that I'd add the unicode conversion with the trap.
I would still not fill in the message url (i let the api layer do that).

swh/web/ui/views/api.py
492 ↗(On Diff #138)

That'd be here that i'd add the message_url if message is None.
In utils.enrich_revision's function definition (to be transverse to all).
utils is not api agnostic.

jbertran edited edge metadata.

Move raw revision message url logic from converters to utils

swh/web/ui/tests/test_utils.py
579 ↗(On Diff #139)

Can you please remove the print statement?

636 ↗(On Diff #139)

If it's the same function as defined in prior test, define it globally once and reuse it.
Make it so that its name starts with __.

swh/web/ui/utils.py
247 ↗(On Diff #139)

Instead of the 3 tests before, can't we simplify to one test on 'message_decoding_failed' existence?

I mean, isn't this sufficient?

if 'message_decoding_failed' in revision:
    ...

?

utils, test_utils: refactor to eliminate useless tests and avoid redundant functions

test_utils: rename helper function to show it's a helper

olasd removed a reviewer: olasd.
This revision is now accepted and ready to land.Jun 10 2016, 12:46 PM
This revision was automatically updated to reflect the committed changes.
jbertran marked 5 inline comments as done.