Re: [Snowdrift-dev] [PATCH] Add new route /u/
On Mon, Mar 07, 2016 at 03:30:00AM +0200, fr33domlover wrote: > On Tue, 1 Mar 2016 16:27:41 -0800 Bryan Richter wrote: > > > ** Tech action items > > > > - UserHandle should be redefined to Text > > - The existing route should be renamed getUserByIdR and should use > > UserId instead of UserHandle > > - The new route should get the original name UserR > > One thing I noticed when I started working on the code is that > UserHandle is confusing. I checked what that type is used for. And > it's used exactly for one thing: The type of the path piece in the > routes file. ... So far. :) It was only just created. I did so to provide a layer of abstraction. I'd rather have it than a bare Int or Text. signature.asc Description: Digital signature ___ Dev mailing list Dev@lists.snowdrift.coop https://lists.snowdrift.coop/mailman/listinfo/dev
Re: [Snowdrift-dev] [PATCH] Add new route /u/
On 03/06/2016 05:30 PM, fr33domlover wrote: > On Tue, 1 Mar 2016 16:27:41 -0800 > Bryan Richter wrote: > >> ** Tech action items >> >> - UserHandle should be redefined to Text >> - The existing route should be renamed getUserByIdR and should use >> UserId instead of UserHandle >> - The new route should get the original name UserR > > One thing I noticed when I started working on the code is that UserHandle is > confusing. I checked what that type is used for. And it's used exactly for one > thing: The type of the path piece in the routes file. Since the auth ID is > still the user ID (as it should be), I think UserHandle is just confusing and > we can safely remove that type. > > Instead we can use Text directly. > > Since changing from UserId to Text in the /u/ route also requires > changes > in handlers and the DB queries they run, moving between types back and forth > isn't trivial anyway, I don't see a particular reason to use a type alias if > we > do use plain Text for other things in the code. If you want to define a > newtype > that wraps Text, that's a different story and a separate discussion. > > For now I'm leaving UserHandle as-is in the code, waiting for your thoughts. > It's not a critical thing, just wanted to share what I feel about it (but as a > beginner Yesod user I may be wrong). > > --fr33 > I can't speak to the code and type details but the term "handle" is terribly confusing, nobody gets what it is easily, and I support all efforts to remove it from our code (but not saying they are necessarily priority). We should use less confusing terms. signature.asc Description: OpenPGP digital signature ___ Dev mailing list Dev@lists.snowdrift.coop https://lists.snowdrift.coop/mailman/listinfo/dev
Re: [Snowdrift-dev] [PATCH] Add new route /u/
On Tue, 1 Mar 2016 16:27:41 -0800 Bryan Richter wrote: > ** Tech action items > > - UserHandle should be redefined to Text > - The existing route should be renamed getUserByIdR and should use > UserId instead of UserHandle > - The new route should get the original name UserR One thing I noticed when I started working on the code is that UserHandle is confusing. I checked what that type is used for. And it's used exactly for one thing: The type of the path piece in the routes file. Since the auth ID is still the user ID (as it should be), I think UserHandle is just confusing and we can safely remove that type. Instead we can use Text directly. Since changing from UserId to Text in the /u/ route also requires changes in handlers and the DB queries they run, moving between types back and forth isn't trivial anyway, I don't see a particular reason to use a type alias if we do use plain Text for other things in the code. If you want to define a newtype that wraps Text, that's a different story and a separate discussion. For now I'm leaving UserHandle as-is in the code, waiting for your thoughts. It's not a critical thing, just wanted to share what I feel about it (but as a beginner Yesod user I may be wrong). --fr33 ___ Dev mailing list Dev@lists.snowdrift.coop https://lists.snowdrift.coop/mailman/listinfo/dev
Re: [Snowdrift-dev] [PATCH] Add new route /u/
One additional note: The uniqueness for the table is based on that id #, not on the user handle. If we're going to change the URL to be based on the user handle (which isn't a bad idea), we need to change the uniqueness of the table and/or ensure we don't have any duplicate user handles now or going forward. On Tue, Mar 1, 2016 at 6:27 PM, Bryan Richter wrote: > On Wed, Mar 02, 2016 at 01:46:41AM +0200, fr33domlover wrote: > > From: fr33domlover > > > > A URL path like `/u/wolftune` is much more convenient than `/u/3`, > > and most websites seem to use the former indeed. This is a (mostly, > > see below) non-breaking change which: > > > > 1. Adds a new handler for `/u/` which returns the usual > >user page > > 2. Makes `/u/` be a simple redirect to the new route > > > > Hopefully this is the beginning of moving to `/u/` being > > the primary path and `/u/` being secondary or even not > > existing at all. > > This is superb. I would like confirmation from the UI/UX team, but I'm > going to move forward assuming they agree it's a good idea. If nothing > else, it is a good *technology* decision that shields implementation > details from the UI. > > > NOTE: If the current production database has a user whose > > `userIdent` contains only digits, this change may create a problem. > > This is unlikely, but worth checking just in case. > > This does bring up additional concerns we must address. > > I agree we should move towards /u/ being the default, and > thus it should be reflected in the names. > > ** Tech action items > > - UserHandle should be redefined to Text > - The existing route should be renamed getUserByIdR and should use > UserId instead of UserHandle > - The new route should get the original name UserR > > Additionally we need to safeguard ourselves against ambiguity. I just > confirmed that no existing ident is all digits, so we have no existing > problem. For the future, however, perhaps we should disallow all-digit > usernames? > > ** Questions for other teams > > - Move forward with /u/? > - Disallow all-digit usernames to avoid future ambiguity? > > fr33, if you don't mind, can you be in charge of representing the > development team in discussing this plan with the other teams? > > Thanks again either way. This is definitely a good idea. > > > --- > > config/routes| 3 ++- > > src/Handler/NewDesign.hs | 8 +++- > > 2 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/config/routes b/config/routes > > index 999f36b..575cff3 100644 > > --- a/config/routes > > +++ b/config/routes > > @@ -74,7 +74,8 @@ > > -- ## Browsing a particular user (may need fleshing out?) > > -- > > > > -/u/#UserHandle UserR GET > > +/u/!#UserHandle UserRGET > > +/u/#TextUserByIdentR GET > > > > -- Yesod jibber jabber > > /static StaticR Static appStatic > > diff --git a/src/Handler/NewDesign.hs b/src/Handler/NewDesign.hs > > index 05829ab..dcfd060 100644 > > --- a/src/Handler/NewDesign.hs > > +++ b/src/Handler/NewDesign.hs > > @@ -212,9 +212,15 @@ postCreateAccountR = do > > -- | Public profile for a user. > > getUserR :: UserId -> Handler Html > > getUserR user_id = do > > +user <- runYDB $ get404 user_id > > +redirect $ UserByIdentR $ userIdent user > > + > > +-- | Public profile for a user. > > +getUserByIdentR :: Text -> Handler Html > > +getUserByIdentR user_ident = do > > mviewer_id <- maybeAuthId > > > > -user <- runYDB $ get404 user_id > > +Entity user_id user <- runYDB $ getBy404 $ UniqueUser user_ident > > > > projects_and_roles <- runDB (fetchUserProjectsAndRolesDB user_id) > > when ( Just user_id == mviewer_id > > -- > > 1.9.1 > > > > ___ > > Dev mailing list > > Dev@lists.snowdrift.coop > > https://lists.snowdrift.coop/mailman/listinfo/dev > > ___ > Dev mailing list > Dev@lists.snowdrift.coop > https://lists.snowdrift.coop/mailman/listinfo/dev > > ___ Dev mailing list Dev@lists.snowdrift.coop https://lists.snowdrift.coop/mailman/listinfo/dev
Re: [Snowdrift-dev] [PATCH] Add new route /u/
On Wed, Mar 02, 2016 at 01:46:41AM +0200, fr33domlover wrote: > From: fr33domlover > > A URL path like `/u/wolftune` is much more convenient than `/u/3`, > and most websites seem to use the former indeed. This is a (mostly, > see below) non-breaking change which: > > 1. Adds a new handler for `/u/` which returns the usual >user page > 2. Makes `/u/` be a simple redirect to the new route > > Hopefully this is the beginning of moving to `/u/` being > the primary path and `/u/` being secondary or even not > existing at all. This is superb. I would like confirmation from the UI/UX team, but I'm going to move forward assuming they agree it's a good idea. If nothing else, it is a good *technology* decision that shields implementation details from the UI. > NOTE: If the current production database has a user whose > `userIdent` contains only digits, this change may create a problem. > This is unlikely, but worth checking just in case. This does bring up additional concerns we must address. I agree we should move towards /u/ being the default, and thus it should be reflected in the names. ** Tech action items - UserHandle should be redefined to Text - The existing route should be renamed getUserByIdR and should use UserId instead of UserHandle - The new route should get the original name UserR Additionally we need to safeguard ourselves against ambiguity. I just confirmed that no existing ident is all digits, so we have no existing problem. For the future, however, perhaps we should disallow all-digit usernames? ** Questions for other teams - Move forward with /u/? - Disallow all-digit usernames to avoid future ambiguity? fr33, if you don't mind, can you be in charge of representing the development team in discussing this plan with the other teams? Thanks again either way. This is definitely a good idea. > --- > config/routes| 3 ++- > src/Handler/NewDesign.hs | 8 +++- > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/config/routes b/config/routes > index 999f36b..575cff3 100644 > --- a/config/routes > +++ b/config/routes > @@ -74,7 +74,8 @@ > -- ## Browsing a particular user (may need fleshing out?) > -- > > -/u/#UserHandle UserR GET > +/u/!#UserHandle UserRGET > +/u/#TextUserByIdentR GET > > -- Yesod jibber jabber > /static StaticR Static appStatic > diff --git a/src/Handler/NewDesign.hs b/src/Handler/NewDesign.hs > index 05829ab..dcfd060 100644 > --- a/src/Handler/NewDesign.hs > +++ b/src/Handler/NewDesign.hs > @@ -212,9 +212,15 @@ postCreateAccountR = do > -- | Public profile for a user. > getUserR :: UserId -> Handler Html > getUserR user_id = do > +user <- runYDB $ get404 user_id > +redirect $ UserByIdentR $ userIdent user > + > +-- | Public profile for a user. > +getUserByIdentR :: Text -> Handler Html > +getUserByIdentR user_ident = do > mviewer_id <- maybeAuthId > > -user <- runYDB $ get404 user_id > +Entity user_id user <- runYDB $ getBy404 $ UniqueUser user_ident > > projects_and_roles <- runDB (fetchUserProjectsAndRolesDB user_id) > when ( Just user_id == mviewer_id > -- > 1.9.1 > > ___ > Dev mailing list > Dev@lists.snowdrift.coop > https://lists.snowdrift.coop/mailman/listinfo/dev signature.asc Description: Digital signature ___ Dev mailing list Dev@lists.snowdrift.coop https://lists.snowdrift.coop/mailman/listinfo/dev
[Snowdrift-dev] [PATCH] Add new route /u/
From: fr33domlover A URL path like `/u/wolftune` is much more convenient than `/u/3`, and most websites seem to use the former indeed. This is a (mostly, see below) non-breaking change which: 1. Adds a new handler for `/u/` which returns the usual user page 2. Makes `/u/` be a simple redirect to the new route Hopefully this is the beginning of moving to `/u/` being the primary path and `/u/` being secondary or even not existing at all. NOTE: If the current production database has a user whose `userIdent` contains only digits, this change may create a problem. This is unlikely, but worth checking just in case. --- config/routes| 3 ++- src/Handler/NewDesign.hs | 8 +++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/config/routes b/config/routes index 999f36b..575cff3 100644 --- a/config/routes +++ b/config/routes @@ -74,7 +74,8 @@ -- ## Browsing a particular user (may need fleshing out?) -- -/u/#UserHandle UserR GET +/u/!#UserHandle UserRGET +/u/#TextUserByIdentR GET -- Yesod jibber jabber /static StaticR Static appStatic diff --git a/src/Handler/NewDesign.hs b/src/Handler/NewDesign.hs index 05829ab..dcfd060 100644 --- a/src/Handler/NewDesign.hs +++ b/src/Handler/NewDesign.hs @@ -212,9 +212,15 @@ postCreateAccountR = do -- | Public profile for a user. getUserR :: UserId -> Handler Html getUserR user_id = do +user <- runYDB $ get404 user_id +redirect $ UserByIdentR $ userIdent user + +-- | Public profile for a user. +getUserByIdentR :: Text -> Handler Html +getUserByIdentR user_ident = do mviewer_id <- maybeAuthId -user <- runYDB $ get404 user_id +Entity user_id user <- runYDB $ getBy404 $ UniqueUser user_ident projects_and_roles <- runDB (fetchUserProjectsAndRolesDB user_id) when ( Just user_id == mviewer_id -- 1.9.1 ___ Dev mailing list Dev@lists.snowdrift.coop https://lists.snowdrift.coop/mailman/listinfo/dev
