Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Conversation

jarifibrahim
Copy link
Member

@jarifibrahim jarifibrahim commented Oct 25, 2018

This is the first PR in a series of PR to rename all system.* fields to system_* fields.
I've broken the changes into separate PRs so that it can be easily reviewed.

This PR will be merged to osio-story-743-rename-fields branch. The tests would fail because this is not the complete implementation. Once all changes have been merged to osio-story-743-rename-fields branch, I'll create a new PR to merge this branch into master. I hope this will make it easier to review the code.

See #2177 and #2177 (comment)

This PR does not change any golden files. They will be updated in #2333 PR.

@jarifibrahim jarifibrahim self-assigned this Oct 25, 2018
@alien-ike
Copy link

alien-ike commented Oct 25, 2018

Ike Plugins (test-keeper)

Thank you @jarifibrahim for this contribution!

It seems that this PR already contains some added or changed tests. Good job!

For more information please head over to official documentation. You can find there how to configure the plugin.

@jarifibrahim jarifibrahim requested a review from kwk October 25, 2018 07:48
@jarifibrahim jarifibrahim changed the title Rename system.* to system_* in a few files Rename system.* to system_* in a some files Oct 25, 2018
Copy link
Collaborator

@kwk kwk left a comment

Choose a reason for hiding this comment

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

LGTM

RemoteAssigneeLogins = "system.assignees.login"
RemoteAssigneeProfileURLs = "system.assignees.profile_url"
remoteCreatorLogin = "system_creator_login"
remoteCreatorProfileURL = "system_creator_profile_url"
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed on a call last Friday, please beware that we cannot simply replace system. with system_ but that we also might have to deal with multiple JSONAPI violations per field name.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kwk Yes, I've created a static map that would deal with mapping the fields for now 0433e16


if strings.Contains(fieldName, ".") {
// Check if the field name contains an underscore
if strings.Contains(fieldName, "_") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This reminds me that this piece of code needs some special treatment as the logic was previously to only handle system.XXX fields here and to distinguish between those fields that map to columns inside the work_items table and those that actually go to the fields column.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kwk I guess you're talking about this piece of code

mappedFieldName, isColumnField := fieldMap[fieldName]
if isColumnField {
return Column(WorkItemStorage{}.TableName(), mappedFieldName), false
}
if strings.Contains(fieldName, ".") {
// leave field untouched
return fieldName, true
}

Do you think I should fix something? (I couldn't understand what did you want me to change)

Copy link
Collaborator

Choose a reason for hiding this comment

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

No No, this was just a mental note to myself.

@jarifibrahim jarifibrahim merged commit 5300ce5 into fabric8-services:osio-story-743-rename-fields Oct 29, 2018
jarifibrahim added a commit that referenced this pull request Jan 22, 2019
* Rename system.* to system_* in a all files (#2330)

* Update branch osio-story-743-rename-fields (#2350)

* Add temporary code for field rename (#2333)

    * Add replaceFieldName and addFieldName with tests

    * Remove unnecessary log.Error from golden_files_test.go

    * Fix failing test in expression_compiler_blackbox_test.go

    * Skip FullTextSearch and iteration test until migrations are added

    * Add golden files with old and new field names

    * Add todo to skipped tests

    * Add foo.bar test for expression_compiler


* Update /workitemtype response for field rename (#2335)

    * Change workitemtype response payload

    * Update golden files related to workitemtype response change

* Add onField to the /workitem/event response (#2337)

    * Add event_name attribute to the event response

    * fix golden files related to workitem_event

    * Rename event_name to onField in event/show response

    * Update golden files

* Add migrations for field name rename (#2340)

    * Add migration for workitem_type field renames

    * Add field rename migrations for work_items and work_item_revisions table

* Enable skipped test for field rename and update TSV trigger (#2342)

    * Add migration for TSV vector and triggers for field renames

* Change migration file names
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants