C2025-027-AnnotationsTypeSelect #574

Merged
jjenko merged 10 commits from C2025-027-AnnotationsTypeSelect into Development 2025-07-30 08:23:07 -04:00
Owner

Develop a way to filter annotations so the user can view only the types they want to see.

Develop a way to filter annotations so the user can view only the types they want to see.
plarsen self-assigned this 2025-07-11 16:10:01 -04:00
djankowski was assigned by plarsen 2025-07-11 16:10:01 -04:00
jjenko was assigned by plarsen 2025-07-11 16:10:01 -04:00
mschill was assigned by plarsen 2025-07-11 16:10:01 -04:00
plarsen added 1 commit 2025-07-11 16:10:01 -04:00
plarsen requested review from jjenko 2025-07-11 16:10:22 -04:00
jjenko requested review from mschill 2025-07-11 16:34:04 -04:00
mschill reviewed 2025-07-14 07:56:27 -04:00
@@ -206,7 +206,7 @@ namespace VEPROMS.CSLA.Library
public partial class AnnotationAuditInfoListPropertyDescriptor : vlnListPropertyDescriptor
{
private AnnotationAuditInfo Item { get { return (AnnotationAuditInfo)_Item; } }
public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) { ;}
Owner

Not sure I understand why this change was made? / It appears to be the only change to the file?

Not sure I understand why this change was made? / It appears to be the only change to the file?
Owner

If you want to keep commented out code, you should a comment explaining why to keep it

If you want to keep commented out code, you should a comment explaining why to keep it
Author
Owner

a space was switched at the end of line 209. It is the same code as before. Not sure why that change is there because I did not make it.

Before: public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) { ;}

After: public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) {; }

a space was switched at the end of line 209. It is the same code as before. Not sure why that change is there because I did not make it. Before: public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) { ;} After: public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) {; }
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 07:56:58 -04:00
@@ -0,0 +1,226 @@
// ========================================================================
Owner

Not sure I understand why a _bak file was created and is being checked in for this?

Not sure I understand why a _bak file was created and is being checked in for this?
Author
Owner

Not sure how this got in GIT. It is no long in the project or on the file system.

Not sure how this got in GIT. It is no long in the project or on the file system.
Author
Owner

Sorry I see the file now. I will remove it.

Sorry I see the file now. I will remove it.
Author
Owner

Then odd thing is that I removed it from the project, but it got included in the pull request.

Then odd thing is that I removed it from the project, but it got included in the pull request.
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 07:57:59 -04:00
@@ -0,0 +67,4 @@
dt.Rows.Add(row);
}
//row = dt.NewRow();
Owner

unless reason for keeping the commented out code --- commented out code shouldn't be checked in?

(this would be throughout this file)

unless reason for keeping the commented out code --- commented out code shouldn't be checked in? (this would be throughout this file)
Author
Owner

I removed the commented code.

I removed the commented code.
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 07:59:28 -04:00
@@ -0,0 +16,4 @@
//namespace VEPROMS.CSLA.Library;
// C2025-027 this new file is used to support (data retrival) for selecting Annotation types to display on the Annotation screen.
Owner

Just to double check ---- Since there are many annotation screens, should we be more specific / say this is related to Annotation type filtering through V->Options?

Just to double check ---- Since there are many annotation screens, should we be more specific / say this is related to Annotation type filtering through V->Options?
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 08:05:16 -04:00
mschill reviewed 2025-07-14 08:10:56 -04:00
@@ -0,0 +204,4 @@
throw new DbCslaException("Error on AnnotationTypeInfoList.Get", ex);
}
}
private int _TypeID;
Owner

might be missing something ---- but Not sure I am understanding why these are compile time --- i.e. System.Runtime.CompilerServices.MethodImpl

seams like these would be class specific & if they are, should the methods that reference these not be "static" / i.e. could you end up with strange behavior if those methods are called multiple times (multiple users/open windows of the program)? ---- again, may not be the case, but figured I would double check?

might be missing something ---- but Not sure I am understanding why these are compile time --- i.e. System.Runtime.CompilerServices.MethodImpl seams like these would be class specific & if they are, should the methods that reference these not be "static" / i.e. could you end up with strange behavior if those methods are called multiple times (multiple users/open windows of the program)? ---- again, may not be the case, but figured I would double check?
Author
Owner

This code was borrowed code that is not being used. I may take it out. I have it in the file when I tried to use CSLA DataPortal_Fetch. I have run into too many problems trying to use CSLA. I will give it another try.

This code was borrowed code that is not being used. I may take it out. I have it in the file when I tried to use CSLA DataPortal_Fetch. I have run into too many problems trying to use CSLA. I will give it another try.
Author
Owner

I will remove it. I was initially trying CSLA, but CSLA is not easy to work with. I do not want to waste time to get CSLA to work on minor data requests.

I will remove it. I was initially trying CSLA, but CSLA is not easy to work with. I do not want to waste time to get CSLA to work on minor data requests.
Owner

I would recommend either it should follow CSLA or it should notmainly for 2 reasons:

  1. if it half follows CSLA, then we could run into problems down the line if we upgrade CSLA or replace it with something else
  2. CSLA is a little more bulky / heavy on memory / caching and other things ---- so if we aren't using it for DB access, then (unless there is a specific reason) why introduce the extra overhead.
I would recommend either it should follow CSLA or it should notmainly for 2 reasons: 1. if it half follows CSLA, then we could run into problems down the line if we upgrade CSLA or replace it with something else 2. CSLA is a little more bulky / heavy on memory / caching and other things ---- so if we aren't using it for DB access, then (unless there is a specific reason) why introduce the extra overhead.
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 08:20:10 -04:00
@@ -0,0 +361,4 @@
}
}
private void DataPortal_Fetch(retrieveAnnotSelections criteria)
Owner

