fix: validate presence of price label in issue before checking requir…

[koya0]

…ements

Resolves #99

[koya0]

QA

before commit:

after commit:

[koya0]

@0x4007 could you please review

[0x4007]

Can you link the thread instead of screenshots? Better to QA when we can use it directly.

[0x4007]

What about error aggregation?

[koya0]

as gentlemen said it sounded complex, i just changed the order, but I can make the aggregation, no problem

[koya0]

you think its okay to aggregate only the price label with the errors from checkRequirements? or you mean ALL the errors?

[0x4007]

The idea is that I want to avoid the situation where a user steps through every error and loses patience.

For example:

• you must set your wallet before starting the issue

And then they set their wallet and try start again

• this is not a business priority please start tasks with level 3 priority or higher

I feel like there were other temporary errors I can't recall at the moment. I figured aggregation would prevent this from happening

[koya0]

i ended up aggregating them in feat: aggregate price label and requirements errors , you can take a look at koya0/.ubiquity-os#26

[koya0]

other errors that I thought maybe could be aggregated were "Only collaborators can be assigned to this issue." and "You have reached your max task limit. Please close out some tasks before assigning new ones."

[koya0]

The idea is that I want to avoid the situation where a user steps through every error and loses patience.

For example:

• you must set your wallet before starting the issue

And then they set their wallet and try start again

• this is not a business priority please start tasks with level 3 priority or higher

I feel like there were other temporary errors I can't recall at the moment. I figured aggregation would prevent this from happening

Got it! Its a really good idea. I think the only problem is that it would be a lil complex to aggregate errors from differents plugins

[0x4007]

I think just worry about the errors in this plugin for now.

[koya0]

Can you link the thread instead of screenshots? Better to QA when we can use it directly.

i ended up deleting those issues, but i created other ones:
before - koya0/.ubiquity-os#24
after - koya0/.ubiquity-os#25

[0x4007]

Aggregated errors QA looks good!

#125 (comment)

[gentlementlegen]

QA seems fine: Meniole#59

Please bump @ubiquity-os/plugin-sdk to 2.0.2

Also, it appears that a task with no time estimate can be started if it has a price and a priority, I don't know if that's expected.

[0x4007]

QA seems fine: Meniole#59

Please bump @ubiquity-os/plugin-sdk to 2.0.2

Also, it appears that a task with no time estimate can be started if it has a price and a priority, I don't know if that's expected.

I suppose that technically speaking all we need is a Price label. The Priority and Time labels are just a way for us to determine a price label.

[gentlementlegen]

Even though it works fine, I think that returning a string this way is not really clear, programming side. Why not using AggregateError class and push all the errors there using the logger, and then throw the whole errors altogether which would be handled directly by the SDK? That would also properly break the flow of the program.

[gentlementlegen]

/ask Could this pull-request be rewritten using AggregateError class to compound all the errors together?

[ubiquity-os-beta]

Yes, the pull request can be refactored to utilize the AggregateError class for combining multiple errors. By doing so, you enhance the clarity and maintainability of the error handling mechanism within your project. Instead of throwing individual errors sequentially, AggregateError allows you to collect all pertinent errors and throw them together, which can then be handled more effectively by the SDK.

For example, in your src/handlers/shared/start.ts, you can modify the error handling as follows:

const errors: Error[] = [];

if (!priceLabel) {
  errors.push(new Error("No price label is set to calculate the duration"));
}

if (/* another condition */) {
  errors.push(new Error("Another error message"));
}

if (errors.length > 0) {
  throw new AggregateError(errors, "Multiple errors occurred during the start process");
}

This approach ensures that all encountered issues are reported simultaneously, providing a comprehensive overview of what went wrong. Additionally, it aligns with your project's commitment to code quality by leveraging TypeScript's robust type system and integrating seamlessly with tools like ESLint and Prettier. Make sure to update your Jest tests to handle AggregateError appropriately, ensuring that your testing coverage remains comprehensive.

Implementing AggregateError not only streamlines error management but also improves the user experience by presenting all relevant error information at once, reducing the need for iterative error handling and debugging.

[koya0]

Even though it works fine, I think that returning a string this way is not really clear, programming side. Why not using AggregateError class and push all the errors there using the logger, and then throw the whole errors altogether which would be handled directly by the SDK? That would also properly break the flow of the program.

is this class already implemented?

[gentlementlegen]

You should be able to import it: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AggregateError

[koya0]

I ended up not using AggregateError, because what I would use of it would be AggregateError.errors, which is the same of startErrors array. You can check the new comment format here: koya0/.ubiquity-os#26 (comment)

[gentlementlegen]

The formatting is ok. The thing I wanted to avoid is

cb32e3f#diff-47224fac5bab974df0f6fb9704d6fbb0f06375b3e98285797826104537bda3ccR89

where I would prefer to only throw one error and the SDK would handle the display. Because by doing so, what is the linked error stack? I can't edit the comment so I can't see it.

[0x4007]

I think don't use new Error() because then it prefixes all the messages with "Error: " which is redundant and doesn't look good. Obviously the whole thing is an error.

[koya0]

now I just throw new AggregateError(startErrors). For the display not to be redundant, in SDK you should only get the .message parameter from the Errors

The formatting is ok. The thing I wanted to avoid is

cb32e3f#diff-47224fac5bab974df0f6fb9704d6fbb0f06375b3e98285797826104537bda3ccR89

where I would prefer to only throw one error and the SDK would handle the display. Because by doing so, what is the linked error stack? I can't edit the comment so I can't see it.

[koya0]

I think don't use new Error() because then it prefixes all the messages with "Error: " which is redundant and doesn't look good. Obviously the whole thing is an error.

if I dont use new Error() I guess would have to return a string, which gentlemen said it is not a good option. but this "Error: " prefix can be removed by logging only the .message of the error