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

feat(fd2): adds entry point for soil disturbance events #318

Merged

Conversation

sidlamsal
Copy link
Contributor

Pull Request Description

Adds:

  • soil disturbance entry point
  • lib.js including tests
  • e2e tests

Closes #249


Licensing Certification

FarmData2 is a Free Cultural Work and all accepted contributions are licensed as described in the LICENSE.md file. This requires that the contributor holds the rights to do so. By submitting this pull request I certify that I satisfy the terms of the Developer Certificate of Origin for its contents.

Co-authored-by: Shahir Ahmed [email protected]

@braughtg braughtg changed the title Add starter code for entry point: soil_disturbance feat(fd2): adds entry point for soil disturbance events Jun 26, 2024
@sidlamsal sidlamsal marked this pull request as ready for review June 28, 2024 14:04
Copy link
Contributor

@braughtg braughtg left a comment

Choose a reason for hiding this comment

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

I have only looked at the getPlantAssets function so far. There are a few suggestions in the code.

library/farmosUtil/farmosUtil.js Outdated Show resolved Hide resolved
library/farmosUtil/farmosUtil.js Outdated Show resolved Hide resolved
@braughtg braughtg marked this pull request as draft June 29, 2024 14:00
@sidlamsal sidlamsal marked this pull request as ready for review July 1, 2024 15:06
Copy link
Contributor

@braughtg braughtg left a comment

Choose a reason for hiding this comment

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

Getting really close. See the embedded comments for a few issues.

@braughtg braughtg marked this pull request as draft July 2, 2024 16:46
@sidlamsal sidlamsal marked this pull request as ready for review July 2, 2024 18:49
@braughtg
Copy link
Contributor

braughtg commented Jul 2, 2024

Some prior tests change the database in ways that can make this test fail. So we just need to ensure that the database is reset before the soil_disturbance/submission.e2e.cy.js test is run. You can do this by adding the following before hook in that test:

  before(() => {
    cy.task('initDB');
  });

@braughtg
Copy link
Contributor

braughtg commented Jul 3, 2024

When manually testing the Soil Disturbance endpoint I noticed a few things:

  • The comment is not being added to the Activity Logs that are created. The comment provided should be added to the log for each pass.
  • The Area (%) is not updated based upon the beds that are selected. For example, if 2/4 beds are selected then the Area (%) should default to 50% of the area and allow the user to adapt it from there.

One other small improvement that I think we should make is:

  • when there are multiple passes, add "Pass 1." or "Pass 2.", etc to the beginning of the comment for each log.

The tests should be also be augmented to check for these things.

@braughtg braughtg marked this pull request as draft July 3, 2024 12:27
@sidlamsal sidlamsal marked this pull request as ready for review July 3, 2024 15:42
Copy link
Contributor

@braughtg braughtg left a comment

Choose a reason for hiding this comment

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

Everything looks good. Just one request for a small test change since farmosUtil.js was also modified here.

@braughtg braughtg marked this pull request as draft July 3, 2024 17:33
@sidlamsal sidlamsal marked this pull request as ready for review July 3, 2024 17:43
@braughtg braughtg merged commit 8978916 into FarmData2:development Jul 3, 2024
Copy link

github-actions bot commented Jul 3, 2024

🎉 This PR is included in version 1.4.0-development.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Soil Disturbance Entry Point
2 participants