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

3032 gobierto data add custom field license for dataset #3821

Merged

Conversation

stbnrivas
Copy link
Contributor

@stbnrivas stbnrivas commented Apr 15, 2021

✌️ What does this PR do?

Closes #3032

add to module GobiertoData (for every dataset)

  • new vocabulary license: Open Data (ODbl), Open Data (ODC-By), Open Data (PDDL)
  • new custom_field source (source of data were obtained )
  • new custom_field source-url (url to the font)

refactor zeitwerk compliance remove dirs db/seeds/modules to db/seeds/gobierto_seeds/

🔍 How should this be manually tested?

  1. for Gobierto installation with existing sites which module GobiertoData are enabled, new custom fields should be enable after apply

  2. for Gobierto installation without existing sites, you should run a feed from console, to enable new custom fields

bin/rails c
site = Site.find_by(domain: "madrid.gobierto.test")
GobiertoSeeds::GobiertoData::Recipe.run(site) if site.configuration.gobierto_data_enabled?
  1. refactor mv for db/seeds/modules to db/seeds/gobierto_seeds/gobierto_{data,people,plans}
bin/rails c
site = Site.find_by(domain: "madrid.gobierto.test")
GobiertoSeeds::GobiertoPeople::Recipe.run(site)
GobiertoSeeds::GobiertoPlans::Recipe.run(site)

👀 Screenshots

Before this PR

before-backoffice

before-frontoffice

After this PR

after-backoffice

Screenshot from 2021-04-19 07-43-44

@stbnrivas stbnrivas force-pushed the 3032-gobierto-data-add-custom-field-license-for-dataset branch from 21b0dfd to e52ada2 Compare April 15, 2021 12:15
@stbnrivas stbnrivas force-pushed the 3032-gobierto-data-add-custom-field-license-for-dataset branch from 7afe511 to 3be2115 Compare April 16, 2021 10:45
@@ -22,6 +22,27 @@
:label="labelSubject"
:text="categoryDataset"
/>
<InfoBlockText
Copy link
Member

Choose a reason for hiding this comment

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

@Crashillo @stbnrivas it would be good to show both source text and url as a link (with target blank), and not in two separated fields, don't you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

In order to do so, you should either create a new component or parametrize the current one

Copy link
Member

Choose a reason for hiding this comment

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

yes, could you help @stbnrivas ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we already create a new component to show as a link . Also I opened a new issue with another way to do it.

#3830

if vocabulary.new_record?
vocabulary.name_translations = { ca: "Llicència", en: "License", es: "Licencia" }
vocabulary.save
vocabulary.terms.create(name_translations: { ca: "Open Data (ODbl)", en: "Open Data (ODbl)", es: "Open Data (ODbl)" }, position: 1)
Copy link
Member

Choose a reason for hiding this comment

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

I'd add an option for not known (maybe the first option or default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can I name as:

ca: "Llicència desconeguda"
en: "Unknown license"
es: "Licencia desconocida"

Copy link
Member

Choose a reason for hiding this comment

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

ok!

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to allow to not select a value, and if so, don't show the field.

dataset_source_url.save
end

licenses = site.custom_fields.vocabulary_options.where(class_name: "GobiertoData::Dataset").find_or_initialize_by(uid: 'dataset-license')
Copy link
Member

Choose a reason for hiding this comment

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

why plural name in the variable?

@@ -15,7 +15,7 @@ ca:
- dv
- ds
abbr_month_names:
-
-
Copy link
Contributor

Choose a reason for hiding this comment

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

@ferblape is it possible that this always happen due to the missing tabulator char? Perhaps, should it be??

abbr_month_names:
    - gen
    - feb

Just asking

Copy link
Contributor Author

@stbnrivas stbnrivas Apr 19, 2021

Choose a reason for hiding this comment

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

I'm not sure if the guilty is:

husky > pre-commit (node v14.16.1)
✔ Preparing...
❯ Running tasks...
↓ No staged files match app/javascript//*.{js,vue} [SKIPPED]
↓ No staged files match app/
/.{css,scss} [SKIPPED]
❯ Running tasks for config/locales/**/
.yml
⠇ npm run check_i18n
◼ Applying modifications...
◼ Cleaning up...

Another guilty is my Atom default configuration because. Atom is removing all withespaces at end of line every time that save the file.

  1. I will disable (auto delete end whitespaces)
    https://flight-manual.atom.io/using-atom/sections/editing-and-deleting-text/#whitespace
  2. I will enable highlights spaces like this
    https://atom.io/packages/trailing-spaces

Copy link
Contributor

Choose a reason for hiding this comment

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

No, no, we know the check_i18n script is the reason to set/unset the tabulation. But it's a recurrent issue that I cannot figure out how it happens, sometimes there's empty row, sometimes there isn't :(

Copy link
Member

Choose a reason for hiding this comment

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

it's not an empty row but an empty space in the row, I don't know what causes it, probably an editor linter?

Comment on lines 72 to 73
sourceDataset: Font
sourceDatasetUrl: Font url
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
sourceDataset: Font
sourceDatasetUrl: Font url
sourceDataset: Source
sourceDatasetUrl: Source url

@@ -68,6 +69,8 @@ es:
sets: Conjuntos
showAll: Ver todo
showLess: Ver menos
sourceDataset: Fuente
sourceDatasetUrl: Link Fuente
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the consistency between translations, use Fuente url instead

@stbnrivas stbnrivas force-pushed the 3032-gobierto-data-add-custom-field-license-for-dataset branch from 7774f61 to 69d8fbc Compare April 19, 2021 17:51
@@ -26,6 +27,7 @@ def site

def test_seed_a_site_with_that_module
@subject.seed("GobiertoPeople", site)
byebug
Copy link
Member

Choose a reason for hiding this comment

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

Careful

@stbnrivas stbnrivas force-pushed the 3032-gobierto-data-add-custom-field-license-for-dataset branch from 69d8fbc to c36caa3 Compare April 20, 2021 08:49
@stbnrivas stbnrivas force-pushed the 3032-gobierto-data-add-custom-field-license-for-dataset branch from c36caa3 to 2ba5088 Compare April 20, 2021 10:55
@ferblape ferblape merged commit b080e78 into master Apr 22, 2021
@ferblape ferblape deleted the 3032-gobierto-data-add-custom-field-license-for-dataset branch April 22, 2021 02:54
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.

Data / New fields for datasets: Source and License
4 participants