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

fix: Write preset tags to observations #514

Merged
merged 3 commits into from
Feb 12, 2021
Merged

fix: Write preset tags to observations #514

merged 3 commits into from
Feb 12, 2021

Conversation

luandro
Copy link
Contributor

@luandro luandro commented Feb 10, 2021

Should solve #513

Add logic to include tags from category and clear tags from old category.

@luandro luandro changed the title Write preset tags to observations fix: write preset tags to observations Feb 10, 2021
@luandro luandro changed the title fix: write preset tags to observations fix: Write preset tags to observations Feb 10, 2021
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

Great stuff Luandro. I've made some suggestions that simplify the code and catch some edge-cases (undefined values).

Since this is quite a complex bit of code now, I think it's good to be really clear with naming variables to help future readers. Here's some ideas:

  • selectedPreset = The preset selected by the user (currently called preset)
  • currentDraftTags = The tags currently defined on draftValue (currently called oldTags)
  • prevPresetTags = The tags from the previously selected preset (if there was one)
  • updatedTags = draftTags with tags from the previously selected preset removed

Worth putting some comments in the code to clarify this.

Maybe avoid the newTags terminology, and instead just keep this as selectedPreset.tags, which is explicit.

Can you also check that these tags that come from the preset are not displayed in the ObservationView component? I don't think they are, since I think we only display tags defined referenced by fields in the preset, but worth checking. We will need to fix this on Desktop, because the Desktop code also shows tags on any observation that are not referenced by fields.

src/frontend/screens/CategoryChooser.js Outdated Show resolved Hide resolved
src/frontend/screens/CategoryChooser.js Outdated Show resolved Hide resolved
src/frontend/screens/CategoryChooser.js Outdated Show resolved Hide resolved
src/frontend/screens/CategoryChooser.js Outdated Show resolved Hide resolved
src/frontend/screens/CategoryChooser.js Outdated Show resolved Hide resolved
@luandro
Copy link
Contributor Author

luandro commented Feb 11, 2021

Thanks for thoughtful review @gmaclennan. Pushed new changes hopefully taking every comment into account.

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

Great, just a couple more things, cleaning up the code.

Looks like the translations are in this branch - can you maybe rebase onto develop?

messages/es.json Outdated Show resolved Hide resolved
src/frontend/screens/CategoryChooser.js Outdated Show resolved Hide resolved
src/frontend/screens/CategoryChooser.js Outdated Show resolved Hide resolved
@luandro
Copy link
Contributor Author

luandro commented Feb 11, 2021

Looking much better now @gmaclennan, great tips 👍

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

Getting there :)
One change, and a question.
Also, I need to get CI working again so that the tests run.

src/frontend/screens/CategoryChooser.js Outdated Show resolved Hide resolved
src/frontend/screens/CategoryChooser.js Show resolved Hide resolved
@gmaclennan
Copy link
Member

Great, I'm waiting for Github actions to run on #515 so that I can test install of the compatible version of NDK

@gmaclennan
Copy link
Member

@luandro I managed to fix the CI build, for more details of the error see #517. I'm wondering what the testing plan is for this to check it works? At a minimum, I would like to do a manual test to ensure that this works as intended, e.g. by using presets that define tags and checking that the created observations have those tags, and also checking changing the preset for an existing observation to ensure that tags are removed as planned.

We are also in dire need of strong unit tests for things like this, and that's certainly possible for this, using https://testing-library.com - if you feel up to it, it would be a great place to start getting unit tests in place.

@gmaclennan gmaclennan merged commit ab5e3b8 into develop Feb 12, 2021
@gmaclennan gmaclennan deleted the fix-preset-tags branch February 12, 2021 19:38
@gmaclennan gmaclennan mentioned this pull request Apr 6, 2021
gmaclennan added a commit that referenced this pull request Apr 20, 2021
* develop:
  feat: Setting to choose coordinate display format (#526)
  chore: Re-apply c267a99 with correct npm@6 version and lockfile v1
  Revert "feat: Updated translations for default configuration (vi, srn)"
  feat: Updated translations for default configuration (vi, srn)
  fix: Update Thai, Khmer & Vietnamese translations
  feat: Add Dutch translations
  feat: Add Sranan Tongo translations
  feat: Add French translations
  fix: Fix cutoff of text on OnePlus6T phone #502 (#511)
  fix: Write preset tags to observations (#514)
  chore: Fix Github CI builds by removing NDK r22 (#516)
  fix: Fix import config button crash (#512)
  feat: Updated translations (vi, es, po) (#498)

# Conflicts:
#	src/backend/package-lock.json
#	src/backend/package.json
#	src/frontend/screens/Settings/Settings.js
gmaclennan added a commit that referenced this pull request Apr 29, 2021
* chore: Fix redirect created when a new version is released

Previous link was broken

* chore: Remove untranslated strings from messages (#496)

Reduces memory use and APK size

* chore(CI): Update JDK setup action (#505)

* note to use specific ndk version

* chore: Improve contributor docs (#509)

* chore: Added instructions to build translations and run the Javascript bundler prior to building for Android

* chore: Added instructions for downloading and preparing offline maps wth the needed structure

* chore: Add detailed instructions on how to download maps and add them to phone

* chore: Add note about react-native run-android not opening bundler on all machines

* chore: Add note about com.mapeo changing with different release versions

* chore: Change asar to full flag name

* fix: Localize "Import Config" button (#510)

* fix: Change hardcoded message to intl FormattedMessage

* Add missing string to translations

* chore: prettier

* extract messages

Co-authored-by: Gregor MacLennan <[email protected]>

* feat: Updated translations (vi, es, po) (#498)

* New translations en.json (Vietnamese)

* New translations en.json (Vietnamese)

* New translations en.json (Vietnamese)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Spanish)

* New translations en.json (Portuguese, Brazilian)

Co-authored-by: Gregor MacLennan <[email protected]>

* fix: Fix import config button crash (#512)

* fix: Fix import config button crash

* Update version in package-lock

Co-authored-by: luandro <[email protected]>

* chore: Fix Github CI builds by removing NDK r22 (#516)

See #517 for explanation
An update to the Github Action virtual environment added NDK r22,
which CMake uses, ignoring $ANDROID_NDK_HOME.
Solution: Remove NDK r22 and update to latest NDK r21f

* fix: Write preset tags to observations (#514)

* fix: Add better variable naming and easier to understand logic

* fix: Cleaner and easier to understand logic on updating tags

* fix: Clean redundant code and sort updated value order in preparation for edge cases

* fix: Fix cutoff of text on OnePlus6T phone #502 (#511)

* fix: Fix cutoff of text on OnePlus6T phone #502

Use a custom Text component with fontFamily explicitly set, to work around bug facebook/react-native#15114 (comment)

* Fix implementation of style merging

* fix bugs

* feat: Add French translations

* feat: Add Sranan Tongo translations

* feat: Add Dutch translations

* fix: Update Thai, Khmer & Vietnamese translations

* feat: Updated translations for default configuration (vi, srn)

* Revert "feat: Updated translations for default configuration (vi, srn)"

This reverts commit c267a99.
Install was done using npm@7 which changed the package-lock format and broke the build

* chore: Re-apply c267a99 with correct npm@6 version and lockfile v1

* chore: Prepare release v5.2.0

Co-authored-by: Kira Oakley <[email protected]>
Co-authored-by: luandro <[email protected]>
Co-authored-by: Digidem Bot <[email protected]>
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