Skip to content

Conversation

akshaymankar
Copy link
Member

Federation is not implemented yet, so remote handles will always return 404.

@akshaymankar akshaymankar requested review from jschaul and mheinzel and removed request for jschaul December 9, 2020 10:45
Federation is not implemented yet, so remote handles will always return 404.
@akshaymankar akshaymankar force-pushed the akshaymankar/fed-end-get-handle-info branch from 69ec6cc to 6765c65 Compare December 15, 2020 09:18
@akshaymankar akshaymankar requested a review from jschaul December 15, 2020 16:20
@akshaymankar
Copy link
Member Author

Bonus change:

  • Return qualified id while returning user by ID.

Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Minor nitpicks, looks good otherwise. Perhaps we should remain consistent with the naming of the fields though.

# "email" .= profileEmail u
# []
object
[ "id" .= _qLocalPart (profileQualifiedId u),
Copy link
Member

Choose a reason for hiding this comment

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

FUTUREWORK (maybe in another PR): rename _qLocalPart to something nicer, e.g. localId

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thoughts, this is also used for Qualified Handle , so calling it localId doesn't make sense. I can't think for a better name than "localPart"

Copy link
Member Author

Choose a reason for hiding this comment

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

Overthinking maybe, but localPart is also wrong as it could be a remote thing. So, ideally should be called unqualified or unqualifiedPart.

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, let's continue this discussion in #1290

Copy link
Contributor

Choose a reason for hiding this comment

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

I followed the naming of email adresses there: https://en.wikipedia.org/wiki/Email_address#Local-part

instance ToJSON UserHandleInfo where
toJSON (UserHandleInfo u) =
object
["user" .= u]
[ "user" .= _qLocalPart u, -- For backwards comptaibility
"qualified_user" .= u
Copy link
Member

Choose a reason for hiding this comment

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

so here we use qualified_user not qualified_id? Isn't that inconsistent then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am just prepending qualified_ to whatever we had earlier. Doing this sounds more consistent than changing everything to qualified_id. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

hm, I don't know. The old fields (sometimes called 'user' and sometimes called 'id') seemed inconsistent; this could be a time to change that and not keep that inconsistency. But maybe it doesn't matter if clients use the swagger definitions to create their API.

@akshaymankar akshaymankar merged commit fe64794 into develop Dec 16, 2020
@akshaymankar akshaymankar deleted the akshaymankar/fed-end-get-handle-info branch December 16, 2020 10:46
jschaul added a commit that referenced this pull request Dec 16, 2020
While omitting a json field, or setting the field to null parses the
same for Haskell, that is not the case for other programming languages,
which can easily trip up over a change in behaviour.

The problem was accidentally introduced in #1281, resulting in null
values instead of omitting fields for UserProfile json, e.g.

{
   "deleted" : null,
   "picture" : [],
   "email" : null,
   "accent_id" : 1,
   "team" : null,
   "handle" : "test1",
   "expires_at" : null,
   "locale" : "en-US",
   "qualified_id" : {
      "id" : "d93f2555-e4b7-4051-8d84-2bcf59ea6066",
      "domain" : "staging.zinfra.io"
   },
   "service" : null,
   "name" : "test",
   "id" : "d93f2555-e4b7-4051-8d84-2bcf59ea6066",
   "assets" : [
      {
         "type" : "image",
         "size" : "preview",
         "key" : "3-1-43978d07-040c-4f49-be7e-06e2560701fd"
      },
      {
         "type" : "image",
         "key" : "3-1-f3039a69-a0b8-45a5-bc6b-3da087b680c7",
         "size" : "complete"
      }
   ]
}

This PR reverts the offending change, and adds one manual unit test for
this particular data type. We may wish to add more tests for other data
types as well in future PRs to catch some backwards compatibility changes.
jschaul added a commit that referenced this pull request Dec 16, 2020
)

* unit test and fix for null values in rendered JSON in UserProfile

While omitting a json field, or setting the field to null parses the
same for Haskell, that is not the case for other programming languages,
which can easily trip up over a change in behaviour.

The problem was accidentally introduced in #1281, resulting in null
values instead of omitting fields for UserProfile json, e.g.

{
   "deleted" : null,
   "picture" : [],
   "email" : null,
   "accent_id" : 1,
   "team" : null,
   "handle" : "test1",
   "expires_at" : null,
   "locale" : "en-US",
   "qualified_id" : {
      "id" : "d93f2555-e4b7-4051-8d84-2bcf59ea6066",
      "domain" : "staging.zinfra.io"
   },
   "service" : null,
   "name" : "test",
   "id" : "d93f2555-e4b7-4051-8d84-2bcf59ea6066",
   "assets" : [
      {
         "type" : "image",
         "size" : "preview",
         "key" : "3-1-43978d07-040c-4f49-be7e-06e2560701fd"
      },
      {
         "type" : "image",
         "key" : "3-1-f3039a69-a0b8-45a5-bc6b-3da087b680c7",
         "size" : "complete"
      }
   ]
}

This caused a problem as described in https://wearezeta.atlassian.net/browse/SQCORE-351

This PR reverts the offending change, and adds one manual unit test for
this particular data type. We may wish to add more tests for other data
types as well in future PRs to catch some backwards compatibility changes.
@fisx fisx mentioned this pull request Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants