⚙ D42 Raw revision message download and display

jbertran updated this revision to Diff 137.Jun 8 2016, 7:20 PM

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)

Herald added a reviewer: Reviewers. · View Herald TranscriptJun 8 2016, 7:20 PM

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
183–184	Please use `message_url` instead of `msg_url` for consistency.

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

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

ardumont added a subscriber: ardumont.Jun 8 2016, 11:52 PM

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–219	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	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.

Move raw revision message url logic from converters to utils

ardumont added inline comments.Jun 9 2016, 3:59 PM

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

ardumont accepted this revision.Jun 10 2016, 9:36 AM

ardumont added a reviewer: ardumont.

olasd resigned from this revision.Jun 10 2016, 12:46 PM

olasd removed a reviewer: olasd.

This revision is now accepted and ready to land.Jun 10 2016, 12:46 PM

Closed by commit rDWAPPS4a59ce752ade: converters: setup revision message decoding here. Update tests to match (authored by jbertran). · Explain WhyJun 10 2016, 12:47 PM

This revision was automatically updated to reflect the committed changes.

jbertran marked 5 inline comments as done.

Raw revision message download and display
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 137

swh/web/ui/converters.py

swh/web/ui/service.py

swh/web/ui/templates/revision-log.html

swh/web/ui/templates/revision.html

swh/web/ui/tests/test_converters.py

swh/web/ui/tests/test_service.py

swh/web/ui/tests/views/test_api.py

swh/web/ui/tests/views/test_browse.py

swh/web/ui/views/api.py

swh/web/ui/views/browse.py

Raw revision message download and displayClosedPublicActions