may be missing something .... but if we are doing a "minimal" implementation --- i.e. not using csla (using sqlconnection and createcommand) for DB access, is there a reason we are using csla's DataPortal_Fetch?

may be missing something .... but if we are doing a "minimal" implementation --- i.e. not using csla (using sqlconnection and createcommand) for DB access, is there a reason we are using csla's DataPortal_Fetch?
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 08:20:26 -04:00
@@ -0,0 +404,4 @@
_itemID = itemID;
}
}
private void DataPortal_Fetch(AnnotationSelectByItemIDCriteria criteria)
Owner

may be missing something .... but if we are doing a "minimal" implementation --- i.e. not using csla (using sqlconnection and createcommand) for DB access, is there a reason we are using csla's DataPortal_Fetch?

may be missing something .... but if we are doing a "minimal" implementation --- i.e. not using csla (using sqlconnection and createcommand) for DB access, is there a reason we are using csla's DataPortal_Fetch?
Author
Owner

I will remove it. I was initially trying CSLA, but CSLA is not easy to work with. I do not want to waste time to get CSLA to work on minor data requests.

I will remove it. I was initially trying CSLA, but CSLA is not easy to work with. I do not want to waste time to get CSLA to work on minor data requests.
mschill marked this conversation as resolved
jjenko requested changes 2025-07-14 08:21:25 -04:00
Dismissed
jjenko left a comment
Owner

I agree with Matt's comments.
Please removed commented out code or add a comment with reason to keep
Also _bak files should not be checked in.

Done

I agree with Matt's comments. Please removed commented out code or add a comment with reason to keep Also _bak files should not be checked in. Done
mschill reviewed 2025-07-14 08:30:18 -04:00
@@ -123,1 +123,3 @@
}
// C2025-027
//cbGridAnnoType.DataSource = VEPROMS.CSLA.Library.AnnotationstypeSelections.Get(ProcItem.ItemID);
cbGridAnnoType.DataSource = VEPROMS.CSLA.Library.AnnotationstypeSelections.Get(MyUserInfo.UserID, ProcItem.ItemID);
Owner

this is set to a different DataSource in:
public void SetupAnnotations(DisplaySearch annosrch)

should this be there/replace that?

DisplayMember / ValueMember / etc... is set there - are they the same?

which should be the driving DataSource / (what would happen if a discrepancy between the two - for example if all annotation types were filtered out)?

this is set to a different DataSource in: public void SetupAnnotations(DisplaySearch annosrch) should this be there/replace that? DisplayMember / ValueMember / etc... is set there - are they the same? which should be the driving DataSource / (what would happen if a discrepancy between the two - for example if all annotation types were filtered out)?
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 08:36:43 -04:00
@@ -1296,7 +1296,38 @@ namespace VEPROMS
pi.MyDocVersion.DocVersionConfig.SelectedSlave = 0;
}
//void tv_SelectAnnotations(object sender, vlnTreeEventArgs args)
Owner

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in?

(this would be throughout this file)

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in? (this would be throughout this file)
mschill marked this conversation as resolved
plarsen added 1 commit 2025-07-15 23:13:13 -04:00
plarsen added 1 commit 2025-07-15 23:31:12 -04:00
mschill requested changes 2025-07-16 10:25:43 -04:00
Dismissed
mschill left a comment
Owner

I re-reviewed this morning.

see Pending comments - let me know if you have any questions.

I re-reviewed this morning. see Pending comments - let me know if you have any questions.
@@ -0,0 +14,4 @@
// C2025-027 Annotation Type Filtering
public partial class DlgAnnotationsSelect : Form
{
int AnnotationTypeID;
Owner

just to check - is there a reason these 2 items (the id and NameStr) are global variables / should stay in memory while the form is open?

It looks like they are only used in 1 function - update?

just to check - is there a reason these 2 items (the id and NameStr) are global variables / should stay in memory while the form is open? It looks like they are only used in 1 function - update?
Author
Owner

I moved them

I moved them
mschill marked this conversation as resolved
@@ -0,0 +25,4 @@
public DlgAnnotationsSelect(string userid)
{
InitializeComponent();
//MyItemID = pi.ItemID;
Owner

commented out code should be removed or reason given for why it is commented out / should be in the code long term.

commented out code should be removed or reason given for why it is commented out / should be in the code long term.
Author
Owner

Removed commented code

Removed commented code
mschill marked this conversation as resolved
@@ -0,0 +97,4 @@
// Save selected list to DB.
private void btnUpdate_Click(object sender, EventArgs e)
{
int dltFlg = 1;
Owner

I am not sure what this flag is?
I noticed it will be 1 on 1st iteration then 0 after that.

Can there be comments explaining why?

Flag removed

I am not sure what this flag is? I noticed it will be 1 on 1st iteration then 0 after that. Can there be comments explaining why? Flag removed
Owner

Must be missing something .... Not sure I understand why we are deleting items and reentering them ...... wouldn't we lose history on when they were originally put in if we do that?

When a user changes or adds to his selections, we need to remove his selections in the DB and insert the updates.

Must be missing something .... Not sure I understand why we are deleting items and reentering them ...... wouldn't we lose history on when they were originally put in if we do that? When a user changes or adds to his selections, we need to remove his selections in the DB and insert the updates.
Author
Owner

Delete and reinsert works very well. Updates in this scenario are nightmares.

Delete and reinsert works very well. Updates in this scenario are nightmares.
Owner

not sure if this would help --- but just a thought - passing in a whole table of the annotationtypeselections instead of doing 1 update/insert at a time and then doing a merge with the current table?

In the end, as long as Devin and John are good with it, I am too --- just figured I would check in case we were losing any needed info by removing and re-adding...

not sure if this would help --- but just a thought - passing in a whole table of the annotationtypeselections instead of doing 1 update/insert at a time and then doing a merge with the current table? In the end, as long as Devin and John are good with it, I am too --- just figured I would check in case we were losing any needed info by removing and re-adding...
Owner

Would remove dltFlg and commented out code since no longer used.

Would remove dltFlg and commented out code since no longer used.
Owner

As FYI - still valid

As FYI - still valid
Author
Owner

Flag removed

Flag removed
mschill marked this conversation as resolved
@@ -0,0 +101,4 @@
foreach (AnnotataionItem item in lstSelected.Items.OfType<AnnotataionItem>())
{
lstSelected.DisplayMember = "NameStr";
Owner

just to double check --- Is there a reason we are updating the DisplayMember / ValueMember every time we go through the loop?

Wasn't this already set when the form loaded --- line 160ish?

just to double check --- Is there a reason we are updating the DisplayMember / ValueMember every time we go through the loop? Wasn't this already set when the form loaded --- line 160ish?
Author
Owner

I moved the variables

I moved the variables
mschill marked this conversation as resolved
@@ -24078,0 +24129,4 @@
-- Description: Retrieve Current Annotation Types
-- =============================================
CREATE PROC [dbo].[getAnnotationstypeSelections]
Owner

So .... unless I have this backwards:
This is for filtering out Annotation Types --- so wouldn't anything selected in AnnotationTypeSelections need / be filtered out.

so basically, by default, we would show all annotation types / if a new annotation type was added it would show & only what is selected would be filtered out .......

if this is the case, wouldn't we want this info to come from AnnotationTypes
& be basically:

Select
AnnotationTypes.[TypeID],
AnnotationTypes.[Name],
AnnotationTypes.[Config],
AnnotationTypes.[DTS],
AnnotationTypes.[UserID],
AnnotationTypes.[LastChanged],
(SELECT COUNT(*) FROM [Annotations] WHERE [Annotations].[TypeID]=[AnnotationTypes].[TypeID]) [AnnotationCount],
AnnotationTypes.[IsEPAnnotationType]
FROM [AnnotationTypes]
LEFT OUTER JOIN AnnotationTypeSelections ON AnnotationTypeSelections.TypeID = AnnotationTypes.TypeID
WHERE AnnotationTypeSelections.UsrID = @UsrID
AND
AnnotationTypeSelections.ASTypeID IS NULL

----again, may be thinking of this wrong ... but was thinking we were storing in the db what they picked to filter out.

So .... unless I have this backwards: This is for filtering out Annotation Types --- so wouldn't anything selected in AnnotationTypeSelections need / be filtered out. so basically, by default, we would show all annotation types / if a new annotation type was added it would show & only what is selected would be filtered out ....... if this is the case, wouldn't we want this info to come from AnnotationTypes & be basically: Select AnnotationTypes.[TypeID], AnnotationTypes.[Name], AnnotationTypes.[Config], AnnotationTypes.[DTS], AnnotationTypes.[UserID], AnnotationTypes.[LastChanged], (SELECT COUNT(*) FROM [Annotations] WHERE [Annotations].[TypeID]=[AnnotationTypes].[TypeID]) [AnnotationCount], AnnotationTypes.[IsEPAnnotationType] FROM [AnnotationTypes] LEFT OUTER JOIN AnnotationTypeSelections ON AnnotationTypeSelections.TypeID = AnnotationTypes.TypeID WHERE AnnotationTypeSelections.UsrID = @UsrID AND AnnotationTypeSelections.ASTypeID IS NULL ----again, may be thinking of this wrong ... but was thinking we were storing in the db what they picked to filter out.
Author
Owner

SPs

[getAnnotationstypeSelections] select selected types for right listbox

getAnnotationSelectListTypes select from Annotationtypes table fitering out any selected types for left listbox

getAnnotationstypeFiltered select selected filtered (if any) otherwise show all annotation types. Used in AnnotationDetails drop down.

SPs [getAnnotationstypeSelections] select selected types for right listbox getAnnotationSelectListTypes select from Annotationtypes table fitering out any selected types for left listbox getAnnotationstypeFiltered select selected filtered (if any) otherwise show all annotation types. Used in AnnotationDetails drop down.
Owner

see next set of comments - these moved around due to changes since.

see next set of comments - these moved around due to changes since.
mschill marked this conversation as resolved
@@ -24078,0 +24232,4 @@
-- Description: Store user Annotation selections for annotation filter.
-- =============================================
CREATE TABLE [dbo].[AnnotationTypeSelections](
Owner

Should this table have any other indexes?

Would think that there could be a lot of joins by TypeID and User...

Also, what is the difference between:
UsrID
and
UserID?

Added an index

Should this table have any other indexes? Would think that there could be a lot of joins by TypeID and User... Also, what is the difference between: UsrID and UserID? Added an index
Author
Owner

UsrID is the id of the PROMS user. Userid is the user id that created the annotation type.

UsrID is the id of the PROMS user. Userid is the user id that created the annotation type.
Author
Owner

Added indexes

Added indexes
Owner

I would add them as 1 combined index as I don't think you would ever be providing one without the other ----- also, what should the included columns be for the index?

Should there be a unique index for UsrID, TypeID?

Also, should we check if Index exists before we create (i.e. if it already exists will this cause PROMSFixes to error)?

Done

I would add them as 1 combined index as I don't think you would ever be providing one without the other ----- also, what should the included columns be for the index? Should there be a unique index for UsrID, TypeID? Also, should we check if Index exists before we create (i.e. if it already exists will this cause PROMSFixes to error)? Done
Owner

UsrID is the id of the PROMS user. Userid is the user id that created the annotation type.

can we either --- use something else like CreatedBy or add a comment in the SQL.

> UsrID is the id of the PROMS user. Userid is the user id that created the annotation type. can we either --- use something else like CreatedBy or add a comment in the SQL.
Owner

I would add them as 1 combined index as I don't think you would ever be providing one without the other ----- also, what should the included columns be for the index?

Should there be a unique index for UsrID, TypeID?

Also, should we check if Index exists before we create (i.e. if it already exists will this cause PROMSFixes to error)?

Would need to figure out what is needed in include columns first (review where selecting from AnnotationTypeSelections and determine/update to be the minimum columns needed) --- but index should be similar to:

CREATE UNIQUE INDEX [idx_AnnotationTypeSelections_UsridTypeID] ON [dbo].[AnnotationTypeSelections]
(
[UsrID] ASC,
[TypeID] ASC
)
INCLUDE (ASTypeID, ...)
WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, SORT_IN_TEMPDB = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]

> I would add them as 1 combined index as I don't think you would ever be providing one without the other ----- also, what should the included columns be for the index? > > Should there be a unique index for UsrID, TypeID? > > Also, should we check if Index exists before we create (i.e. if it already exists will this cause PROMSFixes to error)? Would need to figure out what is needed in include columns first (review where selecting from AnnotationTypeSelections and determine/update to be the minimum columns needed) --- but index should be similar to: CREATE UNIQUE INDEX [idx_AnnotationTypeSelections_UsridTypeID] ON [dbo].[AnnotationTypeSelections] ( [UsrID] ASC, [TypeID] ASC ) INCLUDE (ASTypeID, ...) WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, SORT_IN_TEMPDB = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
Owner

As FYI - still valid

As FYI - still valid
mschill marked this conversation as resolved
@@ -0,0 +1,190 @@

namespace VEPROMS
Owner

Naming/casing should be consistent:
class is:
DlgAnnotationsSelect
but
designer is
dlgAnnotationsSelect?

Naming/casing should be consistent: class is: DlgAnnotationsSelect but designer is dlgAnnotationsSelect?
Author
Owner

Fixed

Fixed
mschill marked this conversation as resolved
@@ -1346,3 +1404,4 @@
private DevComponents.DotNetBar.Controls.CheckBoxX cbOTRemember;
private DevComponents.DotNetBar.Controls.CheckBoxX cbOTAutoOpen;
private DevComponents.DotNetBar.Controls.CheckBoxX cbShwRplWrdsColor;
//private DevComponents.DotNetBar.ButtonItem cbShwAnnoFilter;
Owner

commented out code should be removed or reason given for why it is commented out / should be in the code long term.

may be other places in this file as well - this is just where I noticed it.

commented out code should be removed or reason given for why it is commented out / should be in the code long term. may be other places in this file as well - this is just where I noticed it.
Author
Owner

removed comments

removed comments
mschill marked this conversation as resolved
@@ -366,3 +366,3 @@
Properties.Settings.Default.Save(); // save settings
}
}
private void cbShwAnnoFilter_Click(object sender, EventArgs e)
Owner

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in?

(this would be throughout this file)

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in? (this would be throughout this file)
Owner

Now, this is a method with everything inside commented out --- if no code is to execute, should the method be removed?

Now, this is a method with everything inside commented out --- if no code is to execute, should the method be removed?
mschill marked this conversation as resolved
@@ -369,0 +376,4 @@
//Properties.Settings.Default.Save(); // save settings
}
private void buttonX1_Click(object sender, EventArgs e)
Owner

Should this be named something other than buttonX1?

Should this be named something other than buttonX1?
Author
Owner

Fixed

Fixed
mschill marked this conversation as resolved
@@ -544,3 +544,3 @@
tv.ProcedureCheckedOutTo += new vlnTreeViewEvent(tv_ProcedureCheckedOutTo);
tv.ViewPDF += new vlnTreeViewPdfEvent(tv_ViewPDF);
//tv.SelectAnnotations += new (tv_SelectAnnotations);
Owner

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in?

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in?
mschill marked this conversation as resolved
@@ -0,0 +41,4 @@
// if the user has not created a annotation sub-set list saved to AnnotationTypeSelections table.
if (dt.Rows.Count < 1)
{
//dt.Rows.Add(DataPortal.Fetch<AnnotationTypeInfoList>());
Owner

commented out code should be removed or reason given for why it is commented out / should be in the code long term.

commented out code should be removed or reason given for why it is commented out / should be in the code long term.
Author
Owner

Comment removed.

Comment removed.
Owner

still appears to be:

//dt.Rows.Add(DataPortal.Fetch());
//DataPortal.Fetch();

still appears to be: //dt.Rows.Add(DataPortal.Fetch<AnnotationTypeInfoList>()); //DataPortal.Fetch<AnnotationTypeInfoList>();
mschill marked this conversation as resolved
@@ -122,2 +122,2 @@
ProcItem = CurrentItem.MyProcedure;
}
//ProcItem = CurrentItem.MyProcedure;
Owner

just to double check - was this intended to be commented out?

just to double check - was this intended to be commented out?
Owner

was this (ProcItem = CurrentItem.MyProcedure;) intended to be removed?

was this (ProcItem = CurrentItem.MyProcedure;) intended to be removed?
Author
Owner

Yes it did not work where I moved the code to retrieve selected anno types for the annotation details. Also it was not needed any more.

Yes it did not work where I moved the code to retrieve selected anno types for the annotation details. Also it was not needed any more.
Owner

sounds good. Just wanted to double check since this existed before your changes that removing it wasn't going to impact something else.

sounds good. Just wanted to double check since this existed before your changes that removing it wasn't going to impact something else.
mschill marked this conversation as resolved
@@ -124,0 +123,4 @@
//ProcItem = CurrentItem.MyProcedure;
// C2025-027 Annotation Type Filtering
//cbGridAnnoType.WatermarkText = "Select Annotation Type";
//cbGridAnnoType.DataSource = VEPROMS.CSLA.Library.AnnotationstypeSelections.Get(MyUserInfo.UserID);
Owner

commented out code should be removed or reason given for why it is commented out / should be in the code long term. (lines 122-127)

commented out code should be removed or reason given for why it is commented out / should be in the code long term. (lines 122-127)
Author
Owner

Fixed

Fixed
mschill marked this conversation as resolved
@@ -365,6 +370,12 @@ namespace Volian.Controls.Library
cbGridAnnoType.DisplayMember = "Name";
cbGridAnnoType.ValueMember = "TypeId";
cbGridAnnoType.DataSource = AnnotationTypeInfoList.Get().Clone();
Owner

this should be removed if data set is set to something else (see 2 lines after this)

this should be removed if data set is set to something else (see 2 lines after this)
Author
Owner

Data set is the same

Data set is the same
Owner

this line is:
cbGridAnnoType.DataSource = AnnotationTypeInfoList.Get().Clone();

2-4 lines below have:

cbGridAnnoType.DataSource = VEPROMS.CSLA.Library.AnnotationstypeSelections.Get(MyUserInfo.UserID);

Why are we setting it to something only to re-set it to something else 2-4 lines later?

this line is: cbGridAnnoType.DataSource = AnnotationTypeInfoList.Get().Clone(); 2-4 lines below have: cbGridAnnoType.DataSource = VEPROMS.CSLA.Library.AnnotationstypeSelections.Get(MyUserInfo.UserID); Why are we setting it to something only to re-set it to something else 2-4 lines later?
mschill marked this conversation as resolved
plarsen added 1 commit 2025-07-16 15:02:16 -04:00
plarsen added 2 commits 2025-07-22 09:12:54 -04:00
mschill requested changes 2025-07-22 10:29:01 -04:00
Dismissed
mschill left a comment
Owner

Let me know if you have any questions on my added notes.

Let me know if you have any questions on my added notes.
@@ -0,0 +15,4 @@
public partial class dlgAnnotationsSelect : Form
{
//int AnnotationTypeID;
//string AnnotationNameStr = "";
Owner

commented out code should be removed or reason given for why it is commented out / should be in the code long term.

commented out code should be removed or reason given for why it is commented out / should be in the code long term.
Author
Owner

Comment removed.

Comment removed.
mschill marked this conversation as resolved
@@ -0,0 +162,4 @@
lstSelected.DisplayMember = "NameStr";
lstSelected.ValueMember = "TypeID";
DataTable lstSelectedTbl = VEPROMS.CSLA.Library.AnnotationstypeSelections.Retrieve(UserID);
if (lstSelectedTbl.Rows.Count > 0)
Owner

Shouldn't need this as foreach loop below should loop 0 times if no rows.

Shouldn't need this as foreach loop below should loop 0 times if no rows.
Author
Owner

That look is used. It retrieves from AnnotationstypeSelections table.

That look is used. It retrieves from AnnotationstypeSelections table.
mschill marked this conversation as resolved
@@ -24078,0 +24100,4 @@
[ASTypeID] [int] IDENTITY(1,1) NOT NULL,
[TypeID] [int] NULL,
[UsrID] [varchar](50) NULL,
[Name] [nvarchar](100) NULL,
Owner

I could be wrong but I thought some of these were being removed and joins done to the AnnotationType table to get things like Name, IsEpAnnotationType, etc...?

I could be wrong but I thought some of these were being removed and joins done to the AnnotationType table to get things like Name, IsEpAnnotationType, etc...?
Owner

This is still valid.

I thought Name was being removed from AnntationsSelections and just going to be pulled from AnnotationsType?

Also, not sure what Config is for here?

This is still valid. I thought Name was being removed from AnntationsSelections and just going to be pulled from AnnotationsType? Also, not sure what Config is for here?
mschill marked this conversation as resolved
@@ -24078,0 +24109,4 @@
) ON [PRIMARY] TEXTIMAGE_ON [PRIMARY]
GO
--CREATE UNIQUE INDEX idx_AnnotationTypeSelections_Usrid
Owner

if commented out, should be removed or reason given why commented out?

if commented out, should be removed or reason given why commented out?
Author
Owner

Comment removed.

Comment removed.
mschill marked this conversation as resolved
@@ -24078,0 +24119,4 @@
DROP INDEX [idx_AnnotationTypeSelections_UsrID] ON [dbo].[AnnotationTypeSelections];
end
CREATE NONCLUSTERED INDEX idx_AnnotationTypeSelections_UsrID
Owner

Wouldn't you want the index on UsrID,TypeID?

I also thought name was being removed from this table?

see previous notes / wouldn;t this be UNIQUE by UsrID, TypeID

Wouldn't you want the index on UsrID,TypeID? I also thought name was being removed from this table? see previous notes / wouldn;t this be UNIQUE by UsrID, TypeID
Author
Owner

Added an index.

Added an index.
Owner

Comment still valid:

index should be UNIQUE by
(1) UsrID, TypeID
or
(2) TypeID, UsrID
and INCLUDE other columns pulled back like:
ASTypeID

for whether it should be (1) or (2), I suggest trying each individually and looking at the execution plan to see which is grabbed / better --- if you need assistance with this, please let me know and I can give assistance as I am quite familiar with looking at the execution plans in SQL,

Comment still valid: index should be UNIQUE by (1) UsrID, TypeID or (2) TypeID, UsrID and INCLUDE other columns pulled back like: ASTypeID for whether it should be (1) or (2), I suggest trying each individually and looking at the execution plan to see which is grabbed / better --- if you need assistance with this, please let me know and I can give assistance as I am quite familiar with looking at the execution plans in SQL,
Owner

There is another comment around line 24235that is related to this. It just moved since there have been changes since that comment was made. That one should have likely the exact index needed.

There is another comment around line 24235that is related to this. It just moved since there have been changes since that comment was made. That one should have likely the exact index needed.
mschill marked this conversation as resolved
@@ -24078,0 +24124,4 @@
INCLUDE (TypeID, Name)
GO
--CREATE NONCLUSTERED INDEX [idx_AnnotationTypeSelections_Usrid] ON [dbo].[AnnotationTypeSelections]
Owner

if commented out, should be removed or reason given why commented out?

if commented out, should be removed or reason given why commented out?
mschill marked this conversation as resolved
@@ -24078,0 +24130,4 @@
--)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, SORT_IN_TEMPDB = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
--GO
--CREATE NONCLUSTERED INDEX [idx_AnnotationTypeSelections_TypeID] ON [dbo].[AnnotationTypeSelections]
--(
Owner

If commented out, would remove or give reason why commented out?

If commented out, would remove or give reason why commented out?
Author
Owner

Comment removed.

Comment removed.
mschill marked this conversation as resolved
@@ -24078,0 +24160,4 @@
[LastChanged],
(SELECT COUNT(*) FROM [Annotations] WHERE [Annotations].[TypeID]= [TypeID]) [AnnotationCount],
[IsEPAnnotationType]
FROM [AnnotationTypes] --A
Owner

see earlier note - will get better performance if
LEFT OUTER JOIN AnnotationTypeSelections ON AnnotationTypeSelections.TypeID = AnnotationTypes.TypeID

than use a subquery.

see earlier note - will get better performance if LEFT OUTER JOIN AnnotationTypeSelections ON AnnotationTypeSelections.TypeID = AnnotationTypes.TypeID than use a subquery.
Author
Owner

Changed query.

Changed query.
Owner

still valid - this refers to the query in getAnnotationSelectListTypes
around line:
24148

still valid - this refers to the query in getAnnotationSelectListTypes around line: 24148
mschill marked this conversation as resolved
@@ -24078,0 +24220,4 @@
[ASTypeID] [int] IDENTITY(1,1) NOT NULL,
[TypeID] [int] NULL,
[UsrID] [varchar](50) NULL,
[Name] [nvarchar](100) NOT NULL,
Owner

Can name be different than the AnnotationTypes.Name?

Basically, if have a separate field for it -- what happens if someone
selects:
Annotation: TypeID = 5, Name="TEST1"
to be filtered
then
changes the Name of the Annotation Type to be "TEST3"
..
I believe then it would be "TEST3" in PROMS
but the option in the filtering list would say "TEST1"

Can name be different than the AnnotationTypes.Name? Basically, if have a separate field for it -- what happens if someone selects: Annotation: TypeID = 5, Name="TEST1" to be filtered then changes the Name of the Annotation Type to be "TEST3" .. I believe then it would be "TEST3" in PROMS but the option in the filtering list would say "TEST1"
Author
Owner

The all annotation type names in the form are pulled from AnnotationTypes table.

The all annotation type names in the form are pulled from AnnotationTypes table.
mschill marked this conversation as resolved
@@ -24078,0 +24221,4 @@
[TypeID] [int] NULL,
[UsrID] [varchar](50) NULL,
[Name] [nvarchar](100) NOT NULL,
[Config] [nvarchar](max) NULL,
Owner

Can there be Config Data for this?

Can there be Config Data for this?
mschill marked this conversation as resolved
@@ -24078,0 +24225,4 @@
[DTS] [datetime] NOT NULL,
[UserID] [nvarchar](100) NOT NULL,
[LastChanged] [timestamp] NOT NULL,
[IsEPAnnotationType] [bit] NOT NULL
Owner

I am not sure why this has an IsEPAnnotationType?

Wouldn't we just use the flag in AnnotationTypes?

I am not sure why this has an IsEPAnnotationType? Wouldn't we just use the flag in AnnotationTypes?
Author
Owner

Removed it.

Removed it.
Author
Owner

Removed IsEPAnnotationType from AnnotationTypeSelections table.

Removed IsEPAnnotationType from AnnotationTypeSelections table.
mschill marked this conversation as resolved
@@ -24078,0 +24259,4 @@
--GO
--/****** Object: UserDefinedTableType [dbo].[TableValAnnotTypeSelections] Script Date: 7/21/2025 8:06:11 PM ******/
--CREATE TYPE [dbo].[TableValAnnotTypeSelections] AS TABLE(
Owner

If commented out, would remove or give reason why commented out?

If commented out, would remove or give reason why commented out?
Author
Owner

Removed it

Removed it
mschill marked this conversation as resolved
@@ -24078,0 +24275,4 @@
IF EXISTS( SELECT * FROM INFORMATION_SCHEMA.DOMAINS WHERE Domain_Name = 'TableValAnnotTypeSelections' )
DROP TYPE [dbo].[TableValAnnotTypeSelections]
CREATE TYPE [dbo].[TableValAnnotTypeSelections] AS TABLE(
Owner

So, shouldn't NameStr not be part of the AnnotationTypeSelections and instead pull from AnnotationTypes?

(As could have situation where the Names become out of sink if it is stored multiple places)

So, shouldn't NameStr not be part of the AnnotationTypeSelections and instead pull from AnnotationTypes? (As could have situation where the Names become out of sink if it is stored multiple places)
Author
Owner

I left it in the table.

I left it in the table.
Owner

would not loop through this is will perform poorly and should not be necessary. It looks like my previous comment got removed with this change as it is now past the end of this file, so I will reply to this shortly with that same comment again.

would not loop through this is will perform poorly and should not be necessary. It looks like my previous comment got removed with this change as it is now past the end of this file, so I will reply to this shortly with that same comment again.
Owner

would think would want to do something like:

--this would delete only the ones for this User that are not in the uploaded table
DELETE FROM AnnotationTypeSelections where UsrID in
(Select UsrID From @TempTable tmp)
AND
TypeID not in
(Select TypeID From @TempTable tmp)

--this would insert all the ones that are in the uploaded table and not already in AnnotationTypeSelections
Insert INTO AnnotationTypeSelections (TypeID, UsrID)
Select TypeID, UserID
FROM
@TempTable tmp
LEFT OUTER JOIN
AnnotationTypeSelections ATS on ATS.TypeID = tmp.TypeID
AND ATS.UsrID = tmp.UsrID
where
ATS.ASTypeID IS NULL

would think would want to do something like: --this would delete only the ones for this User that are not in the uploaded table DELETE FROM AnnotationTypeSelections where UsrID in (Select UsrID From @TempTable tmp) AND TypeID not in (Select TypeID From @TempTable tmp) --this would insert all the ones that are in the uploaded table and not already in AnnotationTypeSelections Insert INTO AnnotationTypeSelections (TypeID, UsrID) Select TypeID, UserID FROM @TempTable tmp LEFT OUTER JOIN AnnotationTypeSelections ATS on ATS.TypeID = tmp.TypeID AND ATS.UsrID = tmp.UsrID where ATS.ASTypeID IS NULL
Owner

Also as a heads up - I sent an email with an attachment --- looks like there is a side consequence to having Userid be part of the table type and not separate --- that side consequence occurs if you have items selected and then want to de-select all of the items.

Also as a heads up - I sent an email with an attachment --- looks like there is a side consequence to having Userid be part of the table type and not separate --- that side consequence occurs if you have items selected and then want to de-select all of the items.
mschill marked this conversation as resolved
@@ -24078,0 +24302,4 @@
AS
BEGIN
--INSERT INTO CUSTOMER (CustomerId,CustomerName ,Isdeleted )
Owner

If commented out, would remove or give reason why commented out?

If commented out, would remove or give reason why commented out?
Author
Owner

Removed it

Removed it
mschill marked this conversation as resolved
@@ -24078,0 +24333,4 @@
/* When no records are matched with SOURCE table
Then delete the records from the target table */
WHEN NOT MATCHED BY SOURCE
Owner

Wouldn't this delete ones by other users?

would think would want to do something like:

DELETE FROM AnnotationTypeSelections where UsrID in
(Select UsrID From @TempTable tmp)
AND
TypeID not in
(Select TypeID From @TempTable tmp)

Insert INTO AnnotationTypeSelections (TypeID, UsrID)
Select TypeID, UserID
FROM
@TempTable tmp
LEFT OUTER JOIN
AnnotationTypeSelections on AnnotationTypeSelections.TypeID = tmp.TypeID
AND AnnotationTypeSelections.UsrID = tmp.UsrID
where
ASTypeID IS NULL

Wouldn't this delete ones by other users? would think would want to do something like: DELETE FROM AnnotationTypeSelections where UsrID in (Select UsrID From @TempTable tmp) AND TypeID not in (Select TypeID From @TempTable tmp) Insert INTO AnnotationTypeSelections (TypeID, UsrID) Select TypeID, UserID FROM @TempTable tmp LEFT OUTER JOIN AnnotationTypeSelections on AnnotationTypeSelections.TypeID = tmp.TypeID AND AnnotationTypeSelections.UsrID = tmp.UsrID where ASTypeID IS NULL
Author
Owner

Made changes to the insert statement in the SP.

Made changes to the insert statement in the SP.
mschill marked this conversation as resolved
plarsen added 1 commit 2025-07-26 19:43:43 -04:00
mschill requested changes 2025-07-28 08:17:55 -04:00
Dismissed
mschill left a comment
Owner

See Comments.

One comment in DlgAnnotationsSelect.cs
A couple small comments in AnnotationTypeSections.cs

and several in PROMSFixes.Sql

Please let me know if you have any questions.

See Comments. One comment in DlgAnnotationsSelect.cs A couple small comments in AnnotationTypeSections.cs and several in PROMSFixes.Sql Please let me know if you have any questions.
@@ -0,0 +145,4 @@
lstSelected.DisplayMember = "NameStr";
lstSelected.ValueMember = "TypeID";
DataTable lstSelectedTbl = VEPROMS.CSLA.Library.AnnotationstypeSelections.Retrieve(UserID);
if (lstSelectedTbl.Rows.Count > 0)
Owner

I closed this previous comment since it moved down and reposted here - "Shouldn't need this as foreach loop below should loop 0 times if no rows."

This comment moved down thanks to changes since --- this is referring to the if statement now on line 148 - if (lstSelectedTbl.Rows.Count > 0)
since next statement is
foreach (DataRow lstSelectedRow in lstSelectedTbl.Rows)

I closed this previous comment since it moved down and reposted here - "Shouldn't need this as foreach loop below should loop 0 times if no rows." This comment moved down thanks to changes since --- this is referring to the if statement now on line 148 - if (lstSelectedTbl.Rows.Count > 0) since next statement is foreach (DataRow lstSelectedRow in lstSelectedTbl.Rows)
mschill marked this conversation as resolved
@@ -0,0 +164,4 @@
int RowID = 0;
DataTable dt = new DataTable();
dt.Columns.Add("TypeID", typeof(Int32));
dt.Columns.Add("NameStr", typeof(string));
Owner

I thought Name was being removed from AnntationsSelections and just going to be pulled from AnnotationsType?

Also, not quite sure why RowID is needed / was added?
Should only need the TypeID and UserID unless I am missing something?

I thought Name was being removed from AnntationsSelections and just going to be pulled from AnnotationsType? Also, not quite sure why RowID is needed / was added? Should only need the TypeID and UserID unless I am missing something?
mschill marked this conversation as resolved
@@ -24078,0 +24103,4 @@
[Name] [nvarchar](100) NULL,
[Config] [nvarchar](max) NULL,
[DTS] [datetime] NULL,
[UserID] [nvarchar](100) NULL,
Owner

Would suggest not having columns named both UserID and UserID in the same table as it is confusing which is the function of each,
Would suggest renaming UserID as something like CreatedBy.

Would suggest not having columns named both UserID and UserID in the same table as it is confusing which is the function of each, Would suggest renaming UserID as something like CreatedBy.
mschill marked this conversation as resolved
@@ -24078,0 +24242,4 @@
CREATE TYPE [dbo].[TableValAnnotTypeSelections] AS TABLE(
[TypeID] [int] NOT NULL,
[NameStr] [varchar](200) NULL,
Owner

Shouldn't need NameStr or RowID unless I am missing something as these are not columns in AnnotationSelections

Shouldn't need NameStr or RowID unless I am missing something as these are not columns in AnnotationSelections
mschill marked this conversation as resolved
@@ -24078,0 +24246,4 @@
[UserID] [varchar](50) NULL,
[RowID] [int] NOT NULL,
PRIMARY KEY CLUSTERED
(
Owner

shouldn't need a primary key on a type?

shouldn't need a primary key on a type?
mschill marked this conversation as resolved
@@ -24078,0 +24270,4 @@
DECLARE @cnt2 integer = 0
SET @cnt = (SELECT count(*) from @TempTable)
DECLARE @UserID VARCHAR(50) = (SELECT TOP 1 UserID FROM @TempTable)
DELETE FROM AnnotationTypeSelections WHERE usrID = @UserID;
Owner

only need to delete the ones not already in the table. If we delete and re-add then will lose history of when truely added --- see comment below (around line 24278) for more complete,

only need to delete the ones not already in the table. If we delete and re-add then will lose history of when truely added --- see comment below (around line 24278) for more complete,
mschill marked this conversation as resolved
@@ -0,0 +151,4 @@
{
using (SqlCommand cm = cn.CreateCommand())
{
try
Owner

if have a try catch, should there be something in the catch?

if have a try catch, should there be something in the catch?
mschill marked this conversation as resolved
@@ -0,0 +177,4 @@
{
using (SqlCommand cm = cn.CreateCommand())
{
try
Owner

if have a try catch, should there be something in the catch?

if have a try catch, should there be something in the catch?
mschill marked this conversation as resolved
mschill reviewed 2025-07-28 08:19:21 -04:00
@@ -0,0 +168,4 @@
dt.Columns.Add("UserID", typeof(string));
dt.Columns.Add("RowID", typeof(string));
foreach (AnnotataionItem item in lstSelected.Items.OfType<AnnotataionItem>())
Owner

see comments in SQL --- shouldn't need RowID

see comments in SQL --- shouldn't need RowID
mschill marked this conversation as resolved
mschill reviewed 2025-07-28 09:25:10 -04:00
@@ -0,0 +145,4 @@
}
}
}
//public static void Update(string UserID, int TypeID, int dltFlg, string Name = "")
Owner

if commented out, should this be removed?

if commented out, should this be removed?
mschill marked this conversation as resolved
mschill reviewed 2025-07-28 13:18:38 -04:00
@@ -24078,0 +24080,4 @@
-- C2025-027 Annotation Type Filtering
IF EXISTS (SELECT * FROM sys.objects WHERE object_id = OBJECT_ID(N'[dbo].[AnnotationTypeSelections]') AND type in (N'U'))
DROP TABLE [dbo].[AnnotationTypeSelections]
Owner

won't this wipe out all thier pre-existing data every time PROMSFixes is run?

won't this wipe out all thier pre-existing data every time PROMSFixes is run?
mschill marked this conversation as resolved
plarsen added 2 commits 2025-07-29 15:46:08 -04:00
mschill reviewed 2025-07-29 15:59:03 -04:00
mschill left a comment
Owner

Looks good overall.

One very small tweak ----

remove:

, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF

from the one table --- this was what we caught wasn't compatible with SQL Server 2016.

Looks good overall. One very small tweak ---- remove: , OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF from the one table --- this was what we caught wasn't compatible with SQL Server 2016.
@@ -24078,0 +24116,4 @@
[TypeID] ASC
)
INCLUDE (ASTypeID)
WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, SORT_IN_TEMPDB = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
Owner

remove:

, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF

This was what we caught was not compatible with SQL 2016

remove: , OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF This was what we caught was not compatible with SQL 2016
mschill marked this conversation as resolved
plarsen added 1 commit 2025-07-29 22:53:36 -04:00
mschill approved these changes 2025-07-30 07:50:27 -04:00
mschill left a comment
Owner

Looks Good.

Looks Good.
jjenko approved these changes 2025-07-30 08:22:50 -04:00
jjenko left a comment
Owner

New code and changes look good.

New code and changes look good.
jjenko merged commit 453dce9520 into Development 2025-07-30 08:23:07 -04:00
Member

Tested with Version 2.2.2507.3011 and SQL fixes 2507.1014 (these SQL fixes were modified on 7/30). Works properly, but C2025-046 has been submitted for changing names of items within the feature.

Tested with Version 2.2.2507.3011 and SQL fixes 2507.1014 (these SQL fixes were modified on 7/30). Works properly, but C2025-046 has been submitted for changing names of items within the feature.
Sign in to join this conversation.
No Reviewers
No Label
4 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Volian/SourceCode#574
No description provided.