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

Add support for destroying a custom field #109

Merged
merged 7 commits into from
Feb 1, 2023
Merged

Conversation

HashidaTKS
Copy link
Contributor

@HashidaTKS HashidaTKS commented Jan 25, 2023

GitHub: fix GH-108

If a searchable CustomField is destroyed, corresponding FullTextSearch::Target records are also destroyed.
We can't use existing mapper mechanism for it because CustomField uses
has_many :custom_values, :dependent => :delete_all relation. It doesn't call after_destroy callback that is
needed to synchronize CustomFieldValue and FullTextSearch::Target.

We can remove orphaned FullTextSearch::Target for destroyed CustomFieldValue by
bin/rails full_text_search:synchronize.

@HashidaTKS HashidaTKS marked this pull request as draft January 25, 2023 09:01
@kou
Copy link
Member

kou commented Jan 25, 2023

We should remove entries that refer nonexistent Redmine records instead of not touching them on search.

@HashidaTKS
Copy link
Contributor Author

HashidaTKS commented Jan 26, 2023

Thanks!
I will try to fix current changes to remove entries that refer nonexistent records.


class RedmineCustomFieldMapper < RedmineMapper
class << self
def with_project(redmine_class)
Copy link
Contributor Author

@HashidaTKS HashidaTKS Jan 27, 2023

Choose a reason for hiding this comment

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

Question: Do we need to do something in with_project? I think this Mapper is not be used with project because custom field is independent from project.

Copy link
Member

Choose a reason for hiding this comment

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

with_project is for mapper that needs JOIN to collect related projects.

def with_project(redmine_class)
end

def not_mapped(redmine_class, options)
Copy link
Contributor Author

@HashidaTKS HashidaTKS Jan 27, 2023

Choose a reason for hiding this comment

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

Question: What is not_mapped? When we need this method? Do we need to do some thing in this method?

Copy link
Member

Choose a reason for hiding this comment

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

not_mapped is for collecting Redmine records that are not mapped (corresponding FullTextSearch::Target doesn't exist). It's used to synchronize Redmine records and `FullTextSearch::Target.

end
end

class FtsCustomFieldMapper < FtsMapper
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are nothing to do.
So, maybe it is better to not define this class and use FtsMapper directly.

@HashidaTKS
Copy link
Contributor Author

Concern: This changes don't remove already left fts records. (The error is not resolved only apply this changes, already left fts records should be removed.)

@HashidaTKS
Copy link
Contributor Author

HashidaTKS commented Jan 31, 2023

Concern: This changes don't remove already left fts records. (The error is not resolved only apply this changes, already left fts records should be removed.)

When we should remove the already left (orphan) fts records?
I thinks 2. is better.

  1. When migration
  2. When startup Redmine
  3. When removing a field, which means that removing not only fts records using the field, but also orphan fts records not using the field.

@HashidaTKS
Copy link
Contributor Author

@kou

Would you review this when you have time?
Do you have any good idea for #109 (comment) ?

@kou kou changed the title [WIP] Bug: Removed custom fields targeted Add support for removing a custom field Feb 1, 2023
@kou kou marked this pull request as ready for review February 1, 2023 07:57
@kou kou changed the title Add support for removing a custom field Add support for destroying a custom field Feb 1, 2023
@kou kou merged commit 04a810d into master Feb 1, 2023
@kou kou deleted the support-removed-custom-field branch February 1, 2023 08:00
@kou
Copy link
Member

kou commented Feb 1, 2023

bin/rails full_text_search:synchronize removes orphan FullTextSearch::Target.

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.

削除したカスタムフィールドの単語を検索時、エラー
2 participants