Skip to content

Allow embargoed products to still be listed#48

Merged
figuernd merged 4 commits into
masterfrom
nf/embargo-better
Mar 30, 2026
Merged

Allow embargoed products to still be listed#48
figuernd merged 4 commits into
masterfrom
nf/embargo-better

Conversation

@figuernd

Copy link
Copy Markdown
Contributor

Embargoed products are now listed on the cruise menu, but only metadata (date, id) is available and message displayed indicating how to get access.

Also adds checks to ensure exported events never include those belonging to a parents lowering or cruise that is hidden. Somewhat complex checks involved because of the nature of the database.

cruise-embargo lowering-embargo

@figuernd figuernd requested a review from rgov March 13, 2026 20:20

@rgov rgov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm taking your word for it that it works. It seems relatively straightforward.

I worry about the implicit constraints that we have that, say, there aren't overlapping cruises. Having to scan the database every time (on ever request?) to map lowering -> cruise is also very inefficient. I don't think we have a database index for this. The database layer needs to be gutted...

Comment thread lib/access_control.js
Comment thread lib/access_control.js Outdated
Comment thread routes/api/v1/event_aux_data.js
@figuernd

Copy link
Copy Markdown
Contributor Author

I worry about the implicit constraints that we have that, say, there aren't overlapping cruises.

That one is pretty safe since it's unlikely Alvin will ever be on two cruises simultaneously, but it's a house of cards yeah.

Having to scan the database every time (on ever request?) to map lowering -> cruise is also very inefficient. I don't think we have a database index for this. The database layer needs to be gutted...

Have an agent working on that... hopefully figures it out by the time I wake up next week.

@figuernd figuernd requested a review from rgov March 24, 2026 20:18
@figuernd

Copy link
Copy Markdown
Contributor Author

Also cleaned up CI so it can actually build + test on PRs now without attempting a Dockerhub push.

@rgov

rgov commented Mar 25, 2026

Copy link
Copy Markdown
Member

Part of the reason to push to Docker for all PRs was so it could be pulled on an instrument server fairly easily. Is there value in that?

@figuernd

Copy link
Copy Markdown
Contributor Author

Part of the reason to push to Docker for all PRs was so it could be pulled on an instrument server fairly easily. Is there value in that?

Could be useful, but it always failed on PRs (at least on the client side?) because of some auth issue: https://github.com/WHOIGit/ndsf-sealog-client/actions/runs/23510179471/job/68428719568

I think it's using auth you setup and presume your account would have to fix.

@rgov

rgov commented Mar 26, 2026

Copy link
Copy Markdown
Member

For PRs (pull_request target), GHA doesn't allow access to the secrets, so it won't have tokens for push. Makes sense.

@figuernd

Copy link
Copy Markdown
Contributor Author

@rgov good to merge then?

@rgov

rgov commented Mar 27, 2026

Copy link
Copy Markdown
Member

Sure

@figuernd figuernd merged commit 478f9e8 into master Mar 30, 2026
4 checks passed
@figuernd figuernd deleted the nf/embargo-better branch March 30, 2026 16:58
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