Support "Add new instrument name"; Initialize "Upload image"; #164

kunfang98927 · 2024-10-11T15:08:38Z

Support "add new instrument name" by a modal; New names can be submitted to UMIL and optionally to Wikidata;
Initialize "upload image" UI;
Remove "instrument detail" page;
Add "description" field into InstrumentName model;
"Alias" will be stored as instances of InstrumentName;
Fix some small bugs, such as incorrect French translation;
Add more percussions to the instrument list;

Resolves: #150
Resolves: #160
Resolves: #161
Resolves: #163
Resolves: #165

- remove "instrument-detail" page - add two buttons on each instrument card: "Upload new images" & "View on wikidata" - initialize image upload modal UI Refs: #150

Refs: #150

- remove "instrument-detail" page - add two buttons on each instrument card: "Upload new images" & "View on wikidata" - initialize image upload modal UI Refs: #150

Refs: #150

- add "description" & "alias" fields into InstrumentName model; - fix "Non classifiés" in facet search; - support only English and French in facet search; - fix a bug: replace `active_language.en_label == "french"` with `... "French"`; Refs: #160 #161

…load

- add new instrument name, source, description, and alias by a modal; - optionally publish to wikidata; Refs: #163

Refs: #163

web-app/django/VIM/apps/instruments/models/instrument_name.py

web-app/django/VIM/apps/instruments/views/instrument_list.py

web-app/django/VIM/apps/instruments/templates/instruments/includes/stdView.html

Refs: #165

- remove "alias" field from "InstrumentName" model; - move "publish_name" to a separate .py file - use "csrf_protect" decorator for "publish_name" - extract the common code in "mansonryView.html" and "stdView.html" Refs: #163

Ref: #163

dchiller · 2024-10-22T16:29:20Z

web-app/django/VIM/apps/instruments/management/commands/import_instruments.py

@@ -15,7 +15,7 @@ class Command(BaseCommand):

    NOTE: For now, this script only imports instrument names in English and French. It
    also only imports a set of previously-curated instruments that have images available.
-    This list of instruments is stored in startup_data/vim_instruments_with_images-15sept.csv
+    This list of instruments is stored in startup_data/all_instruments_with_16aug_2024.csv


No longer the right file name. You could probably just take out this comment and make the file name a command-line argument or a constant.

dchiller · 2024-10-22T17:01:44Z

I get the following when I try to publish a name:

dchiller · 2024-10-22T17:07:14Z

web-app/django/VIM/apps/instruments/views/publish_name.py

I'm overall confused by the authentication process for wikidata? You are hardcoding a username and password in this view?

Here's the first documentation I find on authentication with wikidata's API: https://www.wikidata.org/wiki/Wikidata:REST_API/Authentication. Not saying it's necessarily the way to go but I'm wondering where this version came from?

I previously referred to the documentation on logging in here: https://www.mediawiki.org/wiki/API:Login#Python
Let me take a closer look.

I think using OAuth is essential, as recommended in the link you provided, to avoid hardcoding credentials. I’ve registered for OAuth and am currently awaiting approval. Based on feedback from last Saturday's project meeting, we also need to offer users a choice between using UMIL’s public account or authorizing their own Wikidata account. I'll create a new issue for this requirement. This pull request can be on hold until the user authentication issue is resolved.

Ref: #163 #174

kunfang98927 · 2025-02-20T18:00:03Z

For "add instrument name" feature, current commits only support using user's own wikidata account to add names.

