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

Several CKAN integration fixes #337

Merged
merged 1 commit into from
Feb 27, 2021

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented Feb 25, 2021

Problems

  • CKAN doesn't get pull requests for around half of new mods with the CKAN badge enabled.
    The following steps lead to no notification sent:
    1. Create mod
    2. Edit mod, check CKAN badge, "Save Changes"
    3. Edit mod, "Edit & Publish"
  • The "Add to CKAN" checkbox is hidden after it has been enabled once. Authors thus can't verify whether they checked it and might get confused; they also can't disable it again for unpublished mods, even though it wouldn't create any problems there.
  • Once the checkbox is activated, it's set in stone. However sometimes it happens that CKAN doesn't index a mod it gets a PR for for whatever reason. It would be nice if admins could deactivate the checkbox again.
  • For some reason the tooltip for the CKAN checkbox doesn't work anymore.
  • The "(un)locked" and "edit" notifications don't get sent.

Causes

  • The second time you edit a mod the checkbox isn't included in the HTML, thus request.form.get("ckan") is None in the POST. So ckan is False, and we never get to the send_to_ckan() part. Even if it weren't, mod.ckan would be already true the second time and prevent it as well.
  • When we reach notify_ckan() for "locked" and "unlocked", mod.published is False. Thus the notification isn't sent.

Changes

  • The CKAN logic in /mod/<int:mod_id>/<path:mod_name>/edit is reworked.

    • If the badge has just been activated, send the mod to CKAN (send_to_ckan() will take care of cancelling if the mod isn't published yet)
    • If the badge has already been activated before, but the mod has been published just now, send it to ckan.
    • If the badge has been activated before (and the mod has already been published previsouly), notify CKAN about an edit.
  • The checkbox can be disabled again, as long as the mod isn't published yet or you're an admin. Yes, authors can also disable the checkbox if the mod is unpublished due to it being locked, but I think this is fine, there's a decent change CKAN froze the mod already anyways (if they got the notification).

  • The HTML around the CKAN checkbox has been polished. The checkbox is shown all the time on the edit page, pre-checked if it has already been activated. However it's locked for users if the mod is published. The toolltip is replaced by a hidden, more detailed help text shown on button press.
    image

  • There's a new force parameter in notify_ckan(), defaulting to False. It overrides (mod.published). We set it to True when calling in lock() and unlock(), and also delete() even though it isn't necessary there but let's guard against future changes.

  • The command for celery in the docker-compose files is fixed. Since version 5.0 the --app/-A option needs to be before the worker argument. I've had this in my working directory for some time apparently and forgot about it. The systemd files are fine, they already have it in that order.

  • Postgres is no longer locked to the minor version, only to the major. It doesn't match current production anyway (11.3, bad, I know), and it makes sense to stay up to date locally so we can detect problems early.

  • The Secure cookie settings for Flask are only enabled if app.debug is False, to allow local testing with Chrome. Need to think of a solution for the cookies set dynamically via JS.

@DasSkelett DasSkelett added Type: Bug Area: Backend Related to the Python code that runs inside gunicorn Priority: Normal Status: Ready Area: Frontend Related to HTML, JS, CSS, or other browser things labels Feb 25, 2021
templates/edit_mod.html Outdated Show resolved Hide resolved
templates/edit_mod.html Outdated Show resolved Hide resolved
@HebaruSan
Copy link
Contributor

HebaruSan commented Feb 25, 2021

I'm trying to test this locally with ./start-server.sh, but at http://localhost:5080/ logging in does nothing; I thought maybe because of HTTPS, but https://localhost:5443/ won't connect at all. Does this script still work?

OK, commenting out the _SECURE settings in app.py seems to have helped, I'm in now.

Copy link
Contributor

@HebaruSan HebaruSan left a comment

Choose a reason for hiding this comment

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

The actual code changes here seem to work and they make sense to me. 👍
You can accept or reject the UI suggestions as you like.

@DasSkelett
Copy link
Member Author

I'm trying to test this locally with ./start-server.sh, but at http://localhost:5080 logging in does nothing; I thought maybe because of HTTPS, but localhost:5443 won't connect at all. Does this script still work?

OK, commenting out the _SECURE settings in app.py seems to have helped, I'm in now.

Ugh, you're using Chrome, right? So Chrome, contrary to Firefox, does not ignore the Secure option on localhost. So hiding behind not app.debug it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Backend Related to the Python code that runs inside gunicorn Area: Frontend Related to HTML, JS, CSS, or other browser things Priority: Normal Status: Ready Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants