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

LGC-39 Udix remove print #836

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

LGC-39 Udix remove print #836

wants to merge 15 commits into from

Conversation

poacherone
Copy link
Contributor

No description provided.

@poacherone
Copy link
Contributor Author

Just ignore the commits history, I didn't realise a bunch of old/irrelevant changes stuff was left there.

Copy link
Contributor

@kwahlin kwahlin left a comment

Choose a reason for hiding this comment

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

Shouldn't there be more records in Udix_ID2.txt? Also, those three records are already deleted. Maybe it's the wrong file?

@poacherone
Copy link
Contributor Author

poacherone commented Jan 28, 2021

It was a tmp file, just to test the script. I wasn't sure if it was needed for a code review. But it is now updated with the correct IDs.

Added correct IDs
@kwahlin
Copy link
Contributor

kwahlin commented Jan 28, 2021

Looks good then! Maybe you should check also if the property is left empty after removing the "Externt magasin" object, and if that's the case, remove the property from graph[1]. If you think there are cases where associatedMedia/marc:hasTextualHoldingsBasicBibliographicUnit contains only one ("Externt magasin") object?

Copy link
Member

@andersju andersju left a comment

Choose a reason for hiding this comment

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

Looks fine, just a couple of minor things you might want to look at. Also be mindful of indentation (makes it easier to read / spot problems). And +1 on what Kalle wrote :)

Comment on lines +1 to +2
PrintWriter failedUpdating = getReportWriter("failed-updating")
PrintWriter scheduledForUpdating = getReportWriter("scheduled-updates")
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK created/modified/deleted are now default reports, created automatically, so these shouldn't be necessary. (Which means scheduledForUpdating and failedUpdating below can also be removed.)

def holds = hold.graph[1]

propertiesToCheck.each { prop ->
holds[prop].removeIf { it["marc:publicNote"].contains("Externt magasin / Closed stacks") }
Copy link
Member

Choose a reason for hiding this comment

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

If holds doesn't have prop, or it doesn't have "marc:publicNote", you'll get a NullPointerException, so being a bit more defensive might be in order. E.g.,
holds[prop]?.removeIf { it["marc:publicNote"]?.contains("Externt magasin / Closed stacks") || false }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

Comment on lines +8 to +9
selectByIds( HoldIDs.readLines() ) { hold ->
def holds = hold.graph[1]
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: the use of hold (an individual hold) vs holds (here a part of that individual hold) is slightly confusing. Maybe "holds" => "item" would be better. (Doesn't matter for a one-off script, but for the future 😄).

@poacherone
Copy link
Contributor Author

Looks good then! Maybe you should check also if the property is left empty after removing the "Externt magasin" object, and if that's the case, remove the property from graph[1]. If you think there are cases where associatedMedia/marc:hasTextualHoldingsBasicBibliographicUnit contains only one ("Externt magasin") object?

In general this is probably a very good idea, but the case here is that if there is only one object in associatedMedia/marc:hasTextualHoldingsBasicBibliographicUnit ("Externt magasin") the entire holdings record should be removed. That will be done separately (i.e. registervårdsklienten).

@kwahlin
Copy link
Contributor

kwahlin commented Jan 29, 2021

Looks good then! Maybe you should check also if the property is left empty after removing the "Externt magasin" object, and if that's the case, remove the property from graph[1]. If you think there are cases where associatedMedia/marc:hasTextualHoldingsBasicBibliographicUnit contains only one ("Externt magasin") object?

In general this is probably a very good idea, but the case here is that if there is only one object in associatedMedia/marc:hasTextualHoldingsBasicBibliographicUnit ("Externt magasin") the entire holdings record should be removed. That will be done separately (i.e. registervårdsklienten).

I see! That was the point with writing a script right...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants