Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISHEvent and ISHBackgroundTask field information is checked to add th… #172

Conversation

ArianArt
Copy link
Contributor

…em to theishTypeFieldDefinitions.

xmlDocument.LoadXml(xmlIshFieldSetup);

if (xmlDocument.SelectSingleNode("//ishtypedefinition[@name='ISHEvent']") != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Detecting based on the Xml means that it is API 2.5 (SOAP) API dependent... When this code is rewired to OpenAPI (JSON) then we have to adjust this code as well

AddIshBackgroundTaskTableFieldSetup();
AddIshEventTableFieldSetup();
AddIshTranslationJobItemTableFieldSetup();
if (!hasIshBackgroundTaskFieldDefinitions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using the earlier "detected" boolean (via de xml xpath expression) ... can you not simply check if the _ishTypeFieldDefinitions object at this point already holds ISHEvent and use that as the if-clause to decide the hardcoded piece or not...

if (!hasIshBackgroundTaskFieldDefinitions)
{ AddIshBackgroundTaskTableFieldSetup(); }
if (!hasIshEventFieldDefinitions)
{ AddIshEventTableFieldSetup(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Previous code had

  1. AddIshBackgroundTaskTableFieldSetup
  2. AddIshEventTableFieldSetup
  3. AddIshTranslationJobItemTableFieldSetup <-- where is this in your code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Previously we had an empty method AddIshTranslationJobItemTableFieldSetup which is not used...so do you really want to keep this empty method and put a check around it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Although commented out, it was still a building block on route to #69 ... preferably the RetrieveTypeFieldSetup API call offers these two object types as well.

ishTypeFieldDefinitions.Add(new IshTypeFieldDefinition(_logger, Enumerations.ISHType.ISHBackgroundTask, Enumerations.Level.History, false, false, true, false, false, false, false, true, true, false, "STARTDATE", Enumerations.DataType.DateTime, "", "", "The date time that this execution of the background task was started"));
ishTypeFieldDefinitions.Add(new IshTypeFieldDefinition(_logger, Enumerations.ISHType.ISHBackgroundTask, Enumerations.Level.History, false, false, true, false, false, false, false, true, true, false, "ENDDATE", Enumerations.DataType.DateTime, "", "", "The date time that this execution of the background task was finished"));
ishTypeFieldDefinitions.Add(new IshTypeFieldDefinition(_logger, Enumerations.ISHType.ISHBackgroundTask, Enumerations.Level.History, false, false, true, false, false, false, false, true, true, false, "HOSTNAME", Enumerations.DataType.String, "", "", "The host name from which the background task was created"));
var ishTypeFieldDefinitions = new List<IshTypeFieldDefinition>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I assume that this change is just layout, and not actual changes... did not compare line-by-line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The layout is changed to resolve the warning.

ishTypeFieldDefinitions.Add(new IshTypeFieldDefinition(_logger, Enumerations.ISHType.ISHBackgroundTask, Enumerations.Level.History, false, false, true, false, false, false, true, true, false, "STARTDATE", Enumerations.DataType.DateTime, "", "The date time that this execution of the background task was started"));
ishTypeFieldDefinitions.Add(new IshTypeFieldDefinition(_logger, Enumerations.ISHType.ISHBackgroundTask, Enumerations.Level.History, false, false, true, false, false, false, true, true, false, "ENDDATE", Enumerations.DataType.DateTime, "", "The date time that this execution of the background task was finished"));
ishTypeFieldDefinitions.Add(new IshTypeFieldDefinition(_logger, Enumerations.ISHType.ISHBackgroundTask, Enumerations.Level.History, false, false, true, false, false, false, true, true, false, "HOSTNAME", Enumerations.DataType.String, "", "The host name from which the background task was created"));
//internal IshTypeFieldDefinition(ILogger logger, Enumerations.ISHType ishType, Enumerations.Level level, bool isMandatory, bool isMultiValue, bool allowOnRead, bool allowOnCreate, bool allowOnUpdate, bool allowOnSearch, bool isSystem, bool isBasic, bool isDescriptive, string name, Enumerations.DataType dataType, string referenceLov, string description)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here... only layout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, only layout..

foreach (var ishTypeFieldDefinition in ishTypeFieldDefinitions)
{
{
_ishTypeFieldDefinitions.Add(ishTypeFieldDefinition.Key, ishTypeFieldDefinition);
}
}

private void AddIshEventTableFieldSetup()
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is AddIshTranslationJobItemTableFieldSetup() function as mentioned earlier?

… to see if field information is needed to be added manually.
@ArianArt ArianArt merged commit 018d4eb into master Oct 16, 2023
1 check passed
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.

4 participants