When developing, in order to test if the wikidata callback URL works properly, I config locally to make sure I can get the response from the callback URL(the callback URL is fixed to "vim.simssa.ca/oauth/callback" and can't be changed). So in this case if we push the code, the OAuth workflow will only work in the production server.

Some important URLs and confidentials related to wikidata should be stored in ".env"(WIKIDATA_URL, WIKIDATA_OAUTH_URL, WIKIDATA_REDIRECT_URI, WIKIDATA_CLIENT_APP_KEY, WIKIDATA_CLIENT_APP_SECRET), I'll edit the ".env" file in the production server once this pull request is merged.

dchiller · 2025-02-21T14:21:08Z

When developing, in order to test if the wikidata callback URL works properly, I config locally to make sure I can get the response from the callback URL(the callback URL is fixed to "vim.simssa.ca/oauth/callback" and can't be changed). So in this case if we push the code, the OAuth workflow will only work in the production server.

When this is merged, information about these configurations should be recorded somewhere so future developers can know what to do when something breaks and they have to fix this :) Probably in this repo's wiki...

kunfang98927 · 2025-02-21T14:25:16Z

When developing, in order to test if the wikidata callback URL works properly, I config locally to make sure I can get the response from the callback URL(the callback URL is fixed to "vim.simssa.ca/oauth/callback" and can't be changed). So in this case if we push the code, the OAuth workflow will only work in the production server.

When this is merged, information about these configurations should be recorded somewhere so future developers can know what to do when something breaks and they have to fix this :) Probably in this repo's wiki...

Yes, of course. I'll write a documentation about this.

dchiller · 2025-02-21T16:45:09Z

web-app/django/VIM/apps/instruments/templates/instruments/index.html

@@ -17,7 +17,7 @@
      <div class="col-xl-3">
        <div class="px-3 pt-3 me-1 mb-3 sidebar-container">
          <div class="info-block p-2 mb-3 notranslate">
-            {% if active_language.en_label == "french" %}
+            {% if active_language.en_label == "French" %}


Similar to #178, I think we need to rethink this hardcoding in the template.

dchiller · 2025-02-21T16:45:22Z

web-app/django/VIM/apps/instruments/templates/instruments/index.html

-                       href="?language={{ language.en_label }}">{{ language.autonym|title }}</a>
-                  </li>
+                  {% comment %} Here only English and French are shown {% endcomment %}
+                  {% if language.en_label == "English" or language.en_label == "French" %}


Why only English and French?

It's because the options of other languages all have 500 server issues...So I temporarily only keep the English and French options.

I just deleted this restriction locally and got no 500 errors.

Yes, I didn't get 500 errors locally, but there is a 500 errors in production.

I would suspect that if showing more languages than french and english is causing 500 errors in production but not locally then either we have already fixed the problem somewhere else and that change has just not propagated to production yet or there is some root cause in the production configuration that we need to uncover. In either case, I think this change will obscure more than it helps.

dchiller · 2025-02-21T16:51:43Z

web-app/django/VIM/apps/instruments/templates/instruments/includes/addNameModal.html

+                       id="publishToWikidataCheckbox"
+                       name="publish_to_wikidata" />
+                <label class="form-check-label" for="publishToWikidataCheckbox">
+                  Do you want to publish also to Wikidata?


In my browser, it's not clear there's a box here to check... when I click the words, a checked checkbox appears, but I don't see a box until that point...

Okay I'll change this checkbox.

Perhaps we don't need this checkbox? All edits should be published directly to wikidata by all means, as UMIL actually doesn't display any detail information of each instrument, right? If this is the case, I can just remove this checkbox.

Good point. What happens if I don't check this box? It looks like we are storing this data in a local database as well... what is the use case for storing in the local database and not on Wikidata?

I just tried it with a language that didn't have a name and this is what I got.

dchiller · 2025-02-21T17:00:13Z

web-app/django/VIM/apps/instruments/static/instruments/js/AddName.js

+        if (result.exists) {
+          languageInput.classList.add('is-invalid');
+          languageInput.classList.remove('is-valid');
+          languageFeedbackInvalid.textContent = `This instrument already has a name in ${languageLabel} (${languageCode}): ${result.name}`;
+          allValid = false;
+        } else {
+          languageInput.classList.add('is-valid');
+          languageInput.classList.remove('is-invalid');
+          languageFeedbackValid.textContent = `This instrument does not have a name in ${languageLabel} (${languageCode}) yet. You can add a new name.`;


I don't think we want to do this...people should be able to add names in languages where a name already exists.

So if a name already exists in wikidata, we will overwrite it? Or we should automatically add names in alias?

So if a name already exists in wikidata, we will overwrite it?

I don't think we want to do that.

Or we should automatically add names in alias?

Maybe? That would make sense to me if we are collecting a new name for an instrument in an already existing language.

OK. So I'll publish it as an alias if a name already exists.

We probably do want to think through what happens if the name provided is already an alias. Since there is no way to see aliases in the current VIM frontend (and even if there was, we probably can't assume people are necessarily paying close attention to it), we might want to check for duplication before we publish...I'm not sure whether Wikidata does this kind of check on its end.

dchiller · 2025-02-21T17:05:16Z

When I am trying to add a new name and click "Do you want to publish to Wikidata?", a button pops up that says "Authorize". Am I supposed to click that? What happens if I click "Publish" before I click "Authorize" (when I just did it, it didn't prompt me to go through the authorization process...). I'm not sure people will know that they're supposed to click the Authorize button first. It seems to me that if they need to some authorization to publish, we should guide them through that workflow.

dchiller · 2025-02-21T17:06:28Z

When I click "Authorize" in the add instrument names modal, I get a 404:

dchiller · 2025-02-21T17:09:27Z

I just went through the Upload instrument image workflow, and I got no obvious errors, but I can't see the image, nor do I see any confirmation that it worked...what happened?

dchiller · 2025-02-21T17:22:12Z

web-app/django/VIM/apps/instruments/templates/instruments/includes/uploadImgModal.html

+                   multiple />
+          </div>
+          <div id="previewImages" class="row g-2"></div>
+          <button type="submit" class="btn btn-primary modal-btn">Upload</button>


How is this submission handled? Right now it looks like it is the default? So a get request is made to the /instruments/ endpoint... but that view doesn't do anything with that?

kunfang98927 · 2025-02-22T01:05:23Z

I just went through the Upload instrument image workflow, and I got no obvious errors, but I can't see the image, nor do I see any confirmation that it worked...what happened?

The image uploading module is still incomplete, with only the UI currently implemented. I'll temporarily comment out the image uploading button until this is complete.

kunfang98927 · 2025-02-22T01:40:47Z

When I am trying to add a new name and click "Do you want to publish to Wikidata?", a button pops up that says "Authorize". Am I supposed to click that? What happens if I click "Publish" before I click "Authorize" (when I just did it, it didn't prompt me to go through the authorization process...). I'm not sure people will know that they're supposed to click the Authorize button first. It seems to me that if they need to some authorization to publish, we should guide them through that workflow.

If you want to publish names to wikidata, yes, you should click "authorize" button. You can't go through the authorization process because this process requires WIKIDATA_URL, WIKIDATA_OAUTH_URL, WIKIDATA_REDIRECT_URI, WIKIDATA_CLIENT_APP_KEY, and WIKIDATA_CLIENT_APP_SECRET, which should be stored in the ".env". But ".env" file should not be pushed to github. So the right way is that I directly edit the file in production server, right?

Also, even if you get these information in ".env", you still can't get the callback information locally because wikidata can only send information to "vim.simssa.ca/oauth/callback"... I don't know if there is a better way that you can go through the whole process...

dchiller · 2025-02-24T15:04:02Z

If you want to publish names to wikidata, yes, you should click "authorize" button.

I think it would make sense to walk people through this process. I feel like the most common approach on websites I visit is that if I try to do something that requires authentication, it will direct me to the authentication process and then back to what I was doing before (for example, I try to buy something online and it asks me to log-in before sending me back to the checkout process, or if I visit my mcgill outlook inbox online after I have been logged out it sends me to the log-in page).

kunfang98927 added 8 commits August 29, 2024 15:44

feat: initialize image upload UI

aa13e25

- remove "instrument-detail" page - add two buttons on each instrument card: "Upload new images" & "View on wikidata" - initialize image upload modal UI Refs: #150

fix: update "upload img" init & remove unused css

eaca2c1

Refs: #150

feat: initialize image upload UI

4af1fee

- remove "instrument-detail" page - add two buttons on each instrument card: "Upload new images" & "View on wikidata" - initialize image upload modal UI Refs: #150

fix: update "upload img" init & remove unused css

e85db70

Refs: #150

Merge branch 'img-upload' of https://github.com/DDMAL/VIM into img-up…

b29e655

…load

feat: add new instrument name

364ff92

- add new instrument name, source, description, and alias by a modal; - optionally publish to wikidata; Refs: #163

fix: prettier fix

4539e3c

Refs: #163

kunfang98927 requested a review from dchiller October 11, 2024 15:08

dchiller reviewed Oct 11, 2024

View reviewed changes

kunfang98927 added 4 commits October 11, 2024 12:29

fix: update prettier version in pre-commit

f564ffb

fix: keep prettier version same as pre-commit

fe5801c

fix: update instrument list (add percussions)

888e84b

Refs: #165

fix: refactor code for "add instrument name"

8079346

- remove "alias" field from "InstrumentName" model; - move "publish_name" to a separate .py file - use "csrf_protect" decorator for "publish_name" - extract the common code in "mansonryView.html" and "stdView.html" Refs: #163

kunfang98927 requested a review from dchiller October 21, 2024 16:39

fix: refactor code for "add instrument name"

bac2cc0

Ref: #163

dchiller reviewed Oct 22, 2024

View reviewed changes

kunfang98927 added 5 commits January 30, 2025 14:16

feat: add wikidata oauth module

b5aa583

fix: fix oauth module

bba826a

feat: "add instrument name" & OAuth

9fa9d74

Ref: #163 #174

fix: revert changes for testing OAuth locally

f60a157

fix: revert changes for testing OAuth locally

efb17d9

kunfang98927 requested a review from dchiller February 20, 2025 17:45

kunfang98927 mentioned this pull request Feb 20, 2025

Remove things which are not working in production #176

Open

dchiller reviewed Feb 21, 2025

View reviewed changes

dchiller mentioned this pull request Feb 27, 2025

500 errors #167

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support "Add new instrument name"; Initialize "Upload image"; #164

Support "Add new instrument name"; Initialize "Upload image"; #164

kunfang98927 commented Oct 11, 2024 •

edited

Loading

dchiller Oct 22, 2024

dchiller commented Oct 22, 2024

dchiller Oct 22, 2024

kunfang98927 Oct 23, 2024

kunfang98927 Nov 1, 2024

kunfang98927 commented Feb 20, 2025

dchiller commented Feb 21, 2025

kunfang98927 commented Feb 21, 2025

dchiller Feb 21, 2025

dchiller Feb 21, 2025

kunfang98927 Feb 21, 2025

dchiller Feb 21, 2025

kunfang98927 Feb 21, 2025

dchiller Feb 24, 2025

dchiller Feb 21, 2025

kunfang98927 Feb 21, 2025

kunfang98927 Feb 21, 2025

dchiller Feb 21, 2025

dchiller Feb 21, 2025

kunfang98927 Feb 21, 2025

dchiller Feb 21, 2025

kunfang98927 Feb 21, 2025

dchiller Feb 24, 2025

dchiller commented Feb 21, 2025

dchiller commented Feb 21, 2025

dchiller commented Feb 21, 2025

dchiller Feb 21, 2025

kunfang98927 commented Feb 22, 2025

kunfang98927 commented Feb 22, 2025

dchiller commented Feb 24, 2025

Support "Add new instrument name"; Initialize "Upload image"; #164

Are you sure you want to change the base?

Support "Add new instrument name"; Initialize "Upload image"; #164

Conversation

kunfang98927 commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

dchiller commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunfang98927 commented Feb 20, 2025

dchiller commented Feb 21, 2025

kunfang98927 commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchiller commented Feb 21, 2025

dchiller commented Feb 21, 2025

dchiller commented Feb 21, 2025

Choose a reason for hiding this comment

kunfang98927 commented Feb 22, 2025

kunfang98927 commented Feb 22, 2025

dchiller commented Feb 24, 2025

kunfang98927 commented Oct 11, 2024 •

edited

Loading