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

Fix/subject keywords #5687

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Fix/subject keywords #5687

wants to merge 5 commits into from

Conversation

MSDrao
Copy link
Contributor

@MSDrao MSDrao commented Jan 4, 2025

Pull Request Checklist:

  • Positive Test Case Written by Dev
  • Automated Testing
  • Sufficient User and Developer Documentation
  • Passing Jenkins Build
  • Peer Code review and approval

Positive Test Case

  1. [Enter positive test case here]

@MSDrao MSDrao requested a review from devincowan January 4, 2025 17:05
@MSDrao MSDrao self-assigned this Jan 4, 2025
Copy link

github-actions bot commented Jan 4, 2025

Test Results

    2 files  ±0      2 suites  ±0   1h 30m 30s ⏱️ - 2m 17s
1 415 tests ±0  1 399 ✅ ±0  16 💤 ±0  0 ❌ ±0 
1 527 runs  ±0  1 509 ✅ ±0  18 💤 ±0  0 ❌ ±0 

Results for commit cbcc7bf. ± Comparison against base commit a753301.

♻️ This comment has been updated with latest results.

@devincowan
Copy link
Contributor

@MSDrao please link your PR to the relevant issue. In this case, #5638

Copy link
Contributor

@devincowan devincowan left a comment

Choose a reason for hiding this comment

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

Attempting to add a keyword results in a POST request to http://localhost:8000/hsapi/_internal/undefined/subject/add-metadata/
☝️ your code is not correctly identifying the resIdShort
image

@MSDrao MSDrao linked an issue Jan 8, 2025 that may be closed by this pull request
@MSDrao MSDrao requested a review from devincowan January 9, 2025 14:42
if(newKWLength > 500){
this.newKeyword = this.newKeyword.slice(0, -1);
event.preventDefault();
this.error = "Warning: You have exceeded the character 500 character limit for the subject keywords field. In a future release you will be unable to continue adding keywords beyond the 500 character limit";
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to update this message to match the one that Clara suggested here:
#5638 (comment)

Comment on lines +596 to +621

if element_name.lower() == "subject":
original_value = request.POST.get("value", "")
keywords = original_value.replace(",", "")
# Make request.POST mutable
mutable_post = request.POST.copy()
mutable_post["value"] = keywords
request.POST = mutable_post

handler_response = signals.pre_metadata_element_create.send(
sender=sender_resource, element_name=element_name, request=request
)

if element_name.lower() == "subject" and original_value:
mutable_post["value"] = original_value # Reassign original value
request.POST = mutable_post

for receiver, response in handler_response:
if "is_valid" in response:
if response["is_valid"]:
element_data_dict = response["element_data_dict"]

if element_name == "subject":
if original_value:
element_data_dict["value"] = original_value

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks here like you're trying to handle comma delimited entries.
I'm not convinced that you need to modify this __init__.py file at all.
I think that if you modify your newKeywordsLength function so that it is accurate, you can remove all of the modifications that you are making here.

Comment on lines +110 to 113
newKeywordsLength: function () {
let newVal = this.resKeywords.join("") + this.newKeyword;
return newVal.length;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
newKeywordsLength: function () {
let newVal = this.resKeywords.join("") + this.newKeyword;
return newVal.length;
}
newKeywordsLength: function () {
let newVal = this.resKeywords.join(",") + this.newKeyword;
return newVal.length;
}

☝️ perhaps this will give you a more accurate count of the KW length?

Copy link
Contributor Author

@MSDrao MSDrao Jan 10, 2025

Choose a reason for hiding this comment

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

So, is 500 characters limit including comma ? I thought the 500 char limit is without including comma. @devincowan

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.

Can't remove subject keyword from resource
2 participants