Thread: Prevent duplicate attributes
Hi (yes, me... again), Tiny patch attached to prevent adding attributes with a duplicate name to a type. It seems previous versions prevented this, but must have been allowed in a commit since the last release. Regards Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Sat, Jul 9, 2011 at 9:02 PM, Thom Brown <thom@linux.com> wrote: > Hi (yes, me... again), > > Tiny patch attached to prevent adding attributes with a duplicate name > to a type. It seems previous versions prevented this, but must have > been allowed in a commit since the last release. For consistency with the other checks on the dialogue, this should cause the "Add" button to be disabled, not make it silently fail. That should almost certainly be done in dlgType::CheckChange(). -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 12 July 2011 10:53, Dave Page <dpage@pgadmin.org> wrote: > On Sat, Jul 9, 2011 at 9:02 PM, Thom Brown <thom@linux.com> wrote: >> Hi (yes, me... again), >> >> Tiny patch attached to prevent adding attributes with a duplicate name >> to a type. It seems previous versions prevented this, but must have >> been allowed in a commit since the last release. > > For consistency with the other checks on the dialogue, this should > cause the "Add" button to be disabled, not make it silently fail. That > should almost certainly be done in dlgType::CheckChange(). Yes, you're right. -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, 2011-07-12 at 10:55 +0100, Thom Brown wrote: > On 12 July 2011 10:53, Dave Page <dpage@pgadmin.org> wrote: > > On Sat, Jul 9, 2011 at 9:02 PM, Thom Brown <thom@linux.com> wrote: > >> Hi (yes, me... again), > >> > >> Tiny patch attached to prevent adding attributes with a duplicate name > >> to a type. It seems previous versions prevented this, but must have > >> been allowed in a commit since the last release. > > > > For consistency with the other checks on the dialogue, this should > > cause the "Add" button to be disabled, not make it silently fail. That > > should almost certainly be done in dlgType::CheckChange(). > > Yes, you're right. > Does this patch need to be applied after the "overhaul of type attributes" one? -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On 12 July 2011 11:11, Guillaume Lelarge <guillaume@lelarge.info> wrote: > On Tue, 2011-07-12 at 10:55 +0100, Thom Brown wrote: >> On 12 July 2011 10:53, Dave Page <dpage@pgadmin.org> wrote: >> > On Sat, Jul 9, 2011 at 9:02 PM, Thom Brown <thom@linux.com> wrote: >> >> Hi (yes, me... again), >> >> >> >> Tiny patch attached to prevent adding attributes with a duplicate name >> >> to a type. It seems previous versions prevented this, but must have >> >> been allowed in a commit since the last release. >> > >> > For consistency with the other checks on the dialogue, this should >> > cause the "Add" button to be disabled, not make it silently fail. That >> > should almost certainly be done in dlgType::CheckChange(). >> >> Yes, you're right. >> > > Does this patch need to be applied after the "overhaul of type > attributes" one? No, as this patch shouldn't be applied at all since, as Dave pointed out, the duplicate check should appear in CheckChange(). This is an existing bug unrelated to the type attribute changes patch. I just noticed it while testing my changes. -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, 2011-07-12 at 11:17 +0100, Thom Brown wrote: > On 12 July 2011 11:11, Guillaume Lelarge <guillaume@lelarge.info> wrote: > > On Tue, 2011-07-12 at 10:55 +0100, Thom Brown wrote: > >> On 12 July 2011 10:53, Dave Page <dpage@pgadmin.org> wrote: > >> > On Sat, Jul 9, 2011 at 9:02 PM, Thom Brown <thom@linux.com> wrote: > >> >> Hi (yes, me... again), > >> >> > >> >> Tiny patch attached to prevent adding attributes with a duplicate name > >> >> to a type. It seems previous versions prevented this, but must have > >> >> been allowed in a commit since the last release. > >> > > >> > For consistency with the other checks on the dialogue, this should > >> > cause the "Add" button to be disabled, not make it silently fail. That > >> > should almost certainly be done in dlgType::CheckChange(). > >> > >> Yes, you're right. > >> > > > > Does this patch need to be applied after the "overhaul of type > > attributes" one? > > No, as this patch shouldn't be applied at all since, as Dave pointed > out, the duplicate check should appear in CheckChange(). This is an > existing bug unrelated to the type attribute changes patch. I just > noticed it while testing my changes. > So, you're working on fixing it, right? -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On 14 July 2011 10:24, Guillaume Lelarge <guillaume@lelarge.info> wrote: > On Tue, 2011-07-12 at 11:17 +0100, Thom Brown wrote: >> On 12 July 2011 11:11, Guillaume Lelarge <guillaume@lelarge.info> wrote: >> > On Tue, 2011-07-12 at 10:55 +0100, Thom Brown wrote: >> >> On 12 July 2011 10:53, Dave Page <dpage@pgadmin.org> wrote: >> >> > On Sat, Jul 9, 2011 at 9:02 PM, Thom Brown <thom@linux.com> wrote: >> >> >> Hi (yes, me... again), >> >> >> >> >> >> Tiny patch attached to prevent adding attributes with a duplicate name >> >> >> to a type. It seems previous versions prevented this, but must have >> >> >> been allowed in a commit since the last release. >> >> > >> >> > For consistency with the other checks on the dialogue, this should >> >> > cause the "Add" button to be disabled, not make it silently fail. That >> >> > should almost certainly be done in dlgType::CheckChange(). >> >> >> >> Yes, you're right. >> >> >> > >> > Does this patch need to be applied after the "overhaul of type >> > attributes" one? >> >> No, as this patch shouldn't be applied at all since, as Dave pointed >> out, the duplicate check should appear in CheckChange(). This is an >> existing bug unrelated to the type attribute changes patch. I just >> noticed it while testing my changes. >> > > So, you're working on fixing it, right? Possibly ;) : http://code.pgadmin.org/trac/ticket/329 I haven't had time to look at fixing various bugs over the last couple days but I'll get to them shortly. -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 14 July 2011 10:31, Thom Brown <thom@linux.com> wrote: > On 14 July 2011 10:24, Guillaume Lelarge <guillaume@lelarge.info> wrote: >> So, you're working on fixing it, right? > > Possibly ;) : http://code.pgadmin.org/trac/ticket/329 > > I haven't had time to look at fixing various bugs over the last couple > days but I'll get to them shortly. Apologies for the delay but I've now had a moment to look at it. The attached patch removes the duplicate restriction from OnMemberAdd to OnChangeMember, and also disabled the Add button by default. This is because it's enabled on loading the form which doesn't get disabled again until you start editing the attribute name field, meaning you can click Add without having provided any attribute details. -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Mon, 2011-07-18 at 21:06 +0100, Thom Brown wrote: > On 14 July 2011 10:31, Thom Brown <thom@linux.com> wrote: > > On 14 July 2011 10:24, Guillaume Lelarge <guillaume@lelarge.info> wrote: > >> So, you're working on fixing it, right? > > > > Possibly ;) : http://code.pgadmin.org/trac/ticket/329 > > > > I haven't had time to look at fixing various bugs over the last couple > > days but I'll get to them shortly. > > Apologies for the delay but I've now had a moment to look at it. No apologies needed :) > The > attached patch removes the duplicate restriction from OnMemberAdd to > OnChangeMember, and also disabled the Add button by default. This is > because it's enabled on loading the form which doesn't get disabled > again until you start editing the attribute name field, meaning you > can click Add without having provided any attribute details. > I'm wondering why you didn't take care of the Change button too. I mean, it's so easy now that you've the code working for the Add button that I kind of feel I'm missing something obvious which would explain why you didn't do it. Any particular reason other than "I forgot it"? :) -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On 18 July 2011 21:55, Guillaume Lelarge <guillaume@lelarge.info> wrote: > On Mon, 2011-07-18 at 21:06 +0100, Thom Brown wrote: >> On 14 July 2011 10:31, Thom Brown <thom@linux.com> wrote: >> > On 14 July 2011 10:24, Guillaume Lelarge <guillaume@lelarge.info> wrote: >> >> So, you're working on fixing it, right? >> > >> > Possibly ;) : http://code.pgadmin.org/trac/ticket/329 >> > >> > I haven't had time to look at fixing various bugs over the last couple >> > days but I'll get to them shortly. >> >> Apologies for the delay but I've now had a moment to look at it. > > No apologies needed :) > >> The >> attached patch removes the duplicate restriction from OnMemberAdd to >> OnChangeMember, and also disabled the Add button by default. This is >> because it's enabled on loading the form which doesn't get disabled >> again until you start editing the attribute name field, meaning you >> can click Add without having provided any attribute details. >> > > I'm wondering why you didn't take care of the Change button too. I mean, > it's so easy now that you've the code working for the Add button that I > kind of feel I'm missing something obvious which would explain why you > didn't do it. Any particular reason other than "I forgot it"? :) Wasn't even that. It didn't cross my mind. Reattached with that change. -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Mon, 2011-07-18 at 22:15 +0100, Thom Brown wrote: > On 18 July 2011 21:55, Guillaume Lelarge <guillaume@lelarge.info> wrote: > > On Mon, 2011-07-18 at 21:06 +0100, Thom Brown wrote: > >> On 14 July 2011 10:31, Thom Brown <thom@linux.com> wrote: > >> > On 14 July 2011 10:24, Guillaume Lelarge <guillaume@lelarge.info> wrote: > >> >> So, you're working on fixing it, right? > >> > > >> > Possibly ;) : http://code.pgadmin.org/trac/ticket/329 > >> > > >> > I haven't had time to look at fixing various bugs over the last couple > >> > days but I'll get to them shortly. > >> > >> Apologies for the delay but I've now had a moment to look at it. > > > > No apologies needed :) > > > >> The > >> attached patch removes the duplicate restriction from OnMemberAdd to > >> OnChangeMember, and also disabled the Add button by default. This is > >> because it's enabled on loading the form which doesn't get disabled > >> again until you start editing the attribute name field, meaning you > >> can click Add without having provided any attribute details. > >> > > > > I'm wondering why you didn't take care of the Change button too. I mean, > > it's so easy now that you've the code working for the Add button that I > > kind of feel I'm missing something obvious which would explain why you > > didn't do it. Any particular reason other than "I forgot it"? :) > > Wasn't even that. It didn't cross my mind. Reattached with that change. > Thanks, commited. Only on master. Not sure it really qualifies as a bug. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On 18 July 2011 22:25, Guillaume Lelarge <guillaume@lelarge.info> wrote: > On Mon, 2011-07-18 at 22:15 +0100, Thom Brown wrote: >> On 18 July 2011 21:55, Guillaume Lelarge <guillaume@lelarge.info> wrote: >> > On Mon, 2011-07-18 at 21:06 +0100, Thom Brown wrote: >> >> On 14 July 2011 10:31, Thom Brown <thom@linux.com> wrote: >> >> > On 14 July 2011 10:24, Guillaume Lelarge <guillaume@lelarge.info> wrote: >> >> >> So, you're working on fixing it, right? >> >> > >> >> > Possibly ;) : http://code.pgadmin.org/trac/ticket/329 >> >> > >> >> > I haven't had time to look at fixing various bugs over the last couple >> >> > days but I'll get to them shortly. >> >> >> >> Apologies for the delay but I've now had a moment to look at it. >> > >> > No apologies needed :) >> > >> >> The >> >> attached patch removes the duplicate restriction from OnMemberAdd to >> >> OnChangeMember, and also disabled the Add button by default. This is >> >> because it's enabled on loading the form which doesn't get disabled >> >> again until you start editing the attribute name field, meaning you >> >> can click Add without having provided any attribute details. >> >> >> > >> > I'm wondering why you didn't take care of the Change button too. I mean, >> > it's so easy now that you've the code working for the Add button that I >> > kind of feel I'm missing something obvious which would explain why you >> > didn't do it. Any particular reason other than "I forgot it"? :) >> >> Wasn't even that. It didn't cross my mind. Reattached with that change. >> > > Thanks, commited. Only on master. Not sure it really qualifies as a bug. Cheers. :) -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company