Closed Bug 1025779 Opened 8 years ago Closed 8 years ago

Generation (at POST /call-url) and retrieval (GET /calls/{token}) of the call-url should require/provide an optional "issuer" parameter

Categories

(Hello (Loop) :: Server, defect, P3)

x86_64
Linux
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: alexis+bugs, Assigned: frsela)

References

Details

(Whiteboard: [qa?])

Attachments

(2 files)

issuer: Friendly name, if any, configured by person who generated the URL. If no name is configured, this field will be omitted.
Summary: Generation of the call-url (at POST /call-url) should require an optional "issuer" parameter → Generation (at POST /call-url) and retrieval (GET /calls/{token}) of the call-url should require/provide an optional "issuer" parameter
Priority: -- → P3
Whiteboard: [qa?]
Assignee: nobody → frsela
Attached file Proposed patch
Attachment #8445140 - Flags: feedback?(alexis+bugs)
Comment on attachment 8445140 [details] [review]
Proposed patch

That looks good to me, don't forget to update the tests and the documentation as well!
Attachment #8445140 - Flags: feedback?(alexis+bugs) → feedback+
Attached file Documentation update
Attachment #8445207 - Flags: review?(alexis+bugs)
Comment on attachment 8445140 [details] [review]
Proposed patch

Tests added and issuer renamed as suggested
Attachment #8445140 - Flags: review?(alexis+bugs)
Comment on attachment 8445207 [details] [review]
Documentation update

There is a misunderstanding about what we're trying to do here. I've commented on the documentation patch, I hope that will be clearer to you (it is for me, at least, thanks).
Attachment #8445207 - Flags: review?(alexis+bugs) → review-
Comment on attachment 8445140 [details] [review]
Proposed patch

Obviously, implementation is not up to date anymore with what we want, because the docs isn't.

Let's iterate on the doc and then implement if that's okay for you?
Attachment #8445140 - Flags: review?(alexis+bugs) → review-
Comment on attachment 8445140 [details] [review]
Proposed patch

Github comments addressed
Attachment #8445140 - Flags: review- → review?(alexis+bugs)
Comment on attachment 8445207 [details] [review]
Documentation update

Documentation updated based on your comments
Attachment #8445207 - Flags: review- → review?(alexis+bugs)
Comment on attachment 8445207 [details] [review]
Documentation update

There is still some confusion here. I commented on the github PR.
Attachment #8445207 - Flags: review?(alexis+bugs) → review-
Comment on attachment 8445140 [details] [review]
Proposed patch

clearing the review on the code PR since the docs aren't right yet.
Attachment #8445140 - Flags: review?(alexis+bugs)
Comment on attachment 8445207 [details] [review]
Documentation update

Fixed
Attachment #8445207 - Flags: review- → review?(alexis+bugs)
Comment on attachment 8445207 [details] [review]
Documentation update

Cool, that looks better to me, minus the nitpick I did in the PR.
Attachment #8445207 - Flags: review?(alexis+bugs) → review+
(In reply to Alexis Metaireau (:alexis) from comment #12)
> Comment on attachment 8445207 [details] [review]
> Documentation update
> 
> Cool, that looks better to me, minus the nitpick I did in the PR.

\o/
Thanks !
Nit addressed. Now I'll upload the patch.
Regards
Attachment #8445140 - Flags: review?(alexis+bugs)
Comment on attachment 8445140 [details] [review]
Proposed patch

Tests aren't good enough yet, but we're almost there :)
Attachment #8445140 - Flags: review?(alexis+bugs) → review-
https://github.com/mozilla-services/loop-server/commit/078f422488ac52e255d5519a9f5764d46cad7c27
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Verified in code and in unit tests.